psalm / psalm-plugin-wordpress

WordPress stubs and plugin for Psalm
MIT License
69 stars 18 forks source link

Dynamic hooks trigger `HookNotFound` #12

Closed iandunn closed 8 months ago

iandunn commented 3 years ago

Hi, it looks like dynamic hooks aren't supported yet. e.g., plugin_action_links_{$plugin_file}:

ERROR: HookNotFound - ../../../../../private/tmp/plugin-basic-google-maps-placemarksqDa7oc/settings.php:31:4 - Hook plugin_action_links_basic-google-maps-placemarks/basic-google-maps-placemarks.php not found.

add_filter( 'plugin_action_links_basic-google-maps-placemarks/basic-google-maps-placemarks.php', array( $this, 'addSettingsLink' ) );

https://github.com/humanmade/psalm-plugin-wordpress/blob/8fe21386fb4ff63d4253322dc3b8eaa1bb71c99f/Plugin.php#L245

I don't have any thoughts for an elegant solution at the moment, but I'll think about it some more. Worst case, a brute-force approach would probably work, but might have performance issues.

P.S. Thanks for creating this plugin, it's super helpful!

sanderdekroon commented 3 years ago

To add to this: is there any way to manually add a few hooks? I'm running into an issue where I'm using two Gravity Forms hooks, but I am unable to make Psalm aware of those hooks. I don't see a way to add one or two hook definitions, but maybe I'm overlooking something :)

joehoyle commented 3 years ago

Hmm yes I had noticed this too. I think we could add support by somehow converting the {$var} to a regex, and then matching hook names against regexs. That could be a bit slow, but I think it could work. It would of course then need to assume that plugin_action_links_not_a_real_plugin was a valid action.

joehoyle commented 3 years ago

@sanderdekroon you should be able to add a Psalm stub file, with the apply_filters / do_action call in it, which I think will get scanned, and then added to the hook index. If not, we should fix that.

sanderdekroon commented 3 years ago

Could you perhaps explain how to add the apply_filters call?

I added apply_filters('gform_save_field_value', 'foo', []); as a test to my stub file, but that does not seem to fix anything. Psalm then still returns this error:

ERROR: HookNotFound - src/GravityForms.php:20:9 - Hook gform_save_field_value not found.
        add_filter('gform_save_field_value', [$this, 'encryptFieldValue'], 10);
joehoyle commented 3 years ago

Hrm yeah I discovered scanning is not hookable by plugins, so putting them in a stub file actually won't be analyzed and therefore trigger the hook population code. I created https://github.com/vimeo/psalm/issues/5880 to see if there's a better way

kkmuffme commented 2 years ago

~~Adding onto this: since there is https://github.com/johnbillion/wp-hooks-generator, anybody can just create the stub hooks for whatever plugins one needs. Then https://github.com/humanmade/psalm-plugin-wordpress/blob/master/Plugin.php#L109 needs to be a flexible, so we can load these custom action/filter stubs.~~

Not sure if psalm plugins can read from the psalm config file, but if not, we could just read a custom config file, where you can provide paths to additional actions/filters.jsons?

Implemented in https://github.com/humanmade/psalm-plugin-wordpress/issues/28

joehoyle commented 2 years ago

Hmm yeah until Psalm supports plugins to process scanned files (which stubs are a part of) we might need to add a specific config / stub option for this plugin, which is can scan when populating all hooks. I think the better way to do that would be in PHP rather than a JSON file though, so you just do it like a stub file

kkmuffme commented 2 years ago

Found a method to implement this :-) Works for all dynamic actions/filters on a regex basis. I didn't get any false positives for all cases I tested in a huge codebase.

Does not work for fully dynamic hooks like {$tag} or {$tag}_{$hello}.

Performance impact is basically none (in the ~5ms range/file)

Just waiting for the pending PRs to be merged, then will add PR for it.

kkmuffme commented 9 months ago

@iandunn can you check if the way it works in the newly released v3 works for your use case?