Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Notify that symfony/expression-language is not installed if ExpressionLanguage is used#25823
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
| useSymfony\Component\ExpressionLanguage\ExpressionLanguageasBaseExpressionLanguage; | ||
| if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { | ||
| thrownew \LogicException('Install symfony/expression-language'); |
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.
To be consistent with (the most) similar exceptions already in the code, what about throwing a \RuntimeException with a more precise message like "The ExpressionLanguage class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it." ?
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.
Ok, thanks!
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.
Done
| useSymfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface; | ||
| if (!interface_exists('Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface')) { | ||
| thrownew \RuntimeException('The ExpressionLanguage class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.'); |
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.
The name of this class is "ExpressionLanguageProvider".
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.
Sorry, I've done in a hurry, like a monkey with copy and paste.
| if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { | ||
| thrownew \RuntimeException('The ExpressionLanguage class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.'); | ||
| } |
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.
Unfortunately this approach will not work. The class must be in the "else".
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 ? If an exception is thrown PHP will never reach the class declaration and won't try to find the parent 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.
because PHP parses class ahead of time, we already got bitten by that in the past
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.
Yes some things are done ahead of time for class declarations, but in the case we have here it works without having to put the whole class in an else clause :https://3v4l.org/mELj9
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 already merged such kind of code, and it broke, opcache involved maybe
I wouldn't take the risk another time
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.
@nicolas-grekas I've modified my commit in according with your suggestions. I hope is all ok, thank you.
| useSymfony\Component\ExpressionLanguage\ExpressionFunction; | ||
| useSymfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface; | ||
| if (!interface_exists('Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface')) { |
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.
Done really need that logic on this class? Looks superfluous to me, but I may be missing something.
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.
you are right, this class also used in ExpressionLanguage.
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.
last changes :)
| */ | ||
| class ExpressionLanguageextends BaseExpressionLanguage | ||
| { | ||
| if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { |
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.
CS comment: should use BaseExpressionLanguage::class here, and
| class ExpressionLanguageextends BaseExpressionLanguage | ||
| { | ||
| if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) { | ||
| thrownew \RuntimeException('The ExpressionLanguage class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.'); |
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.
The "%s" class, with FQCN and double quotes around, with ExpressionLanguage::class given
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.
It's not the case in others similar messages (in the Serializer component for example). I guess we should harmonize them ?
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.
Actually not: we don't harmonize such CS things because they create merge conflicts when merging lower branches into upper ones.
47bd5c6 tob16cdc8Compare| class ExpressionLanguageextends BaseExpressionLanguage | ||
| { | ||
| if (!class_exists(BaseExpressionLanguage::class)) { | ||
| thrownew \RuntimeException(sprintf('The "%s" class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.', ExpressionLanguage::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.
👍 for this. To make the language consistent with other spots:
The "%s" class requires the "ExpressionLanguage" component. Try running "composer require symfony/expression-language".
Is there a specific time when this class is used? I mean, obviously, if you reference the class directly, it's used :). But is it if you try to use expressions inside, for example,access_control? I'm asking only because if there is a specific use-case that activates this class 99% of the time, it might be better to mention that use case (e.g. "To use expressions in access_control, you must install...") versus saying that thatExpressionLanguage class requires the component (when the user is not really interacting with this class directly, so might not be sure where this is coming from)
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've updated the message, for the rest i don't know if in the future someone will use this class without install ExpressionLanguage.
xabbuh commentedJan 18, 2018
I wonder if we did that for other classes that extend classes coming from optional dependencies in the past. And if not, why should we do that now for this particular class only? |
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.
(consistent with the experience provided by Flex)
| class ExpressionLanguageextends BaseExpressionLanguage | ||
| { | ||
| if (!class_exists(BaseExpressionLanguage::class)) { | ||
| thrownew \RuntimeException(sprintf('The "%s" class requires the "ExpressionLanguage" component. Try running "composer require symfony/expression-language".', ExpressionLanguage::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.
\LogicException !
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.
👍
…f ExpressionLanguage and ExpressionLanguagePrivider are used
fabpot commentedJan 18, 2018
Thank you@giovannialbero1992. |
…installed if ExpressionLanguage is used (giovannialbero1992)This PR was merged into the 3.4 branch.Discussion----------[Security] Notify that symfony/expression-language is not installed if ExpressionLanguage is used| Q | A| ------------- | ---| Branch? | master for features / 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#25742| License | MIT| Doc PR | not requestedCommits-------6aa2b7c [Security] Notify that symfony/expression-language is not installed if ExpressionLanguage and ExpressionLanguagePrivider are used
This is broken since sf 3.4.4, seesymfony/symfony#25823One of api platforms listener depends on a file that now throws if it isloaded and the expression language component is missing. The solution isto avoid creating the service if the component is missing.This fixes an error people will have even if they do not use theaccess_control annotation attribute.
This is broken since sf 3.4.4, seesymfony/symfony#25823One of api platforms listener depends on a file that now throws if it isloaded and the expression language component is missing. The solution isto avoid creating the service if the component is missing.This fixes an error people will have even if they do not use theaccess_control annotation attribute.
This is broken since sf 3.4.4, seesymfony/symfony#25823One of api platforms listener depends on a file that now throws if it isloaded and the expression language component is missing. The solution isto avoid creating the service if the component is missing.This fixes an error people will have even if they do not use theaccess_control annotation attribute.
This is broken since sf 3.4.4, seesymfony/symfony#25823One of api platforms listener depends on a file that now throws if it isloaded and the expression language component is missing. The solution isto avoid creating the service if the component is missing.This fixes an error people will have even if they do not use theaccess_control annotation attribute.
This is broken since sf 3.4.4, seesymfony/symfony#25823One of api platforms listener depends on a file that now throws if it isloaded and the expression language component is missing. The solution isto avoid creating the service if the component is missing.This fixes an error people will have even if they do not use theaccess_control annotation attribute.
Uh oh!
There was an error while loading.Please reload this page.