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

[WP 5.3] Declaration should be compatible with Walker::walk #448

Closed
Chouby opened this issue Oct 15, 2019 · 6 comments · Fixed by #505
Closed

[WP 5.3] Declaration should be compatible with Walker::walk #448

Chouby opened this issue Oct 15, 2019 · 6 comments · Fixed by #505
Milestone

Comments

@Chouby
Copy link

Chouby commented Oct 15, 2019

Bug Description

PHP 5.6 or above
WP 5.3-beta3
VIPCS 2.0

WP 5.3 modified the signature of Walker::walk(). If we adapt our code to this new signature, we get an error:

Declaration of `PLL_Walker_List::walk($elements, $max_depth, $args)` should be compatible with `Walker::walk($elements, $max_depth)`
(WordPressVIPMinimum.Classes.DeclarationCompatibility.DeclarationCompatibility)

The sniff should be adpated to the new method signature

Minimal Code Snippet

class PLL_Walker_List extends Walker {
	public function walk( $elements, $max_depth, ...$args ) {
		return parent::walk( $elements, $max_depth, $args );
	}
}

Tested Against master branch?

No but the code hasn't been modified for 5 months so cannot take this WP modification into account.

@GaryJones
Copy link
Contributor

@jrfnl
Copy link
Collaborator

jrfnl commented Oct 15, 2019

@Chouby Just a little heads-up, your code sample is bug-prone.

The correct way to do it would be:

class PLL_Walker_List extends Walker {
	public function walk( $elements, $max_depth, ...$args ) {
		return parent::walk( $elements, $max_depth, ...$args );
	}
}

Note the argument unpacking in the return statement. If you pass $args to the parent without that, you are passing an array, not the individual arguments received.

@jrfnl
Copy link
Collaborator

jrfnl commented Oct 15, 2019

Related: https://make.wordpress.org/core/2019/10/09/wp-5-3-introducing-the-spread-operator/

@Chouby
Copy link
Author

Chouby commented Oct 15, 2019

@jrfnl Thank you! This indeed could have been a bug. I removed most of the original code where at some point $args = $args[0] to construct the minimal code snippet, when I should have built a new one to avoid making this mistake ;-)

@jrfnl
Copy link
Collaborator

jrfnl commented Oct 15, 2019

@Chouby No worries, I just pointed it out in hopes of preventing the issue getting into a real codebase (or others finding this snippet and using it blindly).

@jrfnl
Copy link
Collaborator

jrfnl commented Jul 21, 2020

@Chouby PR #505 should fix this. Testing appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants