[ExpressionLanguage] Expr. functions added after first eval/compile didn't exist #21098
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I think you can just create new instance of |
|
I prefer that |
xabbuh
reviewed
Dec 30, 2016
| @@ -21,9 +21,9 @@ class Compiler | |||
| private $source; | |||
| private $functions; | |||
|
|
|||
| public function __construct(array $functions) | |||
| public function __construct(array &$functions) | |||
There was a problem hiding this comment.
I am not sure if passing the functions by reference ID actually a good idea as it may lead to unexpected side effects.
|
@maidmaid the |
|
I would instead forbid changing the ExpressionLanguage functions after calling evaluate the first time. |
|
How do you see this @fabpot? By throwing an exception? |
|
@maidmaid I think throwing an exception would make sense. |
|
But throw an exception in existing behavior, is not it a BC break? |
|
No, as it currently does not work anyway. |
…s (ogizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Allow to count on lazy collection arguments | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#21450 (comment) | License | MIT | Doc PR | todo (with symfony/symfony-docs#7336) When using the new iterator feature of the DI component to lazy load collection, we always know the number of arguments in the collection (only the invalidBehavior set to `IGNORE_ON_INVALID_REFERENCE` may change this number). So we are able to generate and use a `RewindableGenerator` implementing `\Countable` by computing this value ahead. So, in a service accepting `array|iterable`, like in the `GuardAuthenticationListener` (see symfony#21450): ```php class GuardAuthenticationListener implements ListenerInterface { private $guardAuthenticators; /** * @param iterable|GuardAuthenticatorInterface[] $guardAuthenticators The authenticators, with keys that match what's passed to GuardAuthenticationProvider * @param LoggerInterface $logger A LoggerInterface instance */ public function __construct($guardAuthenticators, LoggerInterface $logger = null) { // ... } public function handle(GetResponseEvent $event) { if (null !== $this->logger) { $context = array() if (is_array($this->guardAuthenticators) || $this->guardAuthenticators instanceof \Countable) { $context['authenticators'] = count($this->guardAuthenticators); } $this->logger->debug('Checking for guard authentication credentials.', $context); } // ... } } ``` we still keep the ability to call count without loosing the lazy load benefits. Commits ------- f23e460 [DI] Allow to count on lazy collection arguments
…terj) This PR was merged into the 3.3-dev branch. Discussion ---------- [WebServerBundle] Improved exception message | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - This is a quite minor one, but imo "guessing" is something that's optional. If I don't pass value X, a script will guess value X. However, in this case, the front controller file cannot be specified. It has to be one of the "guessed" file names. That's why I renamed "guessing" to "finding". While doing this, I also added the possible file names in the exception message to ease fixing problems. Commits ------- df46188 Improved exception message
…(nicolas-grekas) This PR was merged into the 3.3-dev branch. Discussion ---------- [DI][Config] Add & use ReflectionClassResource | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | symfony#21079 | License | MIT | Doc PR | - With new changes comming to 3.3, we need a more generic reflection tracking logic than the one already managed by the autowiring subsystem. This PR adds a new ReflectionClassResource in the Config component, and a new ContainerBuilder::getReflectionClass() method in the DI one (for fetching+tracking reflection-related info). ReflectionClassResource tracks changes to any public or protected properties/method. PR updated and ready, best viewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/21419/files?w=1). changelog: * added `ReflectionClassResource` class * added second `$exists` constructor argument to `ClassExistenceResource` - with a special mode that prevents fatal errors from happening when some parent class is broken (logic generalized from AutowiringPass) * made `ClassExistenceResource` also work with interfaces and traits * added `ContainerBuilder::getReflectionClass()` for retrieving and tracking reflection class info * deprecated `ContainerBuilder::getClassResource()`, use `ContainerBuilder::getReflectionClass()` or `ContainerBuilder::addObjectResource()` instead Commits ------- 37e4493 [DI][Config] Add & use ReflectionClassResource
This PR was merged into the 3.3-dev branch. Discussion ---------- [DI] Getter autowiring | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | todo This PR adds support for getter autowiring. symfony#20973 must be merged first. Example: ```yaml # app/config/config.yml services: Foo\Bar: autowire: ['get*'] ``` ```php namespace Foo; class Bar { protected function getBaz(): Baz // this feature only works with PHP 7+ { } } class Baz { } ```` `Baz` will be automatically registered as a service and an instance will be returned when `Bar::getBaz` will be called (and only at this time, lazy loading). This feature requires PHP 7 or superior. Commits ------- c48c36b [DI] Add support for getter autowiring
…t value resolvers (chalasr) This PR was merged into the 3.3-dev branch. Discussion ---------- [HttpKernel][FrameworkBundle] Lazy load argument value resolvers | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a The ArgumentResolver resolves an arg using the first ArgumentValueResolver which `supports()` it (which can be complex, see e.g. [sensiolabs/SensioFrameworkExtraBundle#436](https://github.com/sensiolabs/SensioFrameworkExtraBundle/pull/436/files#diff-865d48d9369c4431bce36ba642834570R10)). Commits ------- 02b4aaa [HttpKernel] Lazy load argument value resolvers
…t, ClosureProxy and arrays
…port IteratorArgument, ClosureProxy and arrays (ogizanagi) This PR was merged into the 3.3-dev branch. Discussion ---------- [FrameworkBundle][Console] Fix descriptors to support IteratorArgument, ClosureProxy and arrays | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | N/A | License | MIT | Doc PR | N/A Fixes both array and iterator/closure-proxy arguments support in descriptors when using `--show-arguments` Commits ------- a94924c [FrameworkBundle][Console] Fix descriptors to support IteratorArgument, ClosureProxy and arrays
…t original output (ogizanagi) This PR was merged into the 2.8 branch. Discussion ---------- [FrameworkBundle][Console] JsonDescriptor: Respect original output | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | no | Fixed tickets | N/A | License | MIT | Doc PR | N/A Follows up symfony#21501. This one fixes the keys order (preserved from the order those keys are added from the `JsonDescriptor`), as for the previous mentioned PR, in order to slightly improve the situation when updating the descriptors fixtures. @nicolas-grekas : Thanks for taking care of the previous one. There are two other PRs required to me in order to fix everything on every branches, but I wonder if it wouldn't be easier (and reduce noise) to apply the following patches while merging this in upper branches instead: - 3.2: ogizanagi@51a0a1c - master: ogizanagi@b35a244 WDYT? Commits ------- bf71776 [FrameworkBundle][Console] JsonDescriptor: Respect original output
* 2.7: [Process] Non ASCII characters disappearing during the escapeshellarg
* 2.8: [FrameworkBundle][Console] JsonDescriptor: Respect original output [Process] Non ASCII characters disappearing during the escapeshellarg
…ype columns (dmaicher) This PR was merged into the 3.2 branch. Discussion ---------- [DoctrineBridge] Fixed validating custom doctrine type columns | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#21619 | License | MIT | Doc PR | - This fixes symfony#21619 by not assuming the invalid `$value` is a Doctrine entity if its an object Commits ------- ad59370 [DoctrineBridge] Fixed validating custom doctrine type columns
* 2.8: fix deps
* 3.2: fix deps [DoctrineBridge] Fixed validating custom doctrine type columns
* 3.2: fix deps
* 3.2: fix phpunit bridge tests
|
Sorry for delay... I done changes. |
* 2.7: fix priority ordering of security voters
* 2.8: fix priority ordering of security voters
* 3.2: fix priority ordering of security voters
* 3.2: fixed bad merge
xabbuh
reviewed
Feb 22, 2017
| * @see ExpressionFunction | ||
| */ | ||
| public function register($name, $compiler, $evaluator) | ||
| { | ||
| if ($this->parser) { |
There was a problem hiding this comment.
Done (but why not use automatic boolean casting in fact?).
xabbuh
reviewed
Feb 22, 2017
| * @see ExpressionFunction | ||
| */ | ||
| public function register($name, $compiler, $evaluator) | ||
| { | ||
| if ($this->parser) { | ||
| throw new \LogicException('It\'s impossible to register function after calling evaluate/compilate the first time.'); |
There was a problem hiding this comment.
I would reword this a bit to let it sound less confusing:
Registering functions after calling evaluate(), compile(), or parse() is not supported.
fabpot
approved these changes
Feb 22, 2017
| @@ -110,10 +110,16 @@ public function parse($expression, $names) | |||
| * @param callable $compiler A callable able to compile the function | |||
| * @param callable $evaluator A callable able to evaluate the function | |||
| * | |||
| * @throws \LogicException when register a function after calling evaluate(), compile() or parse() the first time | |||
There was a problem hiding this comment.
I would say when registering a function after calling evaluate(), compile(), or parse().
2cd27f4 to
800e148
Compare
|
I rebased my PR after update the tests and next 💥. I'm confused, I must recreate a new PR? |
|
I rebased from master branch instead 2.7 branch... I have recreated PR #21722, sorry for the noise. |
fabpot
added a commit
that referenced
this pull request
Feb 22, 2017
…valuate(), compile() or parse() is not supported (maidmaid) This PR was squashed before being merged into the 2.7 branch (closes #21722). Discussion ---------- [ExpressionLanguage] Registering functions after calling evaluate(), compile() or parse() is not supported | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a If we add expr. function after first eval/compile like this: ```php $el = new ExpressionLanguage(); $el->evaluate('1 + 1'); $el->addFunction(new ExpressionFunction('fn', function () {}, function () {})); $el->evaluate('fn()'); ``` A ``SyntaxError`` is thrown that says ``The function "fn" does not exist around position 1.``. It's the same bug with ``$el->compile('fn()')``. This PR fixes this (duplicate of #21098 that was closed). Commits ------- e305369 [ExpressionLanguage] Registering functions after calling evaluate(), compile() or parse() is not supported
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If we add expr. function after first eval/compile like this:
A
SyntaxErroris thrown that saysThe function "fn" does not exist around position 1.. It's the same bug with$el->compile('fn()').This PR fixes this.