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

Remove and update extensions to reflect new stub file #182

Merged
merged 31 commits into from
Jul 27, 2023

Conversation

IanDelMar
Copy link
Contributor

@IanDelMar IanDelMar commented Apr 16, 2023

Removes the following extensions/rules, which are now fully covered by the new (yet unreleased) stub file:

  • GetTaxonomiesDynamicFunctionReturnTypeExtension
  • GetObjectTaxonomiesDynamicFunctionReturnTypeExtension
  • HasFilterDynamicFunctionReturnTypeExtension
  • MySQL2DateDynamicFunctionReturnTypeExtension
  • CurrentTimeDynamicFunctionReturnTypeExtension
  • WpThemeGetDynamicMethodReturnTypeExtension
  • GetPermalinkDynamicFunctionReturnTypeExtension
  • WPErrorParameterDynamicFunctionReturnTypeExtension
  • IsWpErrorFunctionTypeSpecifyingExtension
  • IsWpErrorRule
  • EchoParameterDynamicFunctionReturnTypeExtension

Also removes the tests as they should be part of php-stubs/wordpress-stubs itself.

Updates and merges the following extension, which are partly (though almost fully) covered by the new (yet unreleased) stub file:

  • GetCommentDynamicFunctionReturnTypeExtension
  • GetPostDynamicFunctionReturnTypeExtension

Also removes the tests not related to the return type extension as they should be part of php-stubs/wordpress-stubs itself.

Part of #181

For testing purposes I recommend to

    "require": {
        "php": "^7.2 || ^8.0",
-        "php-stubs/wordpress-stubs": "^4.7 || ^5.0 || ^6.0",
+        "php-stubs/wordpress-stubs": "dev-master",
        "phpstan/phpstan": "^1.10.0",
        "symfony/polyfill-php73": "^1.12.0"
    },

in composer.json

@herndlm
Copy link
Contributor

herndlm commented Apr 16, 2023

Not sure about the test removal tbh, is it possible to add them to the stubs package or would they just be removed for now? The latter feels like a risky loss

Update: if tests should be moved over there, which is understandable - maybe by requiring phpstan there too with the extension if needed?

@johnbillion
Copy link
Contributor

Yeah loss of the tests is very unfortunate. Without the return type extension classes, I'm not sure how you'd test the behaviour.

@szepeviktor
Copy link
Owner

What wonder do we have here?? :)

@herndlm
Copy link
Contributor

herndlm commented Apr 16, 2023

What about dev-requiring phpstan + extension in the stubs repo and making composer aware of the fact that it should use the local stubs checkout instead of the latest release. And then we essentially "just" move the tests over. In theory at least. I can give it a try

@szepeviktor
Copy link
Owner

szepeviktor commented Apr 16, 2023

I have no clue how to manage this dependency problem. For me it takes a couple of months to research.
🤔 Monorepo?

Let's leave tests here until a robust plan comes up.

@IanDelMar
Copy link
Contributor Author

I find it weird to test something that another repository does.

@szepeviktor
Copy link
Owner

szepeviktor commented Apr 16, 2023

The 1st obstacle: How to send a PR that fixes something in the PHPStan extension and in core stubs generation?

@herndlm
Copy link
Contributor

herndlm commented Apr 16, 2023

I did something in php-stubs/wordpress-stubs#76, it didn't work as I initially planned, but it should be even a bit simpler

@szepeviktor szepeviktor changed the base branch from master to 2.x April 18, 2023 23:06
@IanDelMar
Copy link
Contributor Author

IanDelMar commented Apr 19, 2023

Thanks to @herndlm's test setup all tests have been moved to php-stubs/wordpress-stubs.

@IanDelMar
Copy link
Contributor Author

@szepeviktor I removed all but one function from the $wp_error parameter extension. _set_cron_array() is left. This function is marked private by WP. It should not be used and there should not need a dynamic return type extension. What do you think?

@szepeviktor
Copy link
Owner

Please remove that private function too.

@szepeviktor
Copy link
Owner

szepeviktor commented Apr 20, 2023

The next logical step would be to exclude all these __* functions from WP stubs, but I am sure removing them will set up a bomb.

@IanDelMar
Copy link
Contributor Author

The next logical step would be to exclude all these __* functions from WP stubs,

Do you mean _* or __*?

@szepeviktor
Copy link
Owner

szepeviktor commented Apr 20, 2023

Touching anything is WP core is a no-go.
Or else we have to work 1000+ hours.

_*, __* was a typo

@szepeviktor
Copy link
Owner

🎈 Fun fact: there are 462 \sfunction _[^_] functions/methods.

@IanDelMar
Copy link
Contributor Author

What are AssertNotWpErrorTypeSpecifyingExtension and AssertWpErrorTypeSpecifyingExtension for?

README.md Outdated Show resolved Hide resolved
@szepeviktor
Copy link
Owner

❓ Will WordPress 7.0 need no viktorstan to be statically analyzed?

@IanDelMar
Copy link
Contributor Author

IanDelMar commented Apr 23, 2023

Status update

Extensions to check

  • WpThemeMagicPropertiesClassReflectionExtension
  • ShortcodeAttsDynamicFunctionReturnTypeExtension

Extensions to keep

  • ApplyFiltersDynamicFunctionReturnTypeExtension
  • EchoKeyDynamicFunctionReturnTypeExtension
  • StringOrArrayDynamicFunctionReturnTypeExtension
  • GetTermsDynamicFunctionReturnTypeExtension
  • GetPostDynamicFunctionReturnTypeExtension
  • GetPostsDynamicFunctionReturnTypeExtension
  • GetSitesDynamicFunctionReturnTypeExtension
  • WpParseUrlFunctionDynamicReturnTypeExtension
  • AssertWpErrorTypeSpecifyingExtension
  • AssertNotWpErrorTypeSpecifyingExtension

Extensions to discuss

  • GetListTableDynamicFunctionReturnTypeExtension
  • RedirectCanonicalDynamicFunctionReturnTypeExtension

GetListTableDynamicFunctionReturnTypeExtension

Here's what WP says about _get_list_table(): "This is a private function, however, and should not be used by developers." see here. I suggest to remove the extension.

RedirectCanonicalDynamicFunctionReturnTypeExtension

This extension fixes redirect_canonical() which according to WP returns string|void by letting it return string|null instead ignoring the return;. This should be fixed in core not in an extension. The same goes for functions handled via the echo parameter (already moved to wordpress-stubs) and echo key extension and surely for many more core function that are not covered by phpstan-wordpress and wordpress-stubs. By returning null instead of void RedirectCanonicalDynamicFunctionReturnTypeExtension treats this differently than the echo extension. I think how this is treated should be consistent across extension and I suggest to remove RedirectCanonicalDynamicFunctionReturnTypeExtension.

@swissspidy
Copy link
Contributor

What are AssertNotWpErrorTypeSpecifyingExtension and AssertWpErrorTypeSpecifyingExtension for?

See #124 / #30

@IanDelMar
Copy link
Contributor Author

Thank you @swissspidy

@szepeviktor
Copy link
Owner

@IanDelMar Should I cherry-pick #183 #184 and #185 into 2.x branch?

@szepeviktor
Copy link
Owner

szepeviktor commented Jul 27, 2023

image

@IanDelMar I think this PR is complete.

Do we still need "php-stubs/wordpress-stubs": "dev-master"?

@IanDelMar
Copy link
Contributor Author

I think this PR is complete.

I agree with it.

Do we still need "php-stubs/wordpress-stubs": "dev-master"?

Don't think so.

@szepeviktor szepeviktor merged commit 3fec21b into szepeviktor:2.x Jul 27, 2023
@IanDelMar IanDelMar deleted the remove-stubfile-fns branch July 27, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants