search

php-fig / container

MIT License
9.95k stars 53 forks source link

Replace `container-interop/container-interop` v1.2.0 #38

Closed boesing closed 3 years ago

boesing commented 3 years ago

NOTE

Since there is no 1.2.x branch, this PR targets 1.1.x.

Description

With v1.2.0 container-interop/container-interop which was released in 02/2017, the package was abandoned. It fully complies with psr/container. Replacing it while keeping BC by adding class_alias will help upstream projects to finally get rid of container-interop/container-interop dependencies.

Closes #37

Crell commented 3 years ago

My main concern here is that this implies an added file and aliases for every user of PSR-11, basically forever. On a technical level the aliasing solution is sound, but shouldn't it be in a bridge package (possibly even a later release of container-interop) rather than being in the PSR-11 package permanently?

boesing commented 3 years ago

@Crell Right, this might stay forever in the 1.x version of this component. But there is no need to merge-up this change to v2 which will probably be maintained and used long-term. I am not sure if I get you - what do you mean with "bridge package"? Do you think a dedicated package should be created which provides these changes rather than having it psr/container v1.x?

@mnapoli

whether it will have unwanted side effects

I did some investigations in the PR mentioned in #37 (PR). I've added integration tests which verify that the interop interfaces can be:

(I guess that, if the latter works, property type-hints will work as well?)

So regarding unwanted side-effects, I guess I got these covered (as this package only provides 3 interfaces).

alekitto commented 3 years ago

I’m a little bit concerned about having an additional file to be included and required basically at every request even if not used.

In the specific case every user of this package will pay for this change even if they haven’t had any reference to the old container interop interfaces.

While the solution is technically good (even if class_alias scares me and populates my nightmares because bugs like this), I don’t think this is the right place to add it. A new release of container-interop package could be a better solution, and should probably contain deprecation notices suggesting the implementors to shift to psr/container interfaces.

For these reasons this is a 👎 for me.

Crell commented 3 years ago

Hm, limiting it to the 1.x package helps some, but as of right now that's 99% or more of all users of this package. (cf: https://packagist.org/packages/psr/container/stats). So, it's still basically everyone.

I would rather this bridge file exist either in its own package, or in a final release of the container-interop package.

KorvinSzanto commented 3 years ago

Copying my thoughts from discord:

I'm okay with this but I'm not a fan. I think it sets a really bad example and though it's appropriate and fully compatible in this case, it's likely to cause problems in other situations if anyone were to use this as a guide for eliminating old interface dependencies. Also this is only helpful temporarily so I'd like to see a path for these aliases to get removed

mnapoli commented 3 years ago

I encourage everyone participating to use numbers and concrete use cases/arguments. It's hard to move forward based on feelings and/or abstracts "what if".

I’m a little bit concerned about having an additional file to be included and required basically at every request even if not used.

Why? If this is a performance penalty, can you quantify it? With opcache and possible Composer optimizations, it may not be as obvious as it sounds.

I don’t think this is the right place to add it.

Why?

A new release of container-interop package could be a better solution, and should probably contain deprecation notices suggesting the implementors to shift to psr/container interfaces.

container-interop/container-interop is already deprecated, users should already see notices. I think it's safe to say most implementors know they have to upgrade already.

I would rather this bridge file exist either in its own package, or in a final release of the container-interop package.

But then that defeats the purpose of removing dependencies to container-interop. What would be the advantages of what you suggest over the current situation?

I think it sets a really bad example

Why?

it's likely to cause problems in other situations if anyone were to use this as a guide for eliminating old interface dependencies

