[Security] Notify that symfony/expression-language is not installed if ExpressionLanguage is used #25823

giovannialbero1992 · Jan 17, 2018

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 requested

fancyweb · Jan 17, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguage.php

@@ -13,6 +13,10 @@

 use Symfony\Component\ExpressionLanguage\ExpressionLanguage as BaseExpressionLanguage;

+if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {
+    throw new \LogicException('Install symfony/expression-language');


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." ?

Ok, thanks!

fancyweb · Jan 17, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguageProvider.php

@@ -14,6 +14,10 @@
 use Symfony\Component\ExpressionLanguage\ExpressionFunction;
 use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface;

+if (!interface_exists('Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface')) {
+    throw new \RuntimeException('The ExpressionLanguage class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.');


The name of this class is "ExpressionLanguageProvider".

Sorry, I've done in a hurry, like a monkey with copy and paste.

nicolas-grekas · Jan 17, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguage.php

@@ -13,6 +13,10 @@

 use Symfony\Component\ExpressionLanguage\ExpressionLanguage as BaseExpressionLanguage;

+if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {
+    throw new \RuntimeException('The ExpressionLanguage class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.');
+}


Unfortunately this approach will not work. The class must be in the "else".

Why ? If an exception is thrown PHP will never reach the class declaration and won't try to find the parent class.

because PHP parses class ahead of time, we already got bitten by that in the past

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

we already merged such kind of code, and it broke, opcache involved maybe
I wouldn't take the risk another time

@nicolas-grekas I've modified my commit in according with your suggestions. I hope is all ok, thank you.

nicolas-grekas · Jan 17, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguageProvider.php

@@ -14,6 +14,10 @@
 use Symfony\Component\ExpressionLanguage\ExpressionFunction;
 use Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface;

+if (!interface_exists('Symfony\Component\ExpressionLanguage\ExpressionFunctionProviderInterface')) {


Done really need that logic on this class? Looks superfluous to me, but I may be missing something.

you are right, this class also used in ExpressionLanguage.

nicolas-grekas

last changes :)

nicolas-grekas · Jan 17, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguage.php

- */
-class ExpressionLanguage extends BaseExpressionLanguage
-{
+if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {


CS comment: should use BaseExpressionLanguage::class here, and

nicolas-grekas · Jan 17, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguage.php

-class ExpressionLanguage extends BaseExpressionLanguage
-{
+if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {
+    throw new \RuntimeException('The ExpressionLanguage class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.');


The "%s" class, with FQCN and double quotes around, with ExpressionLanguage::class given

It's not the case in others similar messages (in the Serializer component for example). I guess we should harmonize them ?

Actually not: we don't harmonize such CS things because they create merge conflicts when merging lower branches into upper ones.

weaverryan · Jan 17, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguage.php

-class ExpressionLanguage extends BaseExpressionLanguage
-{
+if (!class_exists(BaseExpressionLanguage::class)) {
+    throw new \RuntimeException(sprintf('The "%s" class requires the "ExpressionLanguage" component. Install "symfony/expression-language" to use it.', ExpressionLanguage::class));


👍 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 that ExpressionLanguage 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)

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 · Jan 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

(consistent with the experience provided by Flex)

nicolas-grekas · Jan 18, 2018

src/Symfony/Component/Security/Core/Authorization/ExpressionLanguage.php

-class ExpressionLanguage extends BaseExpressionLanguage
-{
+if (!class_exists(BaseExpressionLanguage::class)) {
+    throw new \RuntimeException(sprintf('The "%s" class requires the "ExpressionLanguage" component. Try running "composer require symfony/expression-language".', ExpressionLanguage::class));


\LogicException !

…f ExpressionLanguage and ExpressionLanguagePrivider are used

fabpot · Jan 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 requested Commits ------- 6aa2b7c [Security] Notify that symfony/expression-language is not installed if ExpressionLanguage and ExpressionLanguagePrivider are used

This is broken since sf 3.4.4, see symfony/symfony#25823 One of api platforms listener depends on a file that now throws if it is loaded and the expression language component is missing. The solution is to avoid creating the service if the component is missing. This fixes an error people will have even if they do not use the access_control annotation attribute.

carsonbot added Status: Needs Review Security Bug labels Jan 17, 2018

fancyweb reviewed Jan 17, 2018

View reviewed changes

giovannialbero1992 force-pushed the 3.4 branch from abbe5b7 to ef25395 Compare January 17, 2018 18:05

fancyweb reviewed Jan 17, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jan 17, 2018

nicolas-grekas requested changes Jan 17, 2018

View reviewed changes

giovannialbero1992 force-pushed the 3.4 branch from ef25395 to 175d0ed Compare January 17, 2018 21:00

giovannialbero1992 changed the title ~~[Security] Notify that symfony/expression-language is not installed if ExpressionLanguage and ExpressionLanguagePrivider are used~~ [Security] Notify that symfony/expression-language is not installed if ExpressionLanguage is used Jan 17, 2018

nicolas-grekas reviewed Jan 17, 2018

View reviewed changes

giovannialbero1992 force-pushed the 3.4 branch 2 times, most recently from 47bd5c6 to b16cdc8 Compare January 17, 2018 21:28

weaverryan reviewed Jan 17, 2018

View reviewed changes

giovannialbero1992 force-pushed the 3.4 branch from b16cdc8 to d58d73f Compare January 18, 2018 07:45

nicolas-grekas approved these changes Jan 18, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jan 18, 2018

nicolas-grekas reviewed Jan 18, 2018

View reviewed changes

[Security] Notify that symfony/expression-language is not installed i…

6aa2b7c

…f ExpressionLanguage and ExpressionLanguagePrivider are used

giovannialbero1992 force-pushed the 3.4 branch from d58d73f to 6aa2b7c Compare January 18, 2018 11:13

xabbuh approved these changes Jan 18, 2018

View reviewed changes

fabpot approved these changes Jan 18, 2018

View reviewed changes

fabpot merged commit 6aa2b7c into symfony:3.4 Jan 18, 2018

fabpot mentioned this pull request Jan 29, 2018

Release v3.4.4 #25956

Merged

fabpot mentioned this pull request Jan 29, 2018

Release v4.0.4 #25957

Merged

greg0ire mentioned this pull request Mar 28, 2018

Avoid creating potentially crashing service api-platform/core#1797

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[Security] Notify that symfony/expression-language is not installed if ExpressionLanguage is used #25823

[Security] Notify that symfony/expression-language is not installed if ExpressionLanguage is used #25823

Uh oh!

Conversation

giovannialbero1992 commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xabbuh commented Jan 18, 2018

Uh oh!

nicolas-grekas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jan 18, 2018

Uh oh!

Uh oh!

giovannialbero1992 commented Jan 17, 2018 •

edited

Loading