Skip to content

[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.

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
Merged
merged 1 commit into from
Dec 11, 2021
Merged

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

merged 1 commit into from
Dec 11, 2021

Conversation

fancyweb
Copy link
Contributor

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 since adapter 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 where adapters is a map that contains only one item and provider 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 if adapters is not an array.

@carsonbot carsonbot added this to the 4.4 milestone Sep 24, 2021
@fancyweb fancyweb force-pushed the fwb/fix-cache-adapters-cfg branch from 2261d82 to 9673243 Compare September 24, 2021 12:58
@fancyweb
Copy link
Contributor Author

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 😕

@ro0NL
Copy link
Contributor

ro0NL commented Sep 24, 2021

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

@nicolas-grekas
Copy link
Member

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

@ro0NL ro0NL mentioned this pull request Nov 21, 2021
Merged
@fancyweb fancyweb force-pushed the fwb/fix-cache-adapters-cfg branch from 9673243 to 4a77f44 Compare December 7, 2021 16:39
@fancyweb
Copy link
Contributor Author

fancyweb commented Dec 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 😅

@fancyweb fancyweb force-pushed the fwb/fix-cache-adapters-cfg branch from 4a77f44 to c4a9dff Compare December 11, 2021 11:34
@fancyweb fancyweb force-pushed the fwb/fix-cache-adapters-cfg branch from c4a9dff to 863dd5e Compare December 11, 2021 11:51
@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas merged commit 6e54976 into symfony:4.4 Dec 11, 2021
This was referenced Dec 29, 2021
Merged
Merged
Merged
Merged
@fancyweb fancyweb deleted the fwb/fix-cache-adapters-cfg branch December 30, 2021 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug FrameworkBundle Status: Reviewed
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
4 participants
@fancyweb @ro0NL @nicolas-grekas @carsonbot