-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[ExpressionLanguage] Expr. functions added after first eval/compile didn't exist #21098
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
Conversation
I think you can just create new instance of |
I prefer that |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if passing the functions by reference ID actually a good idea as it may lead to unexpected side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a break BC?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is.
@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
* @see ExpressionFunction | ||
*/ | ||
public function register($name, $compiler, $evaluator) | ||
{ | ||
if ($this->parser) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
null !== $this->parser
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (but why not use automatic boolean casting in fact?).
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would reword this a bit to let it sound less confusing:
Registering functions after calling evaluate(), compile(), or parse() is not supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
…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
If we add expr. function after first eval/compile like this:
A
SyntaxError
is thrown that saysThe function "fn" does not exist around position 1.
. It's the same bug with$el->compile('fn()')
.This PR fixes this.