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

Remove checks for Symfony's DI Container #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Aug 28, 2021

Q A
Bug fix? no
New feature? no
Doc updated no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets N/A
License MIT

Since Symfony 3.3, the DI component's ContainerInterface has extended the PSR-11 ContainerInterface. So unless Symfony 3.2 and older are still being supported here, there isn't a need to explicitly check for the Symfony container in the lazy driver.

Copy link
Contributor

@W0rma W0rma left a comment

Choose a reason for hiding this comment

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

Since Symfony 3.3, the DI component's ContainerInterface has extended the PSR-11 ContainerInterface. So unless Symfony 3.2 and older are still being supported here, there isn't a need to explicitly check for the Symfony container in the lazy driver.

Do you think we should add a "conflict" for "symfony/dependency-injection": "<3.3" to the composer.json?

@@ -21,12 +20,12 @@ class LazyLoadingDriver implements DriverInterface
private $realDriverId;

/**
* @param ContainerInterface|PsrContainerInterface $container
* @param ContainerInterface $container
*/
public function __construct($container, string $realDriverId)
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we just use a native php type hint here?

Suggested change
public function __construct($container, string $realDriverId)
public function __construct(ContainerInterface $container, string $realDriverId)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically adding a typehint is a B/C break, though the typehint would match up with the runtime check in the constructor (it just changes the error type). If it’s OK with the maintainers, we can add the typehint and remove the inline check.

@mbabker
Copy link
Contributor Author

mbabker commented Oct 6, 2021

Since Symfony 3.3, the DI component's ContainerInterface has extended the PSR-11 ContainerInterface. So unless Symfony 3.2 and older are still being supported here, there isn't a need to explicitly check for the Symfony container in the lazy driver.

Do you think we should add a "conflict" for "symfony/dependency-injection": "<3.3" to the composer.json?

I don't think it needs to go that far. IMO the Symfony container check is a bit of a special case for legacy support (the class originally only supported the Symfony container interface then added PSR-11 support), I think we're far enough past the PSR being accepted and it being widely adopted enough that there doesn't need to be any extra checks or conflicts.

@goetas
Copy link
Collaborator

goetas commented Oct 14, 2021

What is the advantage of this change? is the current code not working?

@mbabker
Copy link
Contributor Author

mbabker commented Oct 14, 2021

It’s a redundant check when using Symfony 3.3 and later as their container interface implements the PSR interface. The current code works, but it’s really only necessary now for legacy Symfony support.

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

Successfully merging this pull request may close these issues.

3 participants