Could you illustrate the kind of problems to expect? Do you mean other PSRs could do the same? If that's the case, wouldn't we simply reject any problematic pull request during code review (in other words, why would we assume that merging a pull request automatically means we'd accept similar pull requests). We have no obligations to merge bad things just because something remotely similar was once merged in another package in the past.

Also this is only helpful temporarily so I'd like to see a path for these aliases to get removed

v2, as suggested above?

alekitto commented 3 years ago

Why? If this is a performance penalty, can you quantify it? With opcache and possible Composer optimizations, it may not be as obvious as it sounds.

I'm on holiday now, I cannot quantify anything until I return to work. Anyway this is not a "what if": an include call and three function calls are producing a number of opcodes greater than zero (which is the current situation).
This can be partly mitigated using the preload feature, but is not always available (cli, embed or Apache environments).

I don’t think this is the right place to add it.

Why?

Because I think that ->

A new release of container-interop package could be a better solution, and should probably contain deprecation notices suggesting the implementors to shift to psr/container interfaces.

I can explain this better: in my opinion, importing foreign symbols into a package in order to deprecate another package is not correct in general: is responsibility of the package which is being abandoned/deprecated to provide an upgrade path to a maintained package. For this reason I agree that this will set a bad example if anyone will take this as an example of a correct way to deprecate a package.

container-interop/container-interop is already deprecated, users should already see notices. I think it's safe to say most implementors know they have to upgrade already.

This PR does not contain any deprecation notice. Using this, the user will not see any notice (not at run time nor while installing the package through composer).

This said, a possible solution is to split those three class_alias into three distinct files and declare a new namespace under the psr-4 block of the composer.json file. Those files could then contain a trigger_error signaling the usage of a deprecated class, followed by the class_alias.

For example the Interop\Container namespace could be declared on the compat folder; the file compat/ContainerInterface.php could be something like:

<?php

namespace Interop\Container;

use Psr\Container\ContainerInterface as PsrContainerInterface;

trigger_error('The interface Interop\\Container\\ContainerInterface is deprecated. Please use Psr\\Container\\ContainerInterface instead.', E_USER_DEPRECATED);
class_alias(PsrContainerInterface::class, ContainerInterface::class);

This will register the alias only on usage and also trigger a deprecation notice on first inclusion.

boesing commented 3 years ago

IMHO - Runtime deprecations should be avoided. But if thats how this can make it into this package, I'll take it.

foreign symbols into a package in order to deprecate another package is not correct in General

I guess thats a consensus. However, the container-interop was in fact the blue print for this package, thats why it was that easy to simply extends the PSR Interfaces in its latest version. I don't think that such combination of pre-created drafts will land in real userland Code again as it was done for the container interop by zendframework, pimple and many others. But dependency injection is the key principle of modern projects and thus its widely used.

weierophinney commented 3 years ago

@alekitto and @KorvinSzanto I don't fully understand your concerns, but let me try and outline what I see as the situation at hand.

container-interop existed as the codebase for the PSR-11 WG. One of the requirements for an acceptance vote is that there are multiple concrete implementations of the standard. As such, we had many different projects that adopted container-interop in order to validate the standard.

For these implementations to be tested, they needed releases, and many of them chose to release new versions that then pinned to container-interop.

When PSR-11 was released, several things happened. First, container-interop was updated to (a) include a dependency on the PSR-11 interfaces, (b) have its own interfaces extend the PSR-11 interfaces, and (c) mark itself deprecated. The idea behind this was for it to bridge between the WG and the ratified PSR; generally speaking, you could use PSR-11 in place of container-interop, but you'd get a warning during installation that it is out-of-date. This was essentially the bridge package that Korvin suggested!

However, there are issues with this approach. Code that has a return type of the container-interop ContainerInterface can be updated to return the PSR-11 ContainerInterface safely, as this is a type widening scenario, and the container-interop interface is now a subtype of the PSR-11 interface. However, this is the smallest subset of PSR-11 usage, as it really only applies to functionality that produces the container, which typically only happens exactly once. Code that has parameter type hints against the container-interop ContainerInterface — which is basically every collaborator for a DI container (factories, delegators, etc.) — cannot change their type hints, because doing so would be type widening, which is not allowed for function and method arguments.

As such, if a DI implementation has defined its own interfaces or has implementations that have container-interop type hints for method arguments, they MUST release a new major version in order to guarantee backwards compatibility.

Considering DI is often a foundational layer of a framework, this has huge knock on effects. As an example, for Laminas, we'd have 34 different packages that would then require new major versions in order to update to the a new laminas-servicemanager version that is functionally the same, because they create implementations of interfaces defined in laminas-servicemanager that typehint against container-interop. And that's just our packages; who knows how many third party libraries or userland applications are creating these implementations and would require updates.

In the early days of FIG, we generally developed in the same namespace as the final specification. When we adopted the v3 by-laws and moved to working groups (which has been TREMDENDOUS), we made a recommendation that WGs use a different namespace. The scenario I outline above was a side effect of that that we didn't anticipate. We badly need a process to make it easier for those projects that help in the specification process to adopt the final standard. What is proposed here is one possible solution to that.

Yes, these projects need to update to the official namespaces; I'm not arguing that at all. But I think we also need to recognize that these projects have end users, and that requiring libraries to bump major versions to bump to functionally equivalent code is a huge headache for those end users.

I've seen the following arguments against the proposed solution in this thread:

My larger point is we need a process for migrating from WG to PSR interfaces for those libraries that participated in testing the specification that does not automatically require BC-breaking new major versions. This is one such solution, based on observing the shortcomings of a previous solution. Unless there are demonstrable technical problems with the proposal, I'm not seeing a down side, and see only benefits for the larger ecosystem of users.

alekitto commented 3 years ago

@weierophinney The issue is clear and the solution is technically correct.

As I previously said, I'd prefer not to merge this solution in this package but in a later release of container-interop.

  • "Foreign symbols into a package in order to deprecate another package is not correct in general." My argument to that is that these are not actually foreign symbols; they are the original symbols that got renamed to form the final specification. They are all part and parcel of the same specification, just from different stages of its evolution.

I understand your point of view, but I disagree: IMO those are foreign symbols as they are declared in another package. If we consider this as an exception to this rule we can set a bad example and make the things easier when we would consider another exception. Please don't do this, not in a package that is defining a standard.

  • "An include call and three function calls are producing a number of opcodes greater than zero (which is the current situation)." As others note, we need to take a look at what this means in practice. Looking through a variety of projects, I've found on average 25 file definitions are present, with libraries such as the Symfony polyfills, Diactoros from Laminas, and a few others defining multiple includes, each with multiple function calls. If we put the proposed changes to PSR-11 into that perspective, we're talking a 1-3% increase in the number of files and function calls during autoloading. Considering these are loaded every request, opcaching kicks in in production, meaning these will incur no additional overhead when working with a primed cache.

If someone has time could measure it (unfortunately my computer is at hundreds of kilometres away from me). But still, I know for sure that none of my projects includes container-interop package in any way and I know for sure that I don't want to pay the price of including symbols of a deprecated package which I never used, even if it is small.

  • "This PR does not contain any deprecation notice. Using this, the user will not see any notice (not at run time nor while installing the package through composer)." This is 100% true. The flip side of the scenario: we get requests weekly in Laminas because of the deprecation notice that occurs when Composer installs dependencies. When we let the reporters know that switching to PSR-11 would require a new major version, the reporters generally indicate that they'd rather not break their application. At the same time, they don't want to run deprecated code. So we're in a catch 22 situation.

This could be solved in various ways: for example another version of the container-interop package as previously proposed. Another solution could be that Laminas independently publish a container-interop-compat package which replaces the deprecated one and declares the same three class_aliases.

Polluting this package with symbols inherited from the WG package should be the last resource.
I repeat once again, as a general rule it is responsibility of the deprecated package to provide an upgrade path. And in this specific case, the container-interop package should provide a simple upgrade path to this package as well.

As a last resource, declaring the deprecated namespace to this package and splitting the three class_alias into three independent (and autoloaded) files could be an acceptable compromise. Still not a fan of it, but in the same time no one is forced to pay the price of something that's not used.

weierophinney commented 3 years ago

@alekitto

This could be solved in various ways: for example another version of the container-interop package as previously proposed.

This is a non-starter - it just perpetuates the issues we have currently.

Another solution could be that Laminas independently publish a container-interop-compat package which replaces the deprecated one and declares the same three class_aliases.

This was our original plan. But it doesn't resolve the issue for the larger PSR-11 ecosystem, and, instead, requires each container-interop implementation to perform such mitigation, which can in turn lead to other issues down the line.

As an example, if a user was composing two different PSR-11 implementations in their application, and each implementation defined such aliases, we now have a scenario where there a conflict, depending on the approach:

It can be done if each package also does a test for if (! interface_exists(...)) before doing the aliasing, however, and this might provide a path forward.

Summary of the above:

The main problem with this approach, while workable, is that if another package is loaded ahead of your own, it could define the aliases differently, which could lead to breakage. I don't anticipate that happening when aliasing against FIG interfaces, but it cannot be ruled out. For me, this is yet another reason to create a process for such scenarios in future specifications, whether that's replicating the WG artifacts in the official package, creating a final release of the WG package that does this, or something else. Whatever the solution is, it needs to be something that makes the transition transparent for packages implementing the WG interfaces, and allows them to do minor, versus major, releases to update to the final specification.

alekitto commented 3 years ago

@weierophinney

this is yet another reason to create a process for such scenarios in future specifications

Absolutely agree! This discussion is a result of lack of such a specification. If we write down how the things should be handled transitioning from WG package to PSR package, we resolve discussions like this on package publish. But probably we should discuss it on another channel.

About the situation you described in the previous comment: is this a "what if" or do you have a real use case?

In case what do you think about the third solution?

weierophinney commented 3 years ago

@alekitto —

About the situation you described in the previous comment: is this a "what if" or do you have a real use case?

Real use case. In the mezzio-skeleton package, for instance, we have dev requirements on multiple container packages so we can test and ensure that each works with the skeleton as expected. Additionally, I've used containers that can proxy to other PSR-11 instances (e.g., auto-wiring DI containers that will proxy to non-wiring instances as an optimization).

Which third solution are you referring to?

Your proposal is essentially this one, with a minor strategic difference (only alias something that is consumed), and addresses all the concerns I had (which are preventing conflicts between implementing packages, while simultaneously giving them an automatic upgrade to PSR-11). I prefer it over my own solution, because in my own, I see a potential for issues if implementing packages that adopt the approach do not all alias to the PSR-11 interfaces.

alekitto commented 3 years ago

@weierophinney I was referring to the first one (aliases split on multiple files).

If there's consensus on it, it could be an acceptable solution stating than this package is a direct continuation of the deprecated package. Anyway these aliases should be removed on a v2.

weierophinney commented 3 years ago

@boesing Okay, so I think we have a solution:

On the maintainers end, we'll need to create a 1.2.x branch, and then we would merge this to that branch for a 1.2.0 release, but NOT merge it to the master (2.0.x) tree (which does not define those aliases)

From there, implementations can then choose how/when to update type hints and implementations, but finally have the option to do so in a new minor instead of a new major.

Crell commented 3 years ago

When we adopted the v3 by-laws and moved to working groups (which has been TREMDENDOUS), we made a recommendation that WGs use a different namespace. The scenario I outline above was a side effect of that that we didn't anticipate.

This... isn't true? There is no such recommendation anywhere in the bylaws. PSR-11 was passed under the v2 era rues. (That's why the meta document lists an Editor, Sponsor, and Contributors, not the Working Group. There technically wasn't one, even though in practice that's how it was managed.) Nothing in FIG v3 says to use a separate namespace, and for PSR-14, for instance, we didn't do so, and worked directly in the psr/event-dispatcher namespace from the beginning.

