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

[DI] Throw an exception if Expression Language is not installed #25285

Merged
merged 1 commit into from Dec 4, 2017

Conversation

Projects
None yet
5 participants
@sroze
Member

sroze commented Dec 3, 2017

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #25277
License MIT
Doc PR ø

The PhpDumper already has this check but it is missing here.

@sroze sroze changed the title from [DI] Throw an exception is expression language is not installed to [DI] Throw an exception if Expression Language is not installed Dec 3, 2017

@ostrolucky

This comment has been minimized.

Show comment
Hide comment
@ostrolucky

ostrolucky Dec 3, 2017

Contributor

Are we really going to do this now on each place where optional dependency is used? I'm 👎 on that. There should be global solution and/or not doing this at all and just document what such php error usually means.

Contributor

ostrolucky commented Dec 3, 2017

Are we really going to do this now on each place where optional dependency is used? I'm 👎 on that. There should be global solution and/or not doing this at all and just document what such php error usually means.

@@ -155,6 +156,10 @@ private function getDefinitionId($id)
private function getExpressionLanguage()
{
if (null === $this->expressionLanguage) {
if (!class_exists('Symfony\Component\ExpressionLanguage\ExpressionLanguage')) {

This comment has been minimized.

@Simperfit

Simperfit Dec 3, 2017

Contributor

why not :

if (!class_exists(ExpressionLanguage::class)) {
//body
}
@Simperfit

Simperfit Dec 3, 2017

Contributor

why not :

if (!class_exists(ExpressionLanguage::class)) {
//body
}

This comment has been minimized.

@sroze

sroze Dec 3, 2017

Member

copy/pasting from the other check, that's it :)

@sroze

sroze Dec 3, 2017

Member

copy/pasting from the other check, that's it :)

This comment has been minimized.

@Simperfit

Simperfit Dec 3, 2017

Contributor

Yeah but I think it's better to use this since we already replaced some of the one we had in the symfony code base. Let's introduce something we don't need to re-modify in the futur :p.

@Simperfit

Simperfit Dec 3, 2017

Contributor

Yeah but I think it's better to use this since we already replaced some of the one we had in the symfony code base. Let's introduce something we don't need to re-modify in the futur :p.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Dec 3, 2017

@nicolas-grekas

This comment has been minimized.

Show comment
Hide comment
@nicolas-grekas

nicolas-grekas Dec 4, 2017

Member

Thank you @sroze.

Member

nicolas-grekas commented Dec 4, 2017

Thank you @sroze.

@nicolas-grekas nicolas-grekas merged commit 75b21e9 into symfony:3.4 Dec 4, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Dec 4, 2017

bug #25285 [DI] Throw an exception if Expression Language is not inst…
…alled (sroze)

This PR was merged into the 3.4 branch.

Discussion
----------

[DI] Throw an exception if Expression Language is not installed

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #25277
| License       | MIT
| Doc PR        | ø

The [`PhpDumper` already has this check](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/DependencyInjection/Dumper/PhpDumper.php#L1688-L1690) but it is missing here.

Commits
-------

75b21e9 Throw an exception is expression language is not installed

@sroze sroze deleted the sroze:ensure-expression-language-is-installed branch Dec 4, 2017

This was referenced Dec 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment