Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DependencyInjection] Fix autocastingnull env values to empty string withcontainer.env_var_processors_locator#51198
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $prefix =''; | ||
| } | ||
| $processor =$processors->has($prefix) ?$processors->get($prefix) :newEnvVarProcessor($this); | ||
| if (false ===$i && EnvVarProcessor::class ===\get_class($processor)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
or instanceof I don't know?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is this extra condition needed at all? I removed it and tests still pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Because we implemented the empty string behavior only in our EnvVarProcessor class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looking at this again, I think it's safer to add the condition 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not fan of hardcoding a behavior for EnvVarProcessor only. Can't we add some phpdoc in the interface to explain how a processor should behave instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We could, but my point is that we added this special behavior on 5.4 as a bug fix 😅 Isn't it too late to change the behavior for every consumer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think it's too much an edge case, nobody replaced the "string" processor :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I added a comment but I'm not convinced it's needed/understandable. I let you takeover 😄
fancyweb commentedJul 31, 2023
Ping@bendavies can you try this patch plz? |
null env values to empty string withcontainer.env_var_processors_locatordb34fb0 toa5abf0dCompare0773b18 to38bc998Compare
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Friendly ping@bendavies can you please try this patch and report back?
| $prefix =''; | ||
| } | ||
| $processor =$processors->has($prefix) ?$processors->get($prefix) :newEnvVarProcessor($this); | ||
| if (false ===$i && EnvVarProcessor::class ===\get_class($processor)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is this extra condition needed at all? I removed it and tests still pass.
bendavies commentedAug 15, 2023
@nicolas-grekas i'm afraid i'm afk on holiday for 3 weeks. i can check when i'm back if you can wait. |
bendavies commentedSep 8, 2023
yes, this seems to resolve the issue. |
38bc998 to5f0a0e2Comparesrc/Symfony/Component/DependencyInjection/EnvVarProcessorInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
… with container.env_var_processors_locator
5f0a0e2 to766bc6eComparenicolas-grekas commentedSep 20, 2023
Thank you@fancyweb. |
The previous fix doesn't work when
$processorscomes fromcontainer.env_var_processors_locator.