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

Replace abandoned `container-interop/container-interop` package with `psr/container` #2885

Merged
merged 2 commits into from Nov 27, 2019

Conversation

@Ayesh
Copy link

Ayesh commented Nov 26, 2019

PSR-11 container-interop/container-interop package is replaced by psr/container, but Slim v3 branch still uses it.

There were just a few uses of interfaces internally used by Slim, and this PR replaces them with the psr/container counterparts.

Quoting from container-interop/container-interop project page:

Deprecation warning!

Starting Feb. 13th 2017, container-interop is officially deprecated in favor of PSR-11.
Container-interop has been the test-bed of PSR-11. From v1.2, container-interop directly extends PSR-11 interfaces.
Therefore, all containers implementing container-interop are now de-facto compatible with PSR-11.

  • Projects implementing container-interop interfaces are encouraged to directly implement PSR-11 interfaces instead.
  • Projects consuming container-interop interfaces are very strongly encouraged to directly type-hint on PSR-11 interfaces, in order to be compatible with PSR-11 containers that are not compatible with container-interop.

Regarding the delegate lookup feature, that is present in container-interop and not in PSR-11, the feature is actually a design pattern. It is therefore not deprecated. Documentation regarding this design pattern will be migrated from this repository into a separate website in the future.

I do not think this will be a BC break (in semantic versioning sense) because the container-interop package implements psr/container internally.

Travis CI tests: https://travis-ci.org/Ayesh/Slim/builds/617400679

Thank you.

…r/container
@coveralls

This comment has been minimized.

Copy link

coveralls commented Nov 26, 2019

Coverage Status

Coverage decreased (-0.001%) to 98.052% when pulling 1a67c19 on Ayesh:3.x into 925445e on slimphp:3.x.

use Interop\Container\ContainerInterface;
use Interop\Container\Exception\ContainerException;
use Psr\Container\ContainerInterface;
use Psr\Container\ContainerExceptionInterface as ContainerException;

This comment has been minimized.

Copy link
@jacekkarczmarczyk

jacekkarczmarczyk Nov 27, 2019

What is the reason for aliasing? I think the word Interface should still be there

This comment has been minimized.

Copy link
@Ayesh

Ayesh Nov 27, 2019

Author

Thanks for your review.

In psr/container, the exception interface is named ContainerExceptionInterface. There are some code in the container class that refers to the container-interop pacakge as ContainerException. With the aliasing, we were skipping changes in a few other places in the file.

I have pushed another commit with changes in the aliases. We not use the psr/container-provided Exception interface directly.

…nterface directly as opposed aliases
@l0gicgate l0gicgate added the Slim 3 label Nov 27, 2019
@l0gicgate l0gicgate added this to the 3.12.3 milestone Nov 27, 2019
@l0gicgate

This comment has been minimized.

Copy link
Contributor

l0gicgate commented Nov 27, 2019

I think this is fine. @akrabat @adriansuter I don't foresee a BC break from this am I blind? 😂

@akrabat

This comment has been minimized.

Copy link
Member

akrabat commented Nov 27, 2019

LGTM. I'm in favour anyway! #YOLO 😂

@l0gicgate l0gicgate merged commit b38f3ec into slimphp:3.x Nov 27, 2019
2 of 3 checks passed
2 of 3 checks passed
coverage/coveralls Coverage decreased (-0.001%) to 98.052%
Details
Scrutinizer No new issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Ayesh

This comment has been minimized.

Copy link
Author

Ayesh commented Nov 27, 2019

Awesome thanks a lot @jacekkarczmarczyk , @akrabat and @l0gicgate!

@lcharette

This comment has been minimized.

Copy link

lcharette commented Nov 29, 2019

Note this introduced a breaking change in our package (UserFrosting). Since we depend on Slim V3, we referenced container-interop/container-interop in our code.

We've already planned to switch to PSR for out next minor version, but in the meantime, we have to lock Slim version so it doesn't impact our users.

@Ayesh

This comment has been minimized.

Copy link
Author

Ayesh commented Nov 29, 2019

@lcharette would it work if you directly require container-interop/container-interop package in your composer.json? If you were running slim v3.12, you are already using psr/container because container-interop requires it.

@lcharette

This comment has been minimized.

Copy link

lcharette commented Nov 29, 2019

@Ayesh Yes and no. Works with Composer. But no, because Slim\Container still doesn't implement the right instance anymore (Our class expected Slim\Container to be an instance of container-interop/container-interop).

In any case, updating our composer.json can be considered a Breaking Change IMO.

@akrabat

This comment has been minimized.

Copy link
Member

akrabat commented Nov 29, 2019

Probably should have considered that it would be type hinted against. I saw it as an implementation detail.

@Harumaro

This comment has been minimized.

Copy link

Harumaro commented Nov 29, 2019

We had an issue following the new minor version update from 3.12.2 to 3.12.3 and had to change our namespace imports for ContainerInterface from Interop to Psr. This definitely broke our BC and I wouldn't have expected this kind of change on a minor tag 😟... just a heads up.

@l0gicgate

This comment has been minimized.

Copy link
Contributor

l0gicgate commented Nov 29, 2019

I had a feeling this was going to happen, unfortunately this sounds like a downstream issue that's not under the scope of this package. Also, having abandoned packages are a security issue. You can always lock the Slim version to 3.12.2, this branch is in maintenance mode only anyway

@Ayesh

This comment has been minimized.

Copy link
Author

Ayesh commented Nov 29, 2019

@Harumaro I do apologize for the BC breaks (I opened the PR 😊).

Similar to what Rob and Pierre said, I was under the assumption that the use of container-interop package was contained within Slim, and downstream packages should not be using it in favor of the psr/container package.

@lcharette

This comment has been minimized.

Copy link

lcharette commented Nov 29, 2019

It should be noted that while Interop was deprecated since Feb. 13th 2017, the "abandoned" status on Packagist/Composer is fairly recent, which didn't help.

@SvenRtbg

This comment has been minimized.

Copy link

SvenRtbg commented Dec 2, 2019

Happy to see this getting released. Yes, it is a BC break technically, but one that can get resolved very quickly because the container-interop interface is exactly the same as the psr/container interface, except for the name.

And what could be done about it? Version 4 has already been released, so that's not an option. Minor version bump? Same BC issue. Limiting the maximum version if you have issues, or adapt and rename your interface consumption in the code.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.