Replace container-interop with PSR-11 container interface #187

michaelmoussa · 2017-03-28T05:10:05Z

This PR replaces use of the container-interop package with the PSR-11 container interface. This is obviously a BC break and would need to be released in ServiceManager 4.0 along with a migration guide.

Since zend-eventmanager comes into play when ProxyManager is used for lazy instantiation of services (via its own dependency on zend-code, which in turn depends on zend-eventmanager), I'll be submitting an equivalent PR on that repository shortly and will link it here when it's up.

weierophinney

This looks great, and I'm ready to merge it to develop for release in 4.0.0. I need a couple of things, though:

A rebase against current develop, as we've pulled in a ton of changes recently.
A migration document, detailing how developers will need to update their code that implements any of the various factory interfaces to work with the new version. This essentially becomes "find 'Interop\Container' and replace with 'Psr\Container' in your codebase".

Once those are in place, I'll merge to develop.

Thanks!

michaelmoussa · 2017-12-02T23:15:08Z

Thanks @weierophinney - will do. Do you happen to have a link to a sample migration document for any similar change made in the past that I might be able to use as a template?

weierophinney · 2017-12-03T12:40:34Z

@michaelmoussa Yes: https://docs.zendframework.com/zend-servicemanager/migration/

michaelmoussa · 2017-12-05T21:20:44Z

@weierophinney Rebase is complete. All the remains is the migration guide. Do you prefer a short section for v4 changes in the existing migration guide, or do you want to keep each major version's migration guide separate?

weierophinney · 2017-12-05T22:53:31Z

All the remains is the migration guide. Do you prefer a short section for v4 changes in the existing migration guide, or do you want to keep each major version's migration guide separate?

Do it as a separate document. I will likely be creating a sub-tree for the v3 docs once we're ready to move to v4, so having them separate will make that easier (no need to split out the information to a new file).

michaelmoussa · 2017-12-06T15:02:08Z

@weierophinney Hmm, separate document but not new file..? Do you mean clear out the existing migration.md file and start putting v4 stuff in it?

weierophinney · 2017-12-06T15:47:17Z

New file. Call it something like migration-v4.md or similar.

michaelmoussa · 2017-12-06T16:49:44Z

@weierophinney Initial draft added. I didn't add it to the mkdocs.yml, so up to you how you want to link it up.

I have a similar PR open for zendframework/zend-eventmanager. It's been a while since I opened these, so I'm not sure if the EventManager one still needs to be merged and released prior to the ServiceManager one. In any event (heh), I'll be updating the EventManager one as well sometime this week.

froschdesign · 2017-12-06T16:56:48Z

docs/book/migration-v4.md

+      ` as ContainerException` in the find-and-replace, and any existing use of `ContainerException`
+      will continue to work.
+
+**NOTE:** If you use fully-qualified class names in your typehints, rather than


For this type of note we should use a blockquote:

> ### Note > > If you use fully-qualified class names in your typehints, rather than > …

froschdesign · 2017-12-07T10:31:34Z

docs/book/migration-v4.md

+      ` as ContainerException` in the find-and-replace, and any existing use of `ContainerException`
+      will continue to work.
+
+### Note


A blockquote needs > in front of every line.

Example "Tip: Serve via Composer": https://docs.zendframework.com/zend-expressive/getting-started/standalone/#5-start-a-web-server

@froschdesign fixed - thanks! Sorry I missed that.

@michaelmoussa

Sorry I missed that.

No problem. Thanks! 👍

Ocramius · 2018-01-09T07:37:25Z

Overall, this looks good, but develop is currently targeting 3.4.0, so we need to wait for that release before moving on with this patch.

michalbundyra · 2018-02-10T19:27:34Z

@Ocramius as I can see develop is now targeting 4.0-dev, is there any chance to merge this PR and than we can move forward with this package? Thanks !

Ocramius · 2018-02-10T19:30:09Z

Could we rebase to see if all is good?

michalbundyra · 2018-02-10T19:31:55Z

@michaelmoussa could you rebase it please? If you don't have time to do that, I can do it if you give me permission to push into your branch 😄

michalbundyra · 2018-02-10T19:29:27Z

docs/book/migration-v4.md

+
+[`container-interop/container-interop`](https://github.com/container-interop/container-interop)
+was officially deprecated in favor of [PSR-11](https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-11-container.md)
+on February 13, 2017. As such, all uses of the `Interop\Container\*` interfaces


There was more than one interface? Why there is *?

michalbundyra · 2018-02-10T19:34:34Z

docs/book/migration-v4.md

+> advantage of `use` statements, you will need to run additional find-and-replace
+> commands to update those class paths within your code. The exact finds and
+> replaces to run for those scenarios are not covered in this migration guide, as
+> they can vary greatly and are not a generally recommended practice.


The migration process is very basic, but I think it would be nice to have migration tool. I think it could be done later on, so I'd like to leave just a note for that. Unless you have another idea?

I think delivering migration tools will help to migrate quicker to newer version.

michalbundyra · 2018-02-10T19:36:43Z

src/Exception/ServiceNotCreatedException.php

-class ServiceNotCreatedException extends SplRuntimeException implements
-    ContainerException,
-    ExceptionInterface
+class ServiceNotCreatedException extends SplRuntimeException implements ContainerExceptionInterface


I think we should implement here just ExceptionInterface instead.

michalbundyra · 2018-02-10T19:37:03Z

src/Exception/ServiceNotFoundException.php

@@ -7,15 +7,13 @@

 namespace Zend\ServiceManager\Exception;

-use Interop\Container\Exception\NotFoundException;
+use Psr\Container\ContainerExceptionInterface;


Alphabetical order please.

michalbundyra · 2018-02-10T19:37:22Z

src/Exception/ServiceNotFoundException.php

-class ServiceNotFoundException extends SplInvalidArgumentException implements
-    ExceptionInterface,
-    NotFoundException
+class ServiceNotFoundException extends SplInvalidArgumentException implements ContainerExceptionInterface


The same here - I think we should implement just ExceptionInterface ... hm...and maybe NotFoundException? hm... but definitely ExceptionInterface instead of ContainerExceptionInterface.

michalbundyra · 2018-02-10T19:38:48Z

src/ServiceManager.php

-use Interop\Container\ContainerInterface;
-use Interop\Container\Exception\ContainerException;
+use Psr\Container\ContainerExceptionInterface;
+use Psr\Container\ContainerInterface;


Alphabetical order please.

michalbundyra · 2018-02-10T19:40:27Z

src/ServiceManager.php

@@ -769,7 +769,7 @@ private function createDelegatorFromName($name, array $options = null)
     * @throws ServiceNotFoundException if unable to resolve the service.
     * @throws ServiceNotCreatedException if an exception is raised when
     *     creating a service.
-     * @throws ContainerException if any other error occurs
+     * @throws ContainerExceptionInterface|Exception if any other error occurs


Exception is very generic, I can't see where it is thrown.
And I think we should have separate @throws definition per exception.

michalbundyra · 2018-02-10T19:40:43Z

src/ServiceManager.php

@@ -781,7 +781,7 @@ private function doCreate($resolvedName, array $options = null)
            } else {
                $object = $this->createDelegatorFromName($resolvedName, $options);
            }
-        } catch (ContainerException $exception) {
+        } catch (ContainerExceptionInterface $exception) {


Hm... Can we use here ExceptionInterface maybe?

michaelmoussa · 2018-02-10T20:21:42Z

@webimpress I gave you push access on my fork. I won't have time to go through a rebase until sometime next week unfortunately.

Removed the PSR-11 book page since it no longer applies, but I imagine there will be some migration docs to take their place before this is would be released.

michalbundyra · 2018-02-11T01:52:31Z

@michaelmoussa Thanks, I've rebased and pushed some changes.

@Ocramius Rebased, seems to be fine. Any chance to get it merged? 😄 Thanks!

Ocramius · 2018-02-11T15:11:32Z

@michaelmoussa @webimpress thanks 👍

samsonasik · 2018-02-13T16:04:45Z

src/AbstractFactoryInterface.php

@@ -16,11 +16,11 @@
 *
 * - rename the method `canCreateServiceWithName()` to `canCreate()`, and:
 *   - rename the `$serviceLocator` argument to `$container`, and change the
- *     typehint to `Interop\Container\ContainerInterface`
+ *     typehint to `Psr\Container\ContainerInterface`


this was correct as it was previously noted for version 2 to version 3 migration

samsonasik · 2018-02-13T16:05:09Z

src/AbstractFactoryInterface.php

 *   - merge the `$name` and `$requestedName` arguments
 * - rename the method `createServiceWithName()` to `__invoke()`, and:
 *   - rename the `$serviceLocator` argument to `$container`, and change the
- *     typehint to `Interop\Container\ContainerInterface`
+ *     typehint to `Psr\Container\ContainerInterface`


this was correct as it was previously noted for version 2 to version 3 migration

This was referenced Mar 28, 2017

Replace container-interop with PSR-11 container interface zendframework/zend-eventmanager#52

Open

Use PSR-11 Container Interface zendframework/zend-expressive-zendviewrenderer#37

Merged

asgrim requested a review from weierophinney June 29, 2017 18:35

Xerkus added the BC Break label Nov 17, 2017

weierophinney suggested changes Nov 27, 2017

View reviewed changes

froschdesign reviewed Dec 6, 2017

View reviewed changes

froschdesign reviewed Dec 7, 2017

View reviewed changes

Ocramius added this to the 4.0.0 milestone Jan 9, 2018

Ocramius added the enhancement label Jan 9, 2018

weierophinney mentioned this pull request Feb 1, 2018

update to psr/container and remove interop/container #245

Closed

michalbundyra reviewed Feb 10, 2018

View reviewed changes

michaelmoussa and others added 7 commits February 11, 2018 01:13

Replace container-interop with PSR-11

779d532

Update docs for container-interop -> PSR-11 change

3adaf58

Removed the PSR-11 book page since it no longer applies, but I imagine there will be some migration docs to take their place before this is would be released.

Add v4 migration guide for PSR-11

e4b93d3

Updated test assets factories to PSR-11 container

946ff63

Fixed order of use statements

37d67f6

Updated exception - all library exceptions extend ExceptionInterface

f4942fd

ServiceNotFoundException implements PSR-11 NotFoundExceptionInterface

2eff858

Updated migration guide

c800b99

Ocramius self-assigned this Feb 11, 2018

Ocramius approved these changes Feb 11, 2018

View reviewed changes

Ocramius merged commit 9a6b68f into zendframework:develop Feb 11, 2018

michalbundyra deleted the psr-11 branch February 12, 2018 07:35

samsonasik reviewed Feb 13, 2018

View reviewed changes

albaquarto mentioned this pull request Dec 9, 2019

container-interop abandoned warning #286

Closed

weierophinney mentioned this pull request Dec 31, 2019

Replace container-interop with PSR-11 container interface laminas/laminas-eventmanager#5

Closed

Ocramius mentioned this pull request Dec 1, 2020

ContainerInterface reference was changed in a minor release laminas/laminas-servicemanager#59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace container-interop with PSR-11 container interface #187

Replace container-interop with PSR-11 container interface #187

michaelmoussa commented Mar 28, 2017

weierophinney left a comment

michaelmoussa commented Dec 2, 2017

weierophinney commented Dec 3, 2017

michaelmoussa commented Dec 5, 2017

weierophinney commented Dec 5, 2017

michaelmoussa commented Dec 6, 2017

weierophinney commented Dec 6, 2017

michaelmoussa commented Dec 6, 2017

froschdesign Dec 6, 2017 •

edited

Loading

froschdesign Dec 7, 2017

michaelmoussa Dec 7, 2017

froschdesign Dec 7, 2017

Ocramius commented Jan 9, 2018

michalbundyra commented Feb 10, 2018

Ocramius commented Feb 10, 2018

michalbundyra commented Feb 10, 2018

michalbundyra Feb 10, 2018

michalbundyra Feb 10, 2018

michalbundyra Feb 10, 2018

michalbundyra Feb 10, 2018

michalbundyra Feb 10, 2018

michalbundyra Feb 10, 2018

michalbundyra Feb 10, 2018

michalbundyra Feb 10, 2018

michaelmoussa commented Feb 10, 2018

michalbundyra commented Feb 11, 2018

Ocramius commented Feb 11, 2018

samsonasik Feb 13, 2018

samsonasik Feb 13, 2018

Replace container-interop with PSR-11 container interface #187

Replace container-interop with PSR-11 container interface #187

Conversation

michaelmoussa commented Mar 28, 2017

weierophinney left a comment

Choose a reason for hiding this comment

michaelmoussa commented Dec 2, 2017

weierophinney commented Dec 3, 2017

michaelmoussa commented Dec 5, 2017

weierophinney commented Dec 5, 2017

michaelmoussa commented Dec 6, 2017

weierophinney commented Dec 6, 2017

michaelmoussa commented Dec 6, 2017

froschdesign Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ocramius commented Jan 9, 2018

michalbundyra commented Feb 10, 2018

Ocramius commented Feb 10, 2018

michalbundyra commented Feb 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelmoussa commented Feb 10, 2018

michalbundyra commented Feb 11, 2018

Ocramius commented Feb 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

froschdesign Dec 6, 2017 •

edited

Loading