Skip to content
This repository was archived by the owner on Jan 21, 2020. It is now read-only.

Use PSR-11 Container Interface #37

Merged
merged 1 commit into from
Dec 12, 2017
Merged

Use PSR-11 Container Interface #37

merged 1 commit into from
Dec 12, 2017

Conversation

geerteltink
Copy link
Member

container-interop can't be removed completely... ZendViewRendererFactoryTest has tests for Zend\ServiceManager\AbstractPluginManager, which is not updated yet.

@michaelmoussa
Copy link

@xtreamwayz this looks good (so far) but I suggest waiting on merging this until container-interop can be removed completely. The partial changes are already a BC break, and removing container-interop entirely later would be another BC break. Might as well do it all in one shot.

There are quite a number of references to it in zendframework/zend-servicemanager which likely tie in to other ZF components, but as long as it gets a major version bump, we shouldn't need to update the other components right away.

@michaelmoussa
Copy link

I went ahead and submitted a PR to both zendframework/zend-servicemanager and zendframework/zend-eventmanager for interop -> PSR-11. Maybe we can end up removing interop from this PR entirely like you were able to do for the other two renderer repositories.

Copy link
Member

@michalbundyra michalbundyra left a comment

Choose a reason for hiding this comment

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

I think it should be marked as BC Break, because we change type of exception. Maybe type hint in factories is not a big issue, don't think so that somebody extends these factories and override __invoke method. But anyway... it's still possible.

@@ -19,7 +19,8 @@
},
"require": {
"php": "^5.6 || ^7.0",
"container-interop/container-interop": "^1.2",
"container-interop/container-interop": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

We don't need above line. We are not using this library here anymore.
We need of course conflict entry, as it is now below.


class MissingHelperException extends DomainException implements
ContainerException,
ContainerExceptionInterface,
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it here BC Break? If somebody catch exception on ContainerException it will stop work.

Copy link
Member

Choose a reason for hiding this comment

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

If they were catching Interop\Container\Exception\ContainerException, however, and the container in use implements PSR-11 only, then there's breakage.

@mwillbanks
Copy link
Contributor

At this point this is starting to look really good. I do think we need to support PSR-11 properly, there are some other comments that might be necessary to look at prior. My approval is strictly on the code and the implements.

@weierophinney weierophinney added this to the 2.0.0alpha1 milestone Dec 12, 2017
@weierophinney weierophinney changed the base branch from develop to release-2.0.0 December 12, 2017 18:28
@weierophinney weierophinney merged commit e0776f5 into zendframework:release-2.0.0 Dec 12, 2017
weierophinney added a commit that referenced this pull request Dec 12, 2017
Use PSR-11 Container Interface

Conflicts:
	composer.lock
@weierophinney
Copy link
Member

Thanks, @xtreamwayz!

@geerteltink geerteltink deleted the feature/psr-11 branch December 12, 2017 19:38
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants