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

Fix callable phpdoc for twig elements #3856

Merged
merged 1 commit into from
Jul 5, 2023
Merged

Fix callable phpdoc for twig elements #3856

merged 1 commit into from
Jul 5, 2023

Conversation

VincentLanglet
Copy link
Contributor

Hi @fabpot

[MyRuntime::class, 'myMethod]

is not a valid callable in PHP 8 https://3v4l.org/tWqPs

But this syntax is supported because twig has a special behavior

$compiler->raw(sprintf('$this->env->getRuntime(\'%s\')->%s', $callable[0], $callable[1]));

The PHPDoc should be updated in order to reflect the real behavior and solve static analysis errors.
A phpstan discussion can be found here: phpstan/phpstan#9567

@fabpot
Copy link
Contributor

fabpot commented Jul 5, 2023

Maybe that's something we want to deprecate?

@xabbuh
Copy link
Contributor

xabbuh commented Jul 5, 2023

@fabpot Isn't it a requirement for lazy loading?

@stof
Copy link
Member

stof commented Jul 5, 2023

Why would we want to deprecate it ? this is what supports the runtime feature, enabling lazy-loading.

@jordisala1991
Copy link

Are you sure is a class-string and not a service id?

@fabpot
Copy link
Contributor

fabpot commented Jul 5, 2023

Thank you @VincentLanglet.

@fabpot fabpot merged commit e145d36 into twigphp:3.x Jul 5, 2023
@VincentLanglet
Copy link
Contributor Author

Are you sure is a class-string and not a service id?

Yes @jordisala1991, the signature is
´´´
public function getRuntime(string $class)
´´´

@VincentLanglet
Copy link
Contributor Author

@fabpot Thanks for the merge.

Currently, all phpstan-bleeding-edge users will have static analysis build failing since this PR is currently not release.
I was wondering what is the twig planning for release, and if it was possible to make a patch release of twig 3.x if none was planned soon. Thanks :)

@fabpot
Copy link
Contributor

fabpot commented Jul 26, 2023

Sorry for the delay @VincentLanglet; this one fell through the cracks. I've just made a new release.

fabpot added a commit that referenced this pull request Jul 28, 2023
This PR was merged into the 3.x branch.

Discussion
----------

Fix callable phpdoc for twig elements

Hi `@fabpot`
I made a stupid mistake in #3856

The correct phpdoc is `array{class-string, string}` and not `array<class-string, string>`.

Code is used this way
```
$compiler->raw(sprintf('$this->env->getRuntime(\'%s\')->%s', $callable[0], $callable[1]));
```
which means the key 0 is a class string, and the key 1 is a string.

`array{class-string, string}` is same as `array{0: class-string, 1: string}`
`array<class-string, string>` is saying all the keys are class-string, and values are string.

Commits
-------

1b58589 Fix callable phpdoc
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.

5 participants