mglaman / phpstan-drupal

Extension for PHPStan to allow analysis of Drupal code.
https://phpstan-drupal.mglaman.dev/
MIT License
195 stars 76 forks source link

Unable to detect #pre_render callback with dynamic class #457

Open plopesc opened 2 years ago

plopesc commented 2 years ago

Hello, This one is coming from https://www.drupal.org/project/drupal/issues/3265408

If I'm not mistaken, https://github.com/mglaman/phpstan-drupal/blob/main/src/Rules/Drupal/RenderCallbackRule.php is unable to detect whether the #pre_render callable class uses a trait.

In the issue above MR, the referenced class (Radios or Checkboxes) uses the CompositeFormElementTrait and the preRenderCompositeFormElement is called as expected. However, the PHPStan check is telling that the method is not callable.

I'm newbie on PHPStan developemtn, but if you have any clue about how to try to solve this issue, I'm happy to help to solve it.

Thank you

mglaman commented 2 years ago

Oh weird.

153    #pre_render callback                                                   
         array{'Drupal\\Core\\Render\\Element\\Checkboxes'|'Drupal\\Core\\Rend  
         er\\Element\\Radios', 'preRenderCompositeF…'} at key '0' is not        
         callable.

Offending line

'#pre_render' => [
          [$class, 'preRenderCompositeFormElement'],

And the $class comes from

[$class, $type] = $element['#multiple'] ? [Checkboxes::class, 'checkboxes'] : [Radios::class, 'radios']; 

$callback = $element['#multiple'] ? [$class, 'processCheckboxes'] : [$class, 'processRadios'];

I'd have no idea until we added a test fixture and ran through the test failure with Xdebug to see how the code is being interpreted.

Currently I don't have docs on adding test fixtures, but I should get them added so it's easier to get a failing PR open for debugging.

plopesc commented 2 years ago

Hi @mglaman

I made some tests locally and the issue - apparently - is not related to the trait, but to the fact that fails when $class is a dynamic value.

Replacing the offending line by:

'#pre_render' => [
          [Checkboxes::class, 'preRenderCompositeFormElement'],

or

'#pre_render' => [
          [Radios::class, 'preRenderCompositeFormElement'],

it passes the PHPStan tests.

This issue is not a blocker anymore for the core issue mentioned given that I made a refactor to avoid the usage of that logic.

Thanks!

mglaman commented 2 years ago

Awesome! I'll keep this open, as I'm sure it will come up again. It's similar to #449. Where there is a string called $callable, but it's expected to be a callable-string. It needed a PHPStan stub.

mglaman commented 1 year ago

This seems worth adding a test case for