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.

Sign up for GitHub

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

Jump to bottom

fix container linting for ProfilerController service in some cases #1459

Merged
merged 1 commit into from
Dec 30, 2021
Merged

fix container linting for ProfilerController service in some cases #1459

merged 1 commit into from
Dec 30, 2021

Conversation

dmaicher
Copy link
Contributor

@dmaicher dmaicher commented Dec 29, 2021 •
edited
Loading

Closes #1457

@dmaicher dmaicher mentioned this pull request Dec 29, 2021
Closed
stof
stof requested changes Dec 29, 2021
View reviewed changes
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.

This needs to be done in a compiler pass, not in the DI extension.

DI extensions are working with a partial container. They don't see services defined by other bundles, so there won't ever be a twig service at that point.

@dmaicher
Copy link
Contributor Author

Thanks @stof. Indeed this was not working 🤦‍♂️ Now done within a compiler pass instead.

@dmaicher dmaicher force-pushed the fix-issue-1457 branch 3 times, most recently from e0d6155 to 68f0251 Compare December 29, 2021 21:47
@dmaicher
Copy link
Contributor Author

Interesting test fail with --prefer-lowest on Symfony 4.4.0 🤔 checking

@dmaicher dmaicher force-pushed the fix-issue-1457 branch 2 times, most recently from 57923fd to 25c4cb5 Compare December 29, 2021 21:58
dmaicher
dmaicher commented Dec 29, 2021
View reviewed changes
composer.json Outdated
@@ -34,7 +34,7 @@
"symfony/cache": "^4.3.3|^5.0|^6.0",
"symfony/config": "^4.4.3|^5.0|^6.0",
"symfony/console": "^3.4.30|^4.3.3|^5.0|^6.0",
"symfony/dependency-injection": "^4.3.3|^5.0|^6.0",
"symfony/dependency-injection": "^4.4.18|^5.1.10|^6.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

needs symfony/symfony#39151

@dmaicher dmaicher requested a review from stof December 29, 2021 22:00
@dmaicher dmaicher added this to the 2.5.4 milestone Dec 29, 2021
ostrolucky
ostrolucky reviewed Dec 30, 2021
View reviewed changes
Resources/config/orm.xml
@@ -235,9 +235,9 @@
</service>

<service id="Doctrine\Bundle\DoctrineBundle\Controller\ProfilerController">
<argument type="service" id="twig" on-invalid="ignore"/>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this whole PR, isn't proper fix to use on-invalid="ignore_uninitialized" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This indeed makes the container linter happy 🤔

But technically the service definition is still useless/invalid if twig or profiler are not available? I don't mind 😊 Can also adjust it.

Copy link
Member

Choose a reason for hiding this comment

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

It's symfony's job to remove useless services automatically. If it doesn't do that, that would be an issue for https://github.com/symfony/symfony. symfony/dependency-injection shouldn't shift such responsibility to all the bundles for such use cases instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this was discussed a while back: symfony/symfony#36701

Anyway let's go with the simple solution for now. I will adjust the PR.

@dmaicher dmaicher changed the title remove ProfilerController service in some cases fix container linting for ProfilerController service in some cases Dec 30, 2021
@ostrolucky ostrolucky merged commit f6191c5 into doctrine:2.5.x Dec 30, 2021
@dmaicher dmaicher deleted the fix-issue-1457 branch December 30, 2021 18:47
@github-actions github-actions bot mentioned this pull request Dec 30, 2021
Merged
@stlrnz stlrnz mentioned this pull request Jan 6, 2022
Closed
dmaicher referenced this pull request Jan 6, 2022
@ostrolucky
Remove ProfilerController forcibly when requirements are not met
7f6209e 
Looks like ignore_unitialized is still not enough, we do really need to remove it manually...

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

@ostrolucky ostrolucky ostrolucky approved these changes

@stof stof Awaiting requested review from stof

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.5.4
Development

Successfully merging this pull request may close these issues.

Invalid definition for service "Doctrine\Bundle\DoctrineBundle\Controller\ProfilerController"
3 participants
@dmaicher @stof @ostrolucky