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

allow environment variables for remove_token_from_body_when_cookies_used #1092

Merged
merged 1 commit into from
Dec 11, 2022

Conversation

usu
Copy link
Contributor

@usu usu commented Dec 3, 2022

#940 introduced a new config option to enable response body when cookies are used:

remove_token_from_body_when_cookies_used: false

However, currently environment variables cannot be used to control this config. An error Environment variables "bool:JWT_REMOVE_TOKEN_FROM_BODY" are never used is thrown. After this PR, this option can also be used as following:

remove_token_from_body_when_cookies_used: '%env(bool:JWT_REMOVE_TOKEN_FROM_BODY)%'

@chalasr
Copy link
Collaborator

chalasr commented Dec 3, 2022

Thanks for the PR. Did you actually test it? I suspect this is not enough to properly support setting it via an env variable:
https://github.com/symfony/symfony/blob/043257f6fc1fceb28bc0c37c59f2bea4d9c8d79d/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L1945

@usu
Copy link
Contributor Author

usu commented Dec 3, 2022

I tested successfully with my own application. Symfony DI automatically resolves the environment parameters when generating the DI factories. I assume this is the reason why environment variables already work for all the other config options.

Happy to provide a test, but couldn't find a good test to hook into or to copy from. There was no test provided when introducing #940 and couldn't really find a test that already utilizes environment variables for other config options.

@usu
Copy link
Contributor Author

usu commented Dec 11, 2022

Hi @chalasr, anything you'd want me to add to this PR to make this mergeable?

@chalasr
Copy link
Collaborator

chalasr commented Dec 11, 2022

Thank you @usu.

@chalasr chalasr merged commit 082b4a4 into lexik:2.x Dec 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants