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

Call to an undefined method Form_5c1ccf4130219ae1b6d2423e3f6af9e6_image_meta|Nette\Forms\Controls\BaseControl::getLabel() #400

Open
lulco opened this issue Jul 19, 2023 · 9 comments

Comments

@lulco
Copy link
Contributor

lulco commented Jul 19, 2023

I don't know why and when, but sometimes we have this issue.

Form has container and some fields. Then there are some dynamic access to fields:

{input use_{$key}}

and phpstan latte thinks the input is contanier or base control, but why not all control classes used in dynamic return type of offsetGet method? Is it because BaseControl is parent of all of them? Need to check this

@lulco
Copy link
Contributor Author

lulco commented Jul 19, 2023

probably same issue here spaze/michalspacek.cz@2d7735e

@lulco
Copy link
Contributor Author

lulco commented Jul 22, 2023

It's because of dynamic name of key:
https://phpstan.org/r/9bbb3feb-b035-4641-929d-bfdb33d8e6fb

phpstan will not support this kind of types:
phpstan/phpstan#9658

we will try to solve this in our side

spaze added a commit to spaze/michalspacek.cz that referenced this issue Jul 22, 2023
efabrica-team/phpstan-latte#400 is probably better and more specific to this problem, add it too.
spaze added a commit to spaze/michalspacek.cz that referenced this issue Jul 22, 2023
efabrica-team/phpstan-latte#400 is probably better and more specific to this problem, add it too.
@lulco
Copy link
Contributor Author

lulco commented Jul 24, 2023

Idea - if component name is variable or property fetch etc., we can store it as string. The same name has to be used in latte.

For example:

$form->addButton($record->id);

This will be stored as name "$record->id" (as string, not value of $record->id)

In latte we have to use same name:

<button n:name="$record->id">

or

{input $record->id}

Point is, we can't use e.g. $myRecord->id in latte because it will not match.

What do you think @spaze @MartinMystikJonas?

@MartinMystikJonas
Copy link
Collaborator

This seems too much magic to me. And it would work only in cases where code exactly matches. In all cases when I use dynamic names it is in loops co it would not match. IMHO this is out of scope for static analysis.

@MartinMystikJonas
Copy link
Collaborator

IMHO best solution would be to force resolving of such types to generic BaseControl instead of union of all possible types. Best way to do it would be transform all dynamic fetches of form controls to fetch for some hardcoded placeholder like '*'.

@lulco
Copy link
Contributor Author

lulco commented Jul 25, 2023

This seems too much magic to me.

Could be documented :)

And it would work only in cases where code exactly matches. In all cases when I use dynamic names it is in loops co it would not match.

I disagree here, if you use e.g. foreach in php to define fields, you should access them same way in latte. For example if you use $record->id in php, you shouldn't use 123 in latte but again $record->id

IMHO this is out of scope for static analysis.

Yes, I'm just finding ways to cover as much edge cases as possible

IMHO best solution would be to force resolving of such types to generic BaseControl instead of union of all possible types.

This is done by phpstan itself. It unions BaseControl with Container because that is correct. We wrongly assumes BaseControl as default field type.

Best way to do it would be transform all dynamic fetches of form controls to fetch for some hardcoded placeholder like '*'.

This seems like much more magic to me and cause a lot of false positives / negatives.

@MartinMystikJonas
Copy link
Collaborator

We assume BaseControl because Container would not make sense to be used in {input}

@lulco
Copy link
Contributor Author

lulco commented Jul 25, 2023

We assume BaseControl because Container would not make sense to be used in {input}

That’s true, but compiled code is similar for input and container I think. We have to differ between them.

@lulco
Copy link
Contributor Author

lulco commented Jul 28, 2023

@MartinMystikJonas @spaze I tried to implement my idea as feature flag here #418 feel free to test it.

For now only Variable and Property fetch nodes are supported, but we can add more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants