Skip to content

[FrameworkBundle] fix wiring of annotations.cached_reader #46427

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

Merged
merged 1 commit into from
May 23, 2022

Conversation

nicolas-grekas
Copy link
Member

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #46202
License MIT
Doc PR -

Also fixes #26088 (comment)

Instead of trying to hack the compilation of the container to prevent it from inlining annotations.cached_reader, this PR relies on an container.do_not_inline tag that just does that, prevent inlining so that removing passes can reliably fetch the corresponding services from the container by id or by tag.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Nice.

@chalasr
Copy link
Member

chalasr commented May 21, 2022

(would be even better with a test case if possible)

Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

Would be great to add a test fetching the annotation reader public service from the container as a functional test

@@ -40,6 +40,10 @@ public function process(ContainerBuilder $container)
}
$decoratingDefinitions = [];

$tagsToKeep = $container->hasParameter('container.behavior_describing_tags')
? $container->getParameter('container.behavior_describing_tags')
: ['container.do_not_inline', 'container.service_locator', 'container.service_subscriber'];
Copy link
Member

Choose a reason for hiding this comment

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

should those be merged with the parameter instead ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so: giving full control to the parameter is more flexible to me.

Copy link
Member

Choose a reason for hiding this comment

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

but does it really make sense to disable core ones ? It would mean that if a project customizes the list, they would not benefit from new additions done in the core like this bugfix until they discover this internal change to the default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

This tag is already used in ResolveInstanceofConditionalsPass where is has to contain container.service_locator and other core tags. This code doesn't introduce anything new to me, convention-wise (since this parameter works by convention).

Copy link
Member

Choose a reason for hiding this comment

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

ok

@nicolas-grekas
Copy link
Member Author

Test added.

Status: needs review

@nicolas-grekas nicolas-grekas merged commit 11a87ad into symfony:4.4 May 23, 2022
@nicolas-grekas nicolas-grekas deleted the annotread branch May 23, 2022 12:39
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