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

Adding checks for the expression language #25137

Merged
merged 1 commit into from Nov 24, 2017
Merged

Conversation

@weaverryan
Copy link
Member

@weaverryan weaverryan commented Nov 23, 2017

Q A
Branch? 4.0
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR not needed

If you try to use the expression syntax in DI, this will drastically improve the error message :)

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Nov 23, 2017

3.4?

Loading

@@ -488,6 +488,10 @@ private function getArgumentsAsPhp(\DOMElement $node, $name, $file, $lowercase =
$arguments[$key] = new Reference($arg->getAttribute('id'), $invalidBehavior);
break;
case 'expression':
if (!class_exists(Expression::class)) {
throw new \LogicException(sprintf('type="expression" cannot be used without the expression component. Try running "composer require symfony/expression-language".'));
Copy link
Member

@javiereguiluz javiereguiluz Nov 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The symfony/ part is always optional when using Flex. Could we use composer require expression-language instead?

Loading

Copy link
Member Author

@weaverryan weaverryan Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is inside a component, it feels too presumptive to put the Flex alias.

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:
The type="expression" attribute cannot be used without the ExpressionLanguage component. Try running "composer require symfony/expression-language". (we already use that ExpressionLanguage name in other similar exceptions.)

Loading

@weaverryan weaverryan changed the base branch from 4.0 to 3.4 Nov 24, 2017
@weaverryan weaverryan force-pushed the expression-check branch from 0f13584 to d6214f9 Nov 24, 2017
@weaverryan
Copy link
Member Author

@weaverryan weaverryan commented Nov 24, 2017

Target branch changed!

Loading

@chalasr chalasr added this to the 3.4 milestone Nov 24, 2017
@@ -775,6 +775,10 @@ private function resolveServices($value, $file, $isParameter = false)
$value[$k] = $this->resolveServices($v, $file, $isParameter);
}
} elseif (is_string($value) && 0 === strpos($value, '@=')) {
if (!class_exists(Expression::class)) {
throw new \LogicException(sprintf('The @= expression syntax cannot be used without the expression component. Try running "composer require symfony/expression-language".'));
Copy link
Member

@nicolas-grekas nicolas-grekas Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "@=" expression syntax cannot be used without the ExpressionLanguage component. Try running "composer require symfony/expression-language".

Loading

Copy link
Member

@nicolas-grekas nicolas-grekas Nov 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(reminder: strings in double quotes are in bold in profiler panels ;) )

Loading

@weaverryan weaverryan force-pushed the expression-check branch from d6214f9 to e17232d Nov 24, 2017
@weaverryan
Copy link
Member Author

@weaverryan weaverryan commented Nov 24, 2017

Changes made!

Loading

@fabpot
Copy link
Member

@fabpot fabpot commented Nov 24, 2017

@weaverryan Can you rebase?

Loading

@weaverryan weaverryan force-pushed the expression-check branch from e17232d to 3502020 Nov 24, 2017
@weaverryan
Copy link
Member Author

@weaverryan weaverryan commented Nov 24, 2017

Rebased. Sorry - I rebased locally... and did a bad job. Looks better now

Loading

fabpot
fabpot approved these changes Nov 24, 2017
@fabpot
Copy link
Member

@fabpot fabpot commented Nov 24, 2017

Thank you @weaverryan.

Loading

@fabpot fabpot merged commit 3502020 into symfony:3.4 Nov 24, 2017
3 checks passed
Loading
fabpot added a commit that referenced this issue Nov 24, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

Adding checks for the expression language

| Q             | A
| ------------- | ---
| Branch?       | 4.0
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | not needed

If you try to use the expression syntax in DI, this will drastically improve the error message :)

Commits
-------

3502020 adding checks for the expression language
@weaverryan weaverryan deleted the expression-check branch Nov 24, 2017
@fabpot fabpot mentioned this pull request Nov 30, 2017
@fabpot fabpot mentioned this pull request Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants