Skip to content
This repository has been archived by the owner on Dec 7, 2019. It is now read-only.

replace container-interop/container-interop with psr/container #40

Merged
merged 14 commits into from
Nov 27, 2017
Merged

replace container-interop/container-interop with psr/container #40

merged 14 commits into from
Nov 27, 2017

Conversation

1ma
Copy link
Contributor

@1ma 1ma commented Nov 21, 2017

Abstract

container-interop/container-interop is deprecated since Feb. 13th 2017 in favor of psr/container (PSR-11 spec). This PR replaces one for the other.

My use case

I use Pimple as the DI container to lazy load command handlers. Pimple provides it's own PSR-11 decorator, but it implements PSR-11's interface, not container-interop's, hence it cannot be fed to the Dispatcher class.

Promotion of psr/container to regular dependency

The Dispatcher class -which lives under src- depends on it, so I believe it should not be disguised as a development/suggested depedency (if it's not installed could break production code).

Potential BC breaks?

Since release 1.2.0 of container-interop Interop\Container\ContainerInterface extends Psr\Container\ContainerInterface, so I understand that there should be none: interop containers are also psr-11 containers (but the inverse does not hold).

About the removal of composer.lock

When installing a dependency with composer the composer.lock file is ignored, and most PHP libraries keep it out of the repository because it is just dead weight.

.gitignore Outdated
@@ -1 +1,2 @@
composer.lock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should not be ignored, we'd like to run test with locked dependencies (see Travis CI configuration in .travis.yml)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it!

@1ma
Copy link
Contributor Author

1ma commented Nov 21, 2017

I'm quite puzzled about the failed tests, nor I'm able to reproduce these failures myself. Any help would be appreciated.

@michalbundyra
Copy link
Member

@1ma It's more complicated to fix these tests here (will be out of the scope). See my PR #41
I think when #41 will be merged then your PR should work fine

@1ma
Copy link
Contributor Author

1ma commented Nov 22, 2017

Great! Once your PR is merged into develop I'll rebase mine and see what happens.

@michalbundyra
Copy link
Member

@1ma meanwhile you can try on local to make sure all is good 😄

@1ma
Copy link
Contributor Author

1ma commented Nov 24, 2017

Myea, success.

@weierophinney weierophinney merged commit 2d1b98d into zfcampus:develop Nov 27, 2017
weierophinney added a commit that referenced this pull request Nov 27, 2017
replace container-interop/container-interop with psr/container
weierophinney added a commit that referenced this pull request Nov 27, 2017
weierophinney added a commit that referenced this pull request Nov 27, 2017
@weierophinney
Copy link
Member

Thanks, @1ma

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

Successfully merging this pull request may close these issues.

3 participants