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

{include}'d template ignores that {include} params may be different #391

Closed
spaze opened this issue Jul 8, 2023 · 8 comments · Fixed by #392
Closed

{include}'d template ignores that {include} params may be different #391

spaze opened this issue Jul 8, 2023 · 8 comments · Fixed by #392

Comments

@spaze
Copy link
Contributor

spaze commented Jul 8, 2023

I have a template with

	{if $order == 'desc'}  // <-- line 40
		{var $dateLineCheck = $now && $training->getStart() < $now}  // <-- line 41

(source)

The template is included in some other templates.
For example like this:

{include "common/dateList.latte", order: 'desc', upcomingIds: $upcomingIds, now: $now}

(source)
or like this:

    {include "common/dateList.latte", order: 'desc', upcomingIds: [], now: null}

(source)
note the difference now: null vs. now: $now (where $now is created as $this->template->now = new DateTime();)

In the first case the error message says

 ------ ---------- -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   Compiled   Admin/Presenters/templates/Trainings/common/dateList.latte rendered from MichalSpacekCz\Admin\Presenters\TrainingsPresenter::default included in Admin/Presenters/templates/Trainings/default.latte
         line       See compiled template: /tmp/phpstan-latte/app/Admin/Presenters/templates/Trainings/common/dateList.latte.7844c3b4b8a153b2b6150427a6cbf075.php
 ------ ---------- -----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  40     74         If condition is always true.
  41     76         Left side of && is always true.

and in the second

 ------ ---------- -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   Compiled   Admin/Presenters/templates/Trainings/common/dateList.latte rendered from MichalSpacekCz\Admin\Presenters\TrainingsPresenter::pastWithPersonalData included in Admin/Presenters/templates/Trainings/pastWithPersonalData.latte
         line       See compiled template: /tmp/phpstan-latte/app/Admin/Presenters/templates/Trainings/common/dateList.latte.57d7b0777d244e57e808707cb870ee8e.php
 ------ ---------- -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  40     74         If condition is always true.
  41     76         Left side of && is always false.

(order: 'desc' on line 40 is changed in some other {include} line to order: 'asc')

In both cases the template is compiled to:

            /* line 40 */
            if ($order == 'desc') {
                /* line 41 */
                $dateLineCheck = $now && $training->getStart() < $now;

but in the first case the compiled source contains:

        /** @var DateTime $now */
        /** @var array<int, int> $upcomingIds */
        /** @var 'desc' $order */

and in the second case:

        /** @var 'desc' $order */
        /** @var array{} $upcomingIds */
        /** @var null $now */

which is correct only in the isolated case and not when the template is reused and the parameter may be different.

I don't know what to do with that, if it's a missing feature or a bug, or if I should refactor my code.

@lulco
Copy link
Contributor

lulco commented Jul 8, 2023

We have to check this. Maybe @MartinMystikJonas will know more about it.

@MartinMystikJonas
Copy link
Collaborator

I have few similar cases in our code too.

It is one of limitations because we always analyse (included) template with given combination of passed parameters.

If we would merge parameters from different calls/includes it would cause even more weird false-positives. Because it would check invalid combinations. Like some bool flag deciding if render given part of template but with null values for data rendered in that part.

I solve it by ignoring these errors.

@spaze
Copy link
Contributor Author

spaze commented Jul 8, 2023

Thanks, I could probably change that and move the variables to be set in presenters so there are no foo: null in {include}s but it doesn't seem to solve the error because I think it's the same or similar as in #390.

@lulco
Copy link
Contributor

lulco commented Jul 8, 2023

Yes it is kind of similar issue. Here you should somehow use ?DateTime type for variable $now. But I understand it could be a lot of rewrite

@lulco
Copy link
Contributor

lulco commented Jul 8, 2023

Also I think some shared templates are large and contains more parts - each somehow conditionally displayed. So very often you have to use some flags (true/false) od null for nullable types. It would probably work neter (for phpstan latte) if you split those shared templates to multiple smaller templates and include only those you need.

@spaze
Copy link
Contributor Author

spaze commented Jul 8, 2023

Also I think some shared templates are large and contains more parts

That's totally possible :-) Do you have an example which template you think it's way too huge?

@lulco
Copy link
Contributor

lulco commented Jul 8, 2023

I didn't work with your app for a while so I don’t remember. I just had that feeling

@spaze
Copy link
Contributor Author

spaze commented Jul 9, 2023

I think this can also be closed by #392, it's a similar issue as #390 anyway.

@lulco lulco closed this as completed in #392 Jul 9, 2023
spaze added a commit to spaze/michalspacek.cz that referenced this issue Jul 10, 2023
…ponent

From 94 level 4 errors to 66 with just this change.

This is probably how efabrica-team/phpstan-latte#391 should be solved, components suggested in #143 (comment)

Docs
https://doc.nette.org/cs/application/components
https://doc.nette.org/en/application/creating-links#toc-links-in-component for {plink}

The UiControl class is there mostly, I mean only to suppress PhpStorm's "Member has private visibility but can be accessed via '__get' magic method" which is thrown in child classes of Nette\Application\UI\Control even though it has the same `@property-read` tag.
spaze added a commit to spaze/michalspacek.cz that referenced this issue Jul 14, 2023
…ponent

From 94 level 4 errors to 66 with just this change.

This is probably how efabrica-team/phpstan-latte#391 should be solved, components suggested in #143 (comment)

Docs
https://doc.nette.org/cs/application/components
https://doc.nette.org/en/application/creating-links#toc-links-in-component for {plink}

The UiControl class is there mostly, I mean only to suppress PhpStorm's "Member has private visibility but can be accessed via '__get' magic method" which is thrown in child classes of Nette\Application\UI\Control even though it has the same `@property-read` tag.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants