8000 [ExpressionLanguage] Expr. functions added after first eval/compile didn't exist by maidmaid · Pull Request #21098 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Closed
wants to merge 5,271 commits into from

Conversation

maidmaid
Copy link
Contributor
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:

$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.

@unkind
Copy link
Contributor
unkind commented Dec 30, 2016

I think you can just create new instance of Compiler in addFunction (and in other mutators if any).

@maidmaid
Copy link
Contributor Author

I prefer that ExpressionLanguage, Compiler and Parser use the same reference of array of functions. If EL adds a new function, Compiler and Parser see this change without be recreated.

@@ -21,9 +21,9 @@ class Compiler
private $source;
private $functions;

public function __construct(array $functions)
public function __construct(array &$functions)
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is.

@unkind
Copy link
Contributor
unkind commented Dec 30, 2016

@maidmaid the ExpressionLanguage service has a design issue: mutable state. If it received functions through constructor only, you wouldn't encounter it. When you want to fix it by adding more mutability (for Compiler and Parser), you repeat 8000 the same mistake.

@fabpot
Copy link
Member
fabpot commented Dec 31, 2016

I would instead forbid changing the ExpressionLanguage functions after calling evaluate the first time.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Jan 2, 2017
@maidmaid
Copy link
Contributor Author
maidmaid commented Jan 2, 2017

How do you see this @fabpot? By throwing an exception?

@fabpot
Copy link
Member
fabpot commented Jan 2, 2017

@maidmaid I think throwing an exception would make sense.

@maidmaid
Copy link
Contributor Author
maidmaid commented Jan 3, 2017

But throw an exception in existing behavior, is not it a BC break?

@fabpot
Copy link
Member
fabpot commented Jan 3, 2017

No, as it currently does not work anyway.

nicolas-grekas and others added 16 commits February 2, 2017 15:11
…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
…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
fabpot and others added 8 commits February 21, 2017 07:00
…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
* 3.2:
  fix deps
  [DoctrineBridge] Fixed validating custom doctrine type columns
* 3.2:
  fix deps
* 3.2:
  fix phpunit bridge tests
@maidmaid
Copy link
Contributor Author

Sorry for delay... I done changes.

9E81

* 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) {
Copy link
Member

Choose a reason for hiding this comment

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

null !== $this->parser

Copy link
Contributor Author

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.');
Copy link
Member
@xabbuh xabbuh Feb 22, 2017

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.

Copy link
Contributor Author

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
Copy link
Member

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().

@maidmaid
Copy link
Contributor Author

I rebased my PR after update the tests and next 💥. I'm confused, I must recreate a new PR?

@maidmaid
Copy link
Contributor Author

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

0