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

Adjust logic targeting inaccessible nodes #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

the-csaba
Copy link

@the-csaba the-csaba commented Apr 12, 2023

Changes

  • Loosen parent check from Class_ to ClassLike to include Traits, Interfaces, and Enums.
  • Remove protected nodes, regardless if the class is final or not.
  • Adjust test

Reason

Unfortunately, sometimes developers define protected or private methods in interfaces or traits.

I'm looking at you WooCommerce:

Fatal error: Class WC_Cart_Totals contains 1 abstract method and must therefore be declared abstract or implement the remaining methods (WC_Cart_Totals::get_values_for_total) in woocommerce-stubs.php on line 15030

The missing abstract method (get_values_for_total) is defined in a trait:
https://github.com/woocommerce/woocommerce/blob/c4a7e9a11b62e9f62fd507f5a0c8ae09740619a0/plugins/woocommerce/includes/traits/trait-wc-item-totals.php#L32

Loosen parent check from Class_ to ClassLike, to include Traits, Interfaces, and Enums.

Remove protected nodes, regardless if the class is final or not.
Adjust test
Comment on lines +247 to +251
if (!$this->includeInaccessibleClassNodes
&& $parent instanceof Class_
&& ($node instanceof ClassMethod || $node instanceof ClassConst || $node instanceof Property)
&& ($node->isPrivate() || $node->isProtected())
) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!$this->includeInaccessibleClassNodes
&& $parent instanceof Class_
&& ($node instanceof ClassMethod || $node instanceof ClassConst || $node instanceof Property)
&& ($node->isPrivate() || $node->isProtected())
) {
if (
!$this->includeInaccessibleClassNodes
&& $parent instanceof Class_
&& ($node instanceof ClassMethod || $node instanceof ClassConst || $node instanceof Property)
&& ($node->isPrivate() || $node->isProtected())
) {

@szepeviktor
Copy link
Member

szepeviktor commented Apr 12, 2023

Thank you for your contribution!

I've given up on WC and disabled this feature, thus including inaccessible things.

Could WC stubs be executed with this PR?
https://github.com/php-stubs/woocommerce-stubs/blob/8f81c28808986d63aeb627692e17b5b7880913ab/.travis.yml#L39

@szepeviktor
Copy link
Member

Remove protected nodes, regardless if the class is final or not.

What is someone extends a WC class with protected methods? Will this introduce static analysis errors?

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.

2 participants