efabrica-team / phpstan-latte

PHPStan extension to check compiled latte templates
MIT License
43 stars 4 forks source link

Undefined variable/Variable is never defined after upgrading to 0.14.0 #415

Closed spaze closed 1 year ago

spaze commented 1 year ago

The title may be misleading, sorry, because the variable is not present in older compiled files either, but I don't know what's going on to create a better title πŸ˜‡ I have read about the "Undefined variable" bc break in the release notes but bear with me.

I have a template

{contentType text/plain}
{ifset $ssid}
# {$ssid}
{/ifset}
...

with this relevant PHP code snippet:

        $this->template->ssid = $this->ssid;
        $this->template->error = $error ?? null;
        $this->template->keys = isset($keys) && !isset($error) ? $keys : null;
        switch ($format) {
            case 'text':
                $this->setView('text');
                break;
            case 'html':
                $this->template->filterTypes = WiFiBand::getKnown();
                $this->template->modelsWithPrefixes = $this->upcKeys->getModelsWithPrefixes();
                $this->template->prefixes = $this->upcKeys->getPrefixes();
                $this->template->placeholder = $this->upcKeys->getSsidPlaceholder();
                break;
            case 'json':
                $this->sendJson(array_filter([
                    'ssid' => $this->ssid,
                    'error' => $error ?? null,
                    'keys' => $keys ?? null,
                ]));
                // no break, Presenter::sendJson() is in earlyTerminatingMethodCalls defined in the phpstan-nette extension config
            default:
                throw new BadRequestException('Unknown format');
        }

After analyzing it with the newly released 0.14.0 I'm now getting:

 ------ ---------- -------------------------------------------------------------------------------------------------------------------------------------
  Line   Compiled   UpcKeys/Presenters/templates/Homepage/text.latte rendered from MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter
         line       See compiled template: /tmp/phpstan-latte/app/UpcKeys/Presenters/templates/Homepage/text.latte.5a750619f6c72ef76d6013210abd4b2c.php
 ------ ---------- -------------------------------------------------------------------------------------------------------------------------------------
  2      34         Variable $ssid in isset() is never defined.
  3      37         Undefined variable: $ssid
  5      41         Variable $error in isset() is never defined.
  6      44         Undefined variable: $error
  8      48         Variable $keys in isset() is never defined.
  10     50         Undefined variable: $keys
 ------ ---------- -------------------------------------------------------------------------------------------------------------------------------------

This has worked before with 0.13.2 (and generally works correctly as the template and the PHP code has not been touched for at least three months).

This is the compiled template: ```php params['filters']; extract($__filters__); /** @var array{baseUrl: string, basePath: string, ʟ_fi: Latte\Runtime\FilterInfo, ʟ_tag: array, ʟ_if: array, ʟ_ifc: array, ʟ_try: array, ʟ_loc: array, ʟ_tmp: mixed, ʟ_input: Nette\Forms\Controls\BaseControl, ʟ_label: (Nette\Utils\Html | string | null), user: Nette\Security\User, localeLinks: (array | null), presenter: MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter, control: MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter, flashes: array} $__variables__ */ $__variables__ = $this->params['variables']; \extract($__variables__); if ($this->getReferringTemplate() === \null || $this->getReferenceType() === 'extends') { foreach (\array_intersect_key(['key' => '10'], $this->params) as $ʟ_v => $ʟ_l) { \trigger_error("Variable \${$ʟ_v} overwritten in foreach on line {$ʟ_l}"); } } if (empty($this->global->coreCaptured) && \in_array($this->getReferenceType(), ['extends', \null], \true)) { /* line 1 */ \header('Content-Type: text/plain'); } unset($ʟ_args); if ($this->global->snippetDriver->renderSnippets($this->blocks[self::LayerSnippet], $this->params)) { return; } /* line 2 */ if (isset($ssid)) { echo '# '; /* line 3 */ echo \Latte\Runtime\Filters::nop($ssid); echo "\n"; } ```

I have noticed it says HomepagePresenter without any action both in the compiled template and in the error message.


There's also another compiled file in the temp dir that starts like this: ```php params['filters']; extract($__filters__); /** @var array{baseUrl: string, basePath: string, ʟ_fi: Latte\Runtime\FilterInfo, ʟ_tag: array, ʟ_if: array, ʟ_ifc: array, ʟ_try: array, ʟ_loc: array, ʟ_tmp: mixed, ʟ_input: Nette\Forms\Controls\BaseControl, ʟ_label: (Nette\Utils\Html | string | null), user: Nette\Security\User, localeLinks: (array | null), presenter: MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter, control: MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter, flashes: array, ssid: (string | null), error: ('Oops, something went wrong, please try again in a moment' | 'Wi-Fi network name is not "UPC" and 7 numbers, the password cannot be recovered by this tool' | null), keys: (array | null), filterTypes: array, modelsWithPrefixes: array>, prefixes: list, placeholder: string} $__variables__ */ $__variables__ = $this->params['variables']; \extract($__variables__); if ($this->getReferringTemplate() === \null || $this->getReferenceType() === 'extends') { foreach (\array_intersect_key(['key' => '10'], $this->params) as $ʟ_v => $ʟ_l) { \trigger_error("Variable \${$ʟ_v} overwritten in foreach on line {$ʟ_l}"); } } if (empty($this->global->coreCaptured) && \in_array($this->getReferenceType(), ['extends', \null], \true)) { /* line 1 */ \header('Content-Type: text/plain'); } unset($ʟ_args); if ($this->global->snippetDriver->renderSnippets($this->blocks[self::LayerSnippet], $this->params)) { return; } /* line 2 */ if (isset($ssid)) { echo '# '; /* line 3 */ echo \Latte\Runtime\Filters::nop($ssid); echo "\n"; } /* line 5 */ ```

notice how now it says "HomepagePresenter::default(text)" (and that it also has the ssid variable somewhere in the @var block.


The temp dir still has some files compiled 3 days ago with the previous version: ```php params); /** @var Nette\Localization\Translator $__filter__translate */ /** @var Closure(mixed, mixed, mixed=): mixed $__filter__modifyDate */ /** @var MichalSpacekCz\Templating\Filters $__filter__staticUrl */ /** @var MichalSpacekCz\Templating\Filters $__filter__staticImageUrl */ /** @var MichalSpacekCz\Templating\Filters $__filter__format */ /** @var MichalSpacekCz\DateTime\DateTimeFormatter $__filter__localeDay */ /** @var MichalSpacekCz\DateTime\DateTimeFormatter $__filter__localeMonth */ /** @var MichalSpacekCz\DateTime\DateTimeFormatter $__filter__localeIntervalDay */ /** @var MichalSpacekCz\DateTime\DateTimeFormatter $__filter__localeIntervalMonth */ $__filters__ = $this->filters->getAll(); /** @var string $baseUrl */ /** @var string $basePath */ /** @var Latte\Runtime\FilterInfo $ʟ_fi */ /** @var array $ʟ_tag */ /** @var array $ʟ_if */ /** @var array $ʟ_ifc */ /** @var array $ʟ_try */ /** @var array $ʟ_loc */ /** @var mixed $ʟ_tmp */ /** @var Nette\Forms\Controls\BaseControl $ʟ_input */ /** @var Nette\Utils\Html|string|null $ʟ_label */ /** @var Nette\Security\User $user */ /** @var array|null $localeLinks */ /** @var MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter $presenter */ /** @var MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter $control */ /** @var array $flashes */ \reset($this->params); if ($this->getReferringTemplate() === \null || $this->getReferenceType() === 'extends') { foreach (\array_intersect_key(['key' => '10'], $this->params) as $ʟ_v => $ʟ_l) { \trigger_error("Variable \${$ʟ_v} overwritten in foreach on line {$ʟ_l}"); } } if (empty($this->global->coreCaptured) && \in_array($this->getReferenceType(), ['extends', \null], \true)) { /* line 1 */ \header('Content-Type: text/plain'); } unset($ʟ_args); if ($this->global->snippetDriver->renderSnippets($this->blocks[self::LayerSnippet], $this->params)) { return; } /* line 2 */ if (isset($ssid)) { echo '# '; /* line 3 */ echo \Latte\Runtime\Filters::nop($ssid); echo "\n"; ```

Notice it also doesn't have any action listed. Also, there are no files neither for default(json) nor for default(html) case, the former may be because there's $this->sendJson() and it doesn't use any template Seems the value in the parentheses doesn't mean what I thought it means, sorry. Seems it's a template/view name, not action params.

There are files only for (taken from the compiled file docblock):

Not sure what's going on here :-)

MartinMystikJonas commented 1 year ago

Because you have template text.latte your presenter also have implicit action Homepage:text that could be invoked. Existence of actionText/renderText methods is not reguired by Nette. And call to this action would cause error in latte.

I would recommend moving setting template parameters to renderDefault/renderText or beforeRender method.

lulco commented 1 year ago

I tried to documented this case here - point 2.

I'm just not sure why it wasn't reported before. Maybe because of old extract($this->params); call which marks all variables as mixed might not be defined. Now only some variables exists - those which are in array shape. So all other variables are undefined.

MartinMystikJonas commented 1 year ago

Yeah with new version it reports bugs where isset is used with not existing variable. It found few bugs in our code where we checked for wrong variable name in isset. In old version with generic extract any variable could exists so PHPStan ignored these types of errors

lulco commented 1 year ago

I've created example in phpstan playground: https://phpstan.org/r/5947bfc6-71b1-4deb-ae50-ce0df0349c90

I'm closing this as answered. Try to fix in your code or ignore as false positives if you wish.

spaze commented 1 year ago

Because you have template text.latte your presenter also have implicit action Homepage:text that could be invoked. Existence of actionText/renderText methods is not reguired by Nette. And call to this action would cause error in latte.

Oh. OH. OOOOHHHH! πŸ˜… Okay, thanks, that makes sense. I wasn't aware of that behavior even though I've read Nette docs where they mention it, and probably more than once.

Yeah with new version it reports bugs where isset is used with not existing variable.

This is nice and makes sense too and I'm happy it reports it. (Or maybe not, if I have to rewrite something πŸ˜…)

Thanks!

(Have refactored it in spaze/michalspacek.cz#189 to use a service method only to generate a plain text string, not a simple template which now seems like an overkill anyway.)

spaze commented 1 year ago

I was just thinking, could it say something like "rendered from MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter(no action method)" or maybe display an "πŸ’‘ error tip Missing action/render, but variable is used in template" and maybe even point to the docs if there's an undefined variable and no action/render method? That would help a lot to at least point me to the right direction.

lulco commented 1 year ago

I was just thinking, could it say something like "rendered from MichalSpacekCz\UpcKeys\Presenters\HomepagePresenter(no action method)" or maybe display an "πŸ’‘ error tip Missing action/render, but variable is used in template" and maybe even point to the docs if there's an undefined variable and no action/render method? That would help a lot to at least point me to the right direction.

I think can be added just in top ”table” where Presenter and action is printed, not as tip for error. We can try something.

MartinMystikJonas commented 1 year ago

AbstractClassStandaloneTemplateResolver now sets name of action to null, we can replace it with template name (+ hint? ) to get something like "AbcPresenter::text.latte(standalone template)"

spaze commented 1 year ago

The error tip with a link would be a nice addition too. The documentation exists, so let's send people there :-)

lulco commented 1 year ago

I'm not able to make it better for now :)

image