Skip to content

Changes setup of expression language functions to fix issue #588 #697

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

Merged
merged 15 commits into from
Jul 8, 2020

Conversation

sgehrig
Copy link
Contributor

@sgehrig sgehrig commented Jul 8, 2020

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Documented? no
Fixed tickets #588
License MIT

As mentioned in comment #588 (comment) the true problem behind issue #588 is that the setup of the expression language functions is flawed. The expression language functions are not designed to take services as constructor arguments. Instead they must rely on dependencies passed into the function during execution.

This PR fixes these issues with several expression language functions. Instead of passing dependencies to the constructor (from where they were injected into closures), the evaluators no rely on the 'globalVariable' in the $arguments parameter - just as the compiler relies on $globalVariable variable in the execution context. 'globalVariable' just points to an instance of Overblog\GraphQLBundle\Definition\GlobalVariables (again just the same as for complied function execution).

Because all of the expression language functions are auto-wired and because the evaluator is only used directly in Overblog\GraphQLBundle\Validator\Constraints\ExpressionValidator, there has only been a slight service configuration change (the service validator.expression (instance of Overblog\GraphQLBundle\Validator\Constraints\ExpressionValidator) now also requires the Overblog\GraphQLBundle\Definition\GlobalVariables service as its second constructor argument.

Copy link
Contributor

@mcg-web mcg-web left a comment

Choose a reason for hiding this comment

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

This look good to me. Thank you @sgehrig !

@murtukov
Copy link
Contributor

murtukov commented Jul 8, 2020

LGTM too. I've made couple of changes in the master tho, so that globalVaribales is not hardcoded. It's used in many places, sometimes plural (globalVariables) and sometimes singular. This change should be applied to the master branch taking into account my changes. I can do it.

@murtukov
Copy link
Contributor

murtukov commented Jul 8, 2020

Thanks you @sgehrig for fixing this annoying issue 👍🏻

@mcg-web mcg-web merged commit 4440c09 into overblog:0.13 Jul 8, 2020
@sgehrig sgehrig deleted the issues/gh588 branch July 8, 2020 13:46
@sgehrig
Copy link
Contributor Author

sgehrig commented Jul 8, 2020

Thanks you @sgehrig for fixing this annoying issue 👍🏻

Any chance to do a new release for 0.13.3? Oh, and thanks for merging the PR so quick.

@murtukov
Copy link
Contributor

murtukov commented Jul 8, 2020

@sgehrig all release related questions are for @mcg-web

@mcg-web
Copy link
Contributor

mcg-web commented Jul 8, 2020

done @sgehrig ! Thanks again for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants