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

Check if a service was removed from the container at compile time #312

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
4 participants
@aschempp
Copy link
Contributor

aschempp commented Feb 1, 2019

I realized there's actually a way to check if a service was removed at compile time, so we can check for that instead of assuming we need to create an instance if there are no constructor arguments.

Also updated the error message to match Symfony.

@Toflar

Toflar approved these changes Feb 1, 2019

Copy link
Member

Toflar left a comment

Neat!

@@ -168,24 +169,17 @@ protected function import($strClass, $strKey=null, $blnForce=false)
{
$this->arrObjects[$strKey] = $container->get($strClass);
}
elseif (strpos($strClass, '.') !== false)
elseif ($container instanceof Container && isset($container->getRemovedIds()[$strClass]))

This comment has been minimized.

@leofeyer

leofeyer Feb 1, 2019

Member

I don't think the instanceof Container check is necessary. We are not checking this above and below, either.

This comment has been minimized.

@aschempp

aschempp Feb 1, 2019

Author Contributor

getRemovedIds is not part of the interface, it has only been in the class since Symfony 3.4

This comment has been minimized.

@leofeyer

leofeyer Feb 1, 2019

Member

Shouldn't we check \in_array('getRemovedIds', get_class_methods($container), true) then?

This comment has been minimized.

@aschempp

aschempp Feb 2, 2019

Author Contributor

No, we require 3.4 minimum.

This comment has been minimized.

@leofeyer

leofeyer Feb 2, 2019

Member

What has that to do with it? We are checking for method existence in other places, too, e.g. here: https://github.com/contao/contao/blob/4.4/core-bundle/src/Doctrine/Schema/DcaSchemaProvider.php#L317

This comment has been minimized.

@ausi

ausi Feb 2, 2019

Member

IMO the instanceof check is correct here. The existence of getRemovedIds() doesn’t guarantee that it works as we expect it, but the BC promise of Symfony should guarantee that the Container class works as expected.

This comment has been minimized.

@aschempp

aschempp Feb 4, 2019

Author Contributor

We're checking for the method existence because there can be differences in our dependencies. Not so in the case, the method must always be there if that class is implemented (which we actually know it is, but the interface does not guarantee that).

@leofeyer leofeyer added the defect label Feb 1, 2019

@leofeyer leofeyer added this to the 4.7.0 milestone Feb 1, 2019

@leofeyer leofeyer merged commit 4983343 into contao:4.7 Feb 4, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
coverage/coveralls Coverage remained the same at 88.499%
Details
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Feb 4, 2019

Thank you @aschempp.

@aschempp aschempp deleted the aschempp:bugfix/removed-service-ids branch Feb 6, 2019

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