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

Adjusted built-in plugin managers for psr/container:^2 compatibility #59

Merged

Conversation

Ocramius
Copy link
Member

This counts as a minor BC break (which is already inherent of switching to psr/container:^2),
since the return type of ExtensionManagerInterface#has() now requires a bool return type,
and so will further child classes.

To mitigate this BC break, the methods for the two ExtensionManagerInterface types have been
moved to the docblock definitions, reducing the chance of inheritance-based BC breaks.

The two ExtensionManagerInterface have also been deprecated, since they came from a time where
Zend_Loader (in zendframework/zendframework1) was still a thing, whereas laminas/laminas-servicemanager
currently provides healthy abstractions for building objects of a specific category, by using
cleaner dependency injection patterns.

Q A
Documentation no
Bugfix yes
BC Break yes (-ish - better to break inheritance, than break the library completely)
New Feature no
RFC no
QA no

This counts as a minor BC break (which is already inherent of switching to `psr/container:^2`),
since the return type of `ExtensionManagerInterface#has()` now requires a `bool` return type,
and so will further child classes.

To mitigate this BC break, the methods for the two `ExtensionManagerInterface` types have been
moved to the docblock definitions, reducing the chance of inheritance-based BC breaks.

The two `ExtensionManagerInterface` have also been deprecated, since they came from a time where
`Zend_Loader` (in `zendframework/zendframework1`) was still a thing, whereas `laminas/laminas-servicemanager`
currently provides healthy abstractions for building objects of a specific category, by using
cleaner dependency injection patterns.
@froschdesign
Copy link
Member

@Ocramius

The two ExtensionManagerInterface have also been deprecated, since they came from a time where
Zend_Loader (in zendframework/zendframework1) was still a thing, whereas laminas/laminas-servicemanager
currently provides healthy abstractions for building objects of a specific category, by using
cleaner dependency injection patterns.

It was introduced as a standalone implementation: 57f1fc8

Added StandaloneExtensionManager as a drop-in extension plugin resolver
Essentially mimics the AbstractPluginManager, but does so as a
standalone implementation of ExtensionManagerInterface.

And that goes in the direction you have suggested: remove the plugin managers of laminas-servicemanager.
Or have I overlooked something?

@Ocramius
Copy link
Member Author

It was introduced as a standalone implementation: 57f1fc8

Hmm, possibly because Drupal didn't want the laminas/laminas-servicemanager dependency? I'd say that the ship has sailed there, since we added it as a hard dependency anyway 🤔

And that goes in the direction you have suggested: remove the plugin managers of laminas-servicemanager.
Or have I overlooked something?

I'd rather say that this goes the opposite way: if we want to use the plugin managers, let them be proper plugin manager (which @gsteel spent a lot of time refining at type level too)

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

To preserve BC, would it not be better to conflict with psr/container >= 2.0 in a patch release? Otherwise 👍

@Ocramius
Copy link
Member Author

would it not be better to conflict with psr/container >= 2.0 in a patch release?

TBH, very vary of changing dependency ranges in patches, since users will just get a previous release installed (depending on SAT).

It's potentially viable, but I feel like this is the smaller evil, as it breaks only for those that extend our provided plugin managers (really rare)

@Ocramius Ocramius self-assigned this Jul 19, 2022
@Ocramius
Copy link
Member Author

🚢 here to go back to ✅ builds and downstream usage.

@froschdesign happy to iterate on the deprecation in a new patch or minor, if you feel like it isn't a clear one.

@Ocramius Ocramius merged commit f2b852e into laminas:2.18.x Jul 19, 2022
@Ocramius Ocramius deleted the fix/psr-container-v2-compatibility branch July 19, 2022 09:20
@froschdesign
Copy link
Member

@Ocramius
You started the discussion in the chat:

I'm wondering if the concept of "plugin managers" still makes sense at all.
In a world of ContainerInterface#get(class-string<T>): T, do we really still need AbstractPluginManager?
Asking because someone just contacted me for help again due to "where do I configure hydrators?" and "where do I configure X?"

That's why I asked. 😄

@Ocramius
Copy link
Member Author

Ah, that part I agree with, but it's certainly better to have our own general concept of plugin managers, than having each package declare its own approach to plugin managers :D

@froschdesign
Copy link
Member

@Ocramius
Good to know and important for documentation.

(Related to laminas/laminas-view#123)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants