NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[FrameworkBundle] Fix cache pool configuration with one adapter and one provider#43164

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

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:4.4fromfancyweb:fwb/fix-cache-adapters-cfg

Dec 11, 2021

Merged

[FrameworkBundle] Fix cache pool configuration with one adapter and one provider#43164

nicolas-grekas merged 1 commit intosymfony:4.4fromfancyweb:fwb/fix-cache-adapters-cfg

Dec 11, 2021

Conversation

Copy link

Contributor

fancyweb commentedSep 24, 2021

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

My current YAML config is:

framework:cache:pools:app.cache.redis:adapter:"cache.adapter.redis"provider:"%env(REDIS_URL)%/4"default_lifetime:86400

I'm trying to migrate it to PHP with the new config builder system. The problem is that->adapter('cache.adapter.redis') does not exist in the generated config class sinceadapter is just a shortcut in the first place. So I have to use->adapters(['cache.adapter.redis']) but the configuration doesn't allow it.

The difference with this patch is that we allow the case whereadapters is a map that contains only one item andprovider is set. The result is exactly the same and it becomes compatible with the config class.

I added an extra check for DX to avoid an\ErrorException ifadapters is not an array.

carsonbot added Status: Needs Review Bug FrameworkBundle labels

Sep 24, 2021

carsonbot added this to the4.4 milestone

Sep 24, 2021

fancyweb force-pushed thefwb/fix-cache-adapters-cfg branch from2261d82 to9673243Compare

September 24, 2021 12:58

Copy link

ContributorAuthor

fancyweb commentedSep 24, 2021

Failures looks related but it's working on my local and I don't see how the changes could have broken them, I'm going to need some help 😕

Copy link

Contributor

ro0NL commentedSep 24, 2021

perhaps more easy to tell config builder about extra methods? maintaining consistency in keys while doing so.

Copy link

Member

nicolas-grekas commentedSep 27, 2021

Should we splitadapter andadapters entries instead? Is that possible?
Then the rule would be more clear: adapters XOR adapter+provider

ro0NL mentioned this pull request

Nov 21, 2021

[Config] Use better typehint in PHP Configuration#44166

Merged

fancyweb force-pushed thefwb/fix-cache-adapters-cfg branch from9673243 to4a77f44Compare

December 7, 2021 16:39

Copy link

ContributorAuthor

fancyweb commentedDec 7, 2021

CI is green, so the failures were not related I guess :-)

@nicolas-grekas Splitting the entries couldn't be considered as a bug fix isn'it? I studied the split a bit and it would be more complicated than the current patch.

Is my proposed change too risky for 4.4? I'd really like to fix this issue to get rid of the last yaml config file in my apps 😅