psalm / psalm-plugin-wordpress

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

Update stub overrides #44

Closed mcaskill closed 1 year ago

mcaskill commented 1 year ago

Updated overrides to reflect php-stubs/wordpress-stubs v6.1.0.

Changed:

Resolves #38

kkmuffme commented 1 year ago

All calculated values in stubs (wordpress-globals) do not work in psalm, since psalm will only read, but not evaluate the file. That's the reason why HOUR_IN_SECONDS was needed (and the same issue is true for all) To fix this, one has to put the wordpress-globals stub file as an autoloader for psalm (not sure if a plugin can do that? Please check) in the config file

mcaskill commented 1 year ago

Thanks for the context. I was having trouble figuring out that information from the commits and just assumed it was like that because of older versions of Psalm and the WordPress stubs.

mcaskill commented 1 year ago

All calculated values in stubs (wordpress-globals) do not work in psalm, since psalm will only read, but not evaluate the file.

That much I've come to understand from Psalm and the like. Which is what led me to adding DirnameReturnTypeProvider in order to evaluate/resolve those literal strings.

That's the reason why HOUR_IN_SECONDS was needed (and the same issue is true for all)

This part I'm not sure I understand. Is the issue that 60 * MINUTE_IN_SECONDS is unresolvable by Psalm and that's why the overridden constant is using an integer?

To fix this, one has to put the wordpress-globals stub file as an autoloader for psalm (not sure if a plugin can do that? Please check) in the config file

As far as I tell from vimeo/psalm, the autoloader mechanism requires the given file (evaluating it) and collects any includes (adding them to the deep scan in the same way as stubs).

We could include the php-stubs/wordpress-globals file from the Plugin's entry point method. We would add a means to disable this inclusion in case the project is already autoloading them (in the same vein as useDefaultStubs and useDefaultHooks).

kkmuffme commented 1 year ago

This part I'm not sure I understand. Is the issue that 60 * MINUTE_IN_SECONDS is unresolvable by Psalm and that's why the overridden constant is using an integer?

60 * MINUTE_IN_SECONDS needs to be evaluated by PHP to get 3600. But since stubs are not evaluated, psalm just reads it as "60 * MINUTE_IN_SECONDS- since this is unknown, psalm just assumes typemixed` for it.

For some reason only this constant was overridden previously, but technically all constants that are evaluatead values must be a hardcoded value for them to work (and not be mixed) as stubs in psalm.

As far as I tell from vimeo/psalm, the autoloader mechanism requires the given file (evaluating it) and collects any includes (adding them to the deep scan in the same way as stubs). We could include the php-stubs/wordpress-globals file from the Plugin's entry point method. We would add a means to disable this inclusion in case the project is already autoloading them (in the same vein as useDefaultStubs and useDefaultHooks).

Sounds good, just try if that works. One major issue is also that evaluated values might report tons of false positives, so what I did is: (not in the plugin, in our psalm config):

define( 'EMPTY_TRASH_DAYS', /** @var int<0, max> $empty_trash_days_stub */ $empty_trash_days_stub = 30 );
kkmuffme commented 1 year ago

@mcaskill fyi I have almost completely overwritten the globals stubs now already to make them work with psalm correctly as well as handle some "special" behavior in some edge cases - I could PR that.

I think regarding the "autoloader": if we just require_once the globals file in the plugin, this should have the same effect as autoloader setting in the config. Can you perhaps test that once? Bc if that works we'll go with that, merge this PR and I PR the globals. Then we finally release the v4 version now that psalm v5 is available

EDIT: or should this be for v5?