So this is a problem that applies only to 1) Specs that chose to use a separate namespace during development and 2) projects that released real versions of their libraries that depended on that development repository (which is not required by FIGv3 procedures; it says "trial implementation" very specifically to avoid that). That is, in fact, a very unusual situation. If it's not unique to PSR-11, it's at least an unusual one.

That's why I am not supportive of making the fix something within the psr/container package itself. Using class aliases as a way for projects to more easily jump from the -interop package to the psr/container package makes sense. I don't think anyone is disagreeing on that. Just on where that aliasing lives. I agree that there should also be one(1) such alias package in existence, not one per project. I... have no idea where the "3 separate files is better than one" bit came from, though.

What I don't understand is why it must be in this package. What would not-work about releasing a container-interop 1.3 or 2.0 that contained nothing but this same bridge file? Consumers of container-interop could update to that, and get a situation where those two names are aliased to each other. Then they can switch their type definitions without incident. Then at some point in the future change to use the psr/container package. It's the same process as going through a 1.2 of psr/container. What is not workable with that?

weierophinney commented 3 years ago

If it's not unique to PSR-11, it's at least an unusual one.

I've experienced it with PSR-11, PSR-15, PSR-17, and PSR-18. It's not unusual at all at this point.

What I don't understand is why it must be in this package. What would not-work about releasing a container-interop 1.3 or 2.0 that contained nothing but this same bridge file? Consumers of container-interop could update to that, and get a situation where those two names are aliased to each other.

The primary reason is that the container-interop package is already marked deprecated. Its because it is marked deprecated that we see people asking if implementors can remove dependencies on it.

If the WG were to release a new version, then the question is if that version would also be marked deprecated? The scenario wouldn't be as bad as it is today, because it could be used in a transition release, with the following workflow:

This would likely work fine, as the deprecation messages would go away very quickly for a majority of users who keep their applications on up-to-date dependencies. We could even suggest users at least specify a dependency on container-interop 1.3.0 in their applications and update their own code to reference solely PSR-11 as a way to become forwards compatible.

I'd still like to see this documented somewhere in FIG for editors and working groups so they can plan for what happens when transitioning the WG to accepted specification, so we don't have future issues like this.

@mnapoli, would this work for you? If so, I can prepare the patch for container-interop. (I don't have commit rights there, I don't think...)

boesing commented 3 years ago

As realized in container-interop/container-interop#97, a new major version is required to avoid BC breaks where upstream libraries/projects do implement both PSR-11 and container-interop interfaces.

Thanks for your feedback in here, this led us to a better solution by adding changes to container-interop. Merging this will break too much stuff and thus, closing here.