Skip to content
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

Define missing macros variable in constructor #4416

Closed
wants to merge 1 commit into from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Oct 24, 2024

When defining a macro in a template, the constructor looks like this:

    public function __construct(\Twig\Environment $env)
    {
        parent::__construct($env);
        $this->source = $this->getSourceContext();
        $this->parent = \false;
        $this->blocks = [];
        $macros["_self"] = $this->macros["_self"] = $this;
    }

The $macros variable does not exist resulting in:

Implicit array creation is not allowed - variable $macros does not exist.

/cc @fabpot Is this the best way to do it? Or would it be better to make this conditional instead?

$compiler
->addDebugInfo($this)
->write('$macros[')
->string($this->getAttribute('name'))
->raw('] = ')
;

When defining a macro in a template, the constructor looks like this:

```php
    public function __construct(\Twig\Environment $env)
    {
        parent::__construct($env);
        $this->source = $this->getSourceContext();
        $this->parent = \false;
        $this->blocks = [];
        $macros["_self"] = $this->macros["_self"] = $this;
    }
```

The `$macros` variable does not exist resulting in:

Implicit array creation is not allowed - variable $macros does not exist.
@@ -90,6 +90,7 @@ public function __construct(Environment \$env)
{
parent::__construct(\$env);

\$macros = \$this->macros;
Copy link
Contributor

Choose a reason for hiding this comment

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

As $macros is not used in the constructor, I'm not sure I understand what it does/fixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before this PR, the compiled constructor looked like this:

    public function __construct(\Twig\Environment $env)
    {
        parent::__construct($env);
        $this->source = $this->getSourceContext();
        $this->parent = \false;
        $this->blocks = [];
        $macros["_self"] = $this->macros["_self"] = $this;
    }

Look at the last line of the method. PHPStan reports this error:

Implicit array creation is not allowed - variable $macros does not exist.

It's because by default, $macros[$name] = is prepended in

$compiler
->addDebugInfo($this)
->write('$macros[')
->string($this->getAttribute('name'))
->raw('] = ')
;

So we have 2 options:

  1. add $macros = $this->macros; as the beginning of the constructor, just like doDisplay and block_ methods. Now PHPStan sees that $macros exists, and that adding something to it is fine.
  2. remove the $macros[$name] = prefix when inside the constructor

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel option 2 makes more sense, shall I try that?

Copy link
Contributor

Choose a reason for hiding this comment

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

This behavior comes from MacroAutoImportNodeVisitor, which has been removed in 3.15, so I don't think there is anything to fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch... then I'll wait for 3.15 to be released. Hopefully this can be done with #4415 included 🙏

@ruudk
Copy link
Contributor Author

ruudk commented Oct 25, 2024

Closing as this will be solved with the release of 3.15 that removes MacroAutoImportNodeVisitor

@ruudk ruudk closed this Oct 25, 2024
@ruudk ruudk deleted the add-missing-macros-variable branch October 25, 2024 06:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants