-
Notifications
You must be signed in to change notification settings - Fork 386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Removing the compiled single-file build #782
Comments
Hi @jtojnar , |
@Simounet Yes, I want to remove the compilation since it just too much hassle, especially once we start adding more dependencies. I would recommend using Composer and either committing the files into the repository like Styx does, or providing users a zip file for easy installation (like we do in selfoss). |
I understand. Sounds like a plan. I have to test it on my side. Thanks for your proposals! |
Thanks for the note @jtojnar |
@jtojnar Thank you for bringing up this topic. Because this is a project of WordPress(?) we should also invite them to this discussion (Ping @jrfnl, @rmccue) However, I see this issue as not just whether or not to keep the single-file build, but what BC means in this project in general. Depending on this definition, it then follows that a single-file build can be preserved or not, because the "feature" of having a single-file build is only a consequence of not having mandatory dependencies to external libraries. I'll try to compile all the statements about the BC that we have so far.
@jtojnar said:
As you pointed out (and one can see in #777) it is possible to keep the dependency to the PSR-18 interface fully optional. Personally I would also prefer to make them a requirement. According to Semver that would not be a breaking change, similar with dropping support for PHP versions (aka "require a newer PHP version"). But: To ensure BC in version 1.x without adding requirements to external interfaces/libraries, a reasonable way for me is to create new layers that allow alternative libraries and deprecate old classes. The layers would also not mean "duplicating the interfaces", but allows us to prune the interfaces to a small subset that we need. We can even change or throw them away at any time because they are marked as For me a deprecation means that I don't have to actively care about it anymore (as long as all tests work). If users report a bug in the deprecated classes, I would point out the alternative or ask for a PR. I would not allow to add new features to deprecated classes to increase the pressure on switching to the alternative.
Theses projects where only some results of a search in Github. There might be even more projects outside of Github that we don't know. Another data point to look at are the download numbers of the build file. But keep in mind that there may be users who create the build themselves using
I would very very strongly advise against defining 3rd-party interfaces in the library, even as wrapping them inside Providing the PSR interfaces inside SimplePie also contradicts your earlier statement. We would definitely "duplicating the interfaces and adding extra code for bridging that we would have to maintain" forcing us to implement methods we don't need but without having the freedom to prune or change them anytime. Saying this I would summarize all this to one question: Is the fact of not having mandatory dependencies to other libraries a part of the BC promise or not? |
Well said. Both Semver and Symfony guidelines really only speak about API, not artifacts. I think it would be reasonable to officially adopt Symfony guidelines, which would allow us to drop the compiled file. (Although they are a bit more strict and would not allow #775.)
Yes, it is a reasonable approach but not one I wanted discuss here to keep the scope of the issue narrow. Suffice to say, I believe we could have less code while still maintaining the Symfony promise. Will discuss it more in #774.
I would not expect many people to build it themselves but, even if we are generous and select 1.6.0, which has the most recorded downloads, assume every project downloads it once assume half the people build it themselves, there will only be ~200 projects using the file. On the other hand, just Packagist lists 75 depending projects, which still leaves millions of extra downloads for projects that are not on Packagist.
Yes, I am aware of this but we would only be providing the compiled code as a courtesy to the very few projects too stubborn to use Composer. This issue would not affect people doing proper dependency management. And we could add a disclaimer next to the file. On the other hand, if we do not track our dependencies using Composer, we will still “avoid all composer version negotiation” jbboehr/php-psr#88 warns against, only affecting everyone.
We would not provide them inside SimplePie, only inside of a single cursed build artifact. Some extra code is inevitable but I would much prefer adding it to a build system, where it is not bound by any contract, over enlarging the API surface.
I think the BC aspect of this was already resolved by deciding raising minimum PHP version is not part of BC promise. So the only question that remains here is whether SimplePie should be allowed to depend on third-party libraries at all. And I want to say is it is pretty much settled that dependency management has its place in modern software engineering, and the minor complexity/overhead is greatly overshadowed by the ability to use high quality components that are widely available, instead of recreating them from scratch for each project. Looking at SimplePie NG, I think this is also what @skyzyx believes. So the matter for discussion is how should we support the hold outs that do not want to use Composer. Ideally, in a way that will not hold us back (too much). |
I've tried to outline the scenarios that SimplePie's development can take. Based on the BC promise decision different consequences are possible. I tried to summarized them in the table below, but do not claim to be complete. Used symbols: 🪶 = optional dependency; 🪢 = mandatory dependency; 🏗️ = work needed; ✅ = work has been done; 🔥 = something can be removed; ❌ = something went very bad
Scenario A: Having no mandatory PSR libs is part of BC promiseI assumed that this scenario A is the actual BC promise of SimplePie. I also based my refactoring roadmap (#731) on this assumption. Therefore I never tried e.g. in #742 to add the dependency to Because I assumed the dependencies as optional in v1 I've created the interop layers for PSR-16 Cache, which can be deleted again in v2 2️, together with all deprecations 3️ (I'm most looking forward to this part 😊). In v2 the dependency on PSR will be mandatory 1️, so I planned to remove the single-file artifact in v2 4️. That's what I assumed and what I'm fine with. But if having no mandatory PSR dependencies is not part of the BC promise, then some things change, see Scenario B. Scenario B: Having no mandatory PSR libs is not part of BC promiseIf we could add mandatory PSR dependencies right now in v1 1️, then this would make things easier. We would not have to create the interop layers for PSR-18, but could simply require the implementations and work with them directly. We could remove the interop layer for PSR-16 2️. For BC we would have to keep the Cache (already deprecated), File and IRI classes, but don't have to use them internally3️. And we have to drop support for the single-file artifact 4️. (If we want to keep it, see Scenario C) For SimplePie v2 we would only have to remove all deprecated classes and methods 3️ and everything would be ready. This scenario would save us some work and personally this scenario is my favorite. 🙂 But I know that there are more reasons and parties to consider. Scenario C: Having no mandatory PSR libs is not part of BC promise, but keep single-file artifactThis scenario C is nearly identically with scenario B, but has a slightly difference that would have bigger consequences. Like in scenario B we could add the PSR dependencies as a requirement in v1 1️, could remove the interop layers for PSR-16 2️. For BC we would have to keep the Cache (already deprecated), File and IRI classes 3️, but don't have to use them internally. But: We want to keep support for the single-file artifact. That could mean to bundle the required PSR interfaces inside the single-file artifact. That would require some complete new techniques, like using I personally have no interest in maintaining such a file. 😐 Scenario D: Having mandatory PSR libs is not allowed at allThis scenario is based on this quote of @jtojnar:
For this scenario I assumed that having mandatory PSR libs is not allowed at all. This scenario would irritate me a lot, as it contradicts all previous wishes for replacement of the Because the PSR libs would not be mandatory 1️, we would also not be able to deprecate the I therefore think that the question "whether SimplePie should be allowed to depend on third-party libraries at all" does not arise at all. The answers seems to be "Yes, it is allowed to depend on third-party libraries at all". I personally think that scenario D can therefore be discarded. Some words about using composer or not: Users who do not use Composer do not hold us back. There may be few good reasons why people don't want to use Composer, and that's perfectly ok. Composer doesn't do anything that you couldn't do manually by hand if you wanted to. Also, no one has argued that Composer should not be used for dependency management, but that the single-file artifact should be remain. These people usually have a copy/fork of SimplePie and have the competence to remove unwanted features or dependencies if they want to. But to say that these users are holding back the development of SimplePie is wrong. That's nothing I want to discuss here. |
We plan to make SimplePie’s HTTP fetcher component replaceable with implementations of the PSR-18 contract. In order to do so, we would need to add a dependency on
psr/http-client
Composer library.Now while is it possible to avoid mandatory dependency on the library, it would mean duplicating the interfaces and adding extra code for bridging that we would have to maintain, complicating the codebase
foreveruntil next BC break window.@Art4 pointed out that @Simounet’s LeedRSS and @ophian’s styx still use the bundle so I want to ask if removal of the bundle would be problematic for you.
Alternately, we could stuff the interfaces into the bundle, wrapping them in
if (!class_exists('...'))
blocks but that could hide dependency version mismatches if app pulls a newer version of the interface.The text was updated successfully, but these errors were encountered: