Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[FrameworkBundle][Cache] Do not process cache aliases that are not present in the container#29384

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
allflame wants to merge1 commit intosymfony:4.2fromallflame:4.2

Conversation

@allflame
Copy link
Contributor

QA
Branch?4.2
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
LicenseMIT

1484117

Introduced a major problem when framework-bundle is used WITHOUT doctrine.
On kernel compilation this results in ServiceNotFoundException: You have requested a non-existent service "doctrine.dbal.default_connection".
This is due to the fact there is no check whether such definition is present in the container
Since there is no clear view as how to handle such a situation (remove default value from configuration or ignore those that are not present) I chose the latter.

@allflameallflame changed the titleDo not process cache aliases that are not present in the container[FrameworkBundle][Cache] Do not process cache aliases that are not present in the containerNov 30, 2018
@nicolas-grekasnicolas-grekas added this to the4.2 milestoneNov 30, 2018
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 30, 2018
edited
Loading

I don't think that works: the container inFrameworkExtension is isolated and won't give access to other bundle's services.
If you don't declare a PDO pool, that shouldn't happen. And if you do and it doesn't work, that means you should declare thedefault_pdo_provider yourself - and thus not hit the issue.
As you can see, I don't understand how you hit this :)
Can you please provide a reproducer?

@allflame
Copy link
ContributorAuthor

@nicolas-grekas You defined default value for pdo here

->scalarNode('default_pdo_provider')->defaultValue('doctrine.dbal.default_connection')->end()

and used existing loop that callsCachePoolPass::getServiceProvider(). The problem is that getServiceProvider method detects only (presumably) dsn and define a service for you, but you passed direct alias that was ignored. So guess what happens if you add an alias definition to a non-existent service.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 30, 2018
edited
Loading

That default config leads to the creation an alias namedcache.default_pdo_provider which is used bycache.adapter.pdo. But since this service is abstract, the fact that the service provider is missing shouldn't be an issue. It would become an issue when a concrete pdo-based pool is declared anddoctrine.dbal.default_connection doesn't exist. In this case, you have to define thedefault_pdo_provider configuration option anyway so that you should be able to make things work.

Please provide a reproducer if you still think there is a bug - or at least more details.

@allflame
Copy link
ContributorAuthor

@nicolas-grekas I probably can provide a minimal installation on Monday, but I think I'm either missing a point or can't explain myself well.
I'd rather ask: is it your impression that your commit should work just fine even if there is nodoctrine.dbal.default_connection service in the container?

@nicolas-grekas
Copy link
Member

Everything works when playing with eg the website-skeleton - even without Doctrine. So yes, I don't see the issue right now. Looking forward to your reproducer :)

@allflame
Copy link
ContributorAuthor

allflame commentedNov 30, 2018
edited
Loading

Ok, i think I found the problem.
That's what's get generated

<serviceid="cache.default_pdo_provider"alias="doctrine.dbal.default_connection"public="false"/>

This will obviously crash ifdoctrine.dbal.default_connectionis not in the container. And it's not. But it's working on skeleton. So the reason it's working is because ofRemovePrivateAliasesPass that will remove this alias as an optimization procedure by default.
Personally I don't think the idea to rely on such a behavior should be considered at all.

To be clear: I believe that the suggested fix shouldn't be taken as is since I do not know what the initial intention of the feature was, but creating alias to a non-existent service shouldn't be done either.
How it breaks: in unit test to avoid injecting all the dependencies I mark all the aliases public and fetch them from container. Since the alias is now public it will not be removed from the container and thus will cause a crash.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 30, 2018
edited
Loading

Do you experience the issue or not? If this is purely theoretical, then you should know there are tons of services that rely on this behavior. Or if it's something that can be reproduced, please, really, describe how you get it.

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 30, 2018
edited
Loading

How it breaks: in unit test to avoid injecting all the dependencies I mark all the aliases public and fetch them from container. Since the alias is now public it will not be removed from the container and thus will cause a crash.

OH OK! Then that's the issue. There can be way more things that could break with this strategy.
I'd recommend creating special test aliases if you want to get acces to any services.
Typically inservices_test.yml:

services:test.my_service:alias:my_servicepublic:true

@allflame
Copy link
ContributorAuthor

@nicolas-grekas This works if you need to mock couple of them. If you have dozens this is not that good of approach.
I don't see how this can break something. On top of that in fairly complex application that was using quite a bit of Symfony components the only failing place is this one. And it runs "Make all aliases public" compiler pass, so it definitely should've caught at least something else. Since I'm not that familiar with Symfony I wouldn't go that far as to question

there are tons of services that rely on this behavior

but this behavior causes a lot of side effects as well (#29359).
My impression is that you should be able to drop the private alias without breaking the container, but it's should, not must. I can be wrong though.
I don't like the fact that it couples two independent components together in very indirect way, but it's just my personal opinion

@nicolas-grekas
Copy link
Member

While merging unrelated PRs, one of them made me remind we have a way to deal with such situations.
Here we are, see#29399

@allflame
Copy link
ContributorAuthor

@nicolas-grekas Hey, thanks for the update. I do have a workaround by just setting up default_pdo_driver to null in the config though.

@nicolas-grekas
Copy link
Member

Thanks for reporting and for the discussion.

nicolas-grekas added a commit that referenced this pull requestDec 1, 2018
…only if the package is installed (nicolas-grekas)This PR was merged into the 4.2 branch.Discussion----------[FrameworkBundle] define doctrine as default_pdo_provider only if the package is installed| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#29384| License       | MIT| Doc PR        | -Commits-------cf75012 [FrameworkBundle] define doctrine as default_pdo_provider only if the package is installed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

4.2

Development

Successfully merging this pull request may close these issues.

3 participants

@allflame@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp