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

5.next: port over thephpleague/container #18090

Open
wants to merge 5 commits into
base: 5.next
Choose a base branch
from

Conversation

LordSimal
Copy link
Contributor

Due to our code style some BC changes had to be done (mostly return types for interface methods) to this lib to get it all green, therefore it is NOT a 1:1 replacement of thephpleague/container

I added it to 5.next and not 6.x since it can be seen as an opt-in feature to use the built-in container instead of the already existing container implementation. If not suited for a minor update this can still be easily rebased onto 6.x

It of course needs to be connected to the rest of the framework (e.g. the Testsuite).

@LordSimal LordSimal added this to the 5.2.0 milestone Dec 16, 2024
use Cake\Container\ServiceProvider\ServiceProviderInterface;
use Psr\Container\ContainerInterface;

class Container implements DefinitionContainerInterface
Copy link
Member

Choose a reason for hiding this comment

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

Would we switch the base class of Cake\Core\Container in a future 5.x release or leave that to 6.x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be a 6.x only move since major changes have already happened here + more major changes are going to happen according to ADmad's christmas wishlist bellow.

@ADmad
Copy link
Member

ADmad commented Dec 19, 2024

If we are internalizing the league/container package, then I would suggest making some additional changes to it too:

  • Make reflection a default feature instead of requiring an additional delegate container. Apart from maybe very small projects I doubt anyone would want to declare all definitions manually.
  • Provide an API to add multiple definitions. Currently the only way to add multiple definitions together is to create a DefinitionAggregate instance and pass it as constructor argument. It would be a lot more convenient if there was for e.g. a Container::addDefinitions(array $definitions) method. Then app/plugins could even have config files of their own which return the required array.
  • Update Definition::addArgument() to have and additional parameter for argument name. Currently if I have a service who's contructor has it's 1st argument as an object and 2nd argument as a scalar, there's no way to set a value for just the 2nd argument and let the 1st argument be handled by reflection.

These are few things that bugged me when using the container.

@LordSimal
Copy link
Contributor Author

Make reflection a default feature instead of requiring an additional delegate container. Apart from maybe very small projects I doubt anyone would want to declare all definitions manually.

Currently I just added the ReflectionContainer by default as you can see here
So you would prefer to remove this kind of ReflectionContainer abstraction as a whole?

I always thought there were reasons why one want's to manually add services to a container instead of relying on Reflection.

@ADmad
Copy link
Member

ADmad commented Dec 19, 2024

Disabling reflection/autowiring should be an option but available by default IMO.

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

Successfully merging this pull request may close these issues.

4 participants