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.

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

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

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

@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

@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 😅

@nicolas-grekas
Copy link
Member

Thank you @fancyweb.

@nicolas-grekas nicolas-grekas merged commit 6e54976 into symfony:4.4 Dec 11, 2021
@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants