Skip to content
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

Open
jtojnar opened this issue Jan 31, 2023 · 7 comments
Open

Removing the compiled single-file build #782

jtojnar opened this issue Jan 31, 2023 · 7 comments

Comments

@jtojnar
Copy link
Contributor

jtojnar commented Jan 31, 2023

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 forever until 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.

@Simounet
Copy link

Hi @jtojnar ,
Thanks a lot for your message! Can we still be able to use php build/compile.php or is it the purpose of this move? Leed is not using any dependency to stay as easy as possible to install but I think we can build it and version it on our side. Correct me if I'm wrong.

@jtojnar
Copy link
Contributor Author

jtojnar commented Jan 31, 2023

@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).

@Simounet
Copy link

I understand. Sounds like a plan. I have to test it on my side. Thanks for your proposals!

@ophian
Copy link

ophian commented Feb 1, 2023

Thanks for the note @jtojnar
I just hoped on the single file with 1.7.0, since that better suites my needs in Serendipity Styx. Also because it needs some additions.
I never and really don't want to use composer stuff, since I think that sort of stuff is abstracting careness into believes and so in the end is less secure, less understandable and less maintained.
So Simounet is right in asking if there is a "handmade" way in using it without. 😀

@Art4
Copy link
Contributor

Art4 commented Feb 1, 2023

@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.

  1. As @rmccue stated Mar 11, 2016 in this comment:

The main thing I ask for any changes: Don't break backwards compatibility - BC must be maintained above (almost) everything else. Adding more stuff is fine, refactoring is fine, as is deprecating, but don't remove anything.

  1. Since Dec 24, 2021 we have stated int the CHANGELOG.md that this project follows Semver.

  2. As I stated here I'm working on the refactoring following the Symfony BC promise that have been battle tested over years and been proven to be effective. I'm documenting the refactoring process in Possible roadmap #731.

@jtojnar said:

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 forever until next BC break window.

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 @internal and thus exclude from the BC promise. I definitely prefer implementing our own interfaces/layers over implementing the PSR interfaces. In the words of PSR-18 SimplePie is a "calling library" and therefor "does not implement this specification's interfaces but consumes an object that implements them (a Client)."

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.

@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.

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 build/compile.php.

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.

I would very very strongly advise against defining 3rd-party interfaces in the library, even as wrapping them inside class_exists(). This has been tried already and will lead to catastrophic results.

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?

@jtojnar
Copy link
Contributor Author

jtojnar commented Feb 1, 2023

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.

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.)

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.

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.

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 build/compile.php .

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.

I would very very strongly advise against defining 3rd-party interfaces in the library, even as wrapping them inside class_exists(). This has been tried already and will lead to catastrophic results.

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.

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.

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.

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?

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).

@Art4
Copy link
Contributor

Art4 commented Feb 2, 2023

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

BC promise  consequences for SimplePie 1.x consequences for SimplePie 2.x
Scenario A:
Having no mandatory PSR libs
is part of BC promise
1️🪶 PSR libs are optional,
2️🏗️ interop layers needed for BC,
3️🏗️ deprecation of Cache, File and IRI,
4️✅ keep single-file artifact
1️🪢 PSR libs are mandatory,
2️🔥 remove interop layers,
3️🔥 remove Cache, File and IRI,
4️🔥 drop single-file artifact
Scenario B:
Having no mandatory PSR libs
is not part of BC promise
1️🪢 PSR libs are mandatory,
2️🔥 remove interop layers,
3️🏗️ deprecation of Cache, File and IRI,
4️🔥 drop single-file artifact
1️🪢 PSR libs are mandatory,
3️🔥 remove Cache, File and IRI,
4️🔥 no single-file artifact
Scenario C:
Having no mandatory PSR libs
is not part of BC promise,
but keep single-file artifact
1️🪢 PSR libs are mandatory,
2️🔥 remove interop layers,
3️🏗️ deprecation of Cache, File and IRI,
5️🏗️ create single-file artifact with bundled PSR libs
1️🪢 PSR libs are mandatory,
3️🔥 remove Cache, File and IRI,
5️✅ single-file artifact with bundled PSR libs
Scenario D:
Having mandatory PSR libs
is not allowed at all
1️🪶 PSR libs are optional,
2️🏗️ interop layers needed for BC,
3️❌ deprecation of Cache, File and IRI not allowed, revert deprecation,
4️✅ keep single-file artifact
1️🪶 PSR libs are optional,
2️🔥 remove interop layers,
3️🏗️ rewrite Cache, File and IRI,
4️✅ keep single-file artifact

Scenario A: Having no mandatory PSR libs is part of BC promise

I 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 "psr/simple-cache": "^1 || ^2 || ^3" as a mandatory requirement 1️. I planned to do this for SimplePie v2. Same with PSR-18/PSR-17 HTTP Client in #777. Instead I create interop layers 2️ and deprecate classes and methods 3️. And I have kept the option to create a single-file artifact 4️, because - why not. It's already there. 🙂

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 promise

If 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 artifact

This 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 interface_exists() and/or working with class aliases 5️. (That's why I name this consequence as "5️" instead of "4️") I cannot estimate the effort but I can imagine that the build script might get a bit bigger and we might have to provide more support, which might justify a separate repository.

I personally have no interest in maintaining such a file. 😐

Scenario D: Having mandatory PSR libs is not allowed at all

This scenario is based on this quote of @jtojnar:

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.

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 File and IRI classes with other libraries and getting rid of this classes. It also contradicts deprecating the internal cache classes to be replaced by PSR-16 cache. Following this scenario we would not only create interop layers for Cache and HTTP clients 2️, but also revert the deprecation of (some) Cache solutions 3️. The single-file artifact can be kept as it is 4️.

Because the PSR libs would not be mandatory 1️, we would also not be able to deprecate the File class, but has to refactor it in v2 3️ to a solution, that allow us to make HTTP requests without relying on an external library.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants