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

`WordPress.Arrays.ArrayDeclarationSpacing` fails on some files (fixer conflict) #968

Closed
pento opened this Issue Jun 16, 2017 · 21 comments

Comments

@pento
Member

pento commented Jun 16, 2017

While running this rule on Core, it causes some files to exceed the pass limit. It seems to be getting stuck in a loop of changing how lines are indented, as each rule violates some other rule.

For example, src/wp-content/themes/twentyeleven/404.php will get stuck.

@JDGrimes

This comment has been minimized.

Show comment
Hide comment
@JDGrimes

JDGrimes Jun 16, 2017

Contributor

Minimum reproducible code:

the_widget( 'WP_Widget_Recent_Posts', array( 'number' => 10 ), array( 'widget_id' => '404' ) );

Basically the issue is that there are two associative arrays on the same line.

I think that the sniff that is actually WordPress.Arrays.ArrayDeclarationSpacing that is causing the issue. It is trying to fix the second array to be multiline after fixing the first array, and that is where it fails. Remove the second associative array and the code is fixed without problem.

Contributor

JDGrimes commented Jun 16, 2017

Minimum reproducible code:

the_widget( 'WP_Widget_Recent_Posts', array( 'number' => 10 ), array( 'widget_id' => '404' ) );

Basically the issue is that there are two associative arrays on the same line.

I think that the sniff that is actually WordPress.Arrays.ArrayDeclarationSpacing that is causing the issue. It is trying to fix the second array to be multiline after fixing the first array, and that is where it fails. Remove the second associative array and the code is fixed without problem.

@JDGrimes JDGrimes removed their assignment Jun 16, 2017

@jrfnl jrfnl self-assigned this Jun 16, 2017

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 16, 2017

Contributor

I'll see if I can get a fix sorted later today.

Contributor

jrfnl commented Jun 16, 2017

I'll see if I can get a fix sorted later today.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 17, 2017

Contributor

I've been investigating this issue and I can confirm @JDGrimes 's analysis. The issue is in the WordPress.Arrays.ArrayDeclarationSpacing sniff, but only when used in combination with an array passed as a parameter in a function call.

The conflict is with the PEAR.Functions.FunctionCallSignature sniff with the "Multi-line function call not indented correctly" error.

My current line of thinking for a solution is:

  • Check if the associative array declaration is found within nested parenthesis (like with a function call)
  • If so, add a new line before the array keyword or [ array opener.
  • I'm also thinking of removing the indenting from the fixer there and leaving that completely up to the ArrayIndentation sniff - which didn't yet exist when the fixer was added to the WordPress.Arrays.ArrayDeclarationSpacing sniff => pulled as #1004

Will need to play with this for a bit & do a lot of testing to see if this will work.

Contributor

jrfnl commented Jun 17, 2017

I've been investigating this issue and I can confirm @JDGrimes 's analysis. The issue is in the WordPress.Arrays.ArrayDeclarationSpacing sniff, but only when used in combination with an array passed as a parameter in a function call.

The conflict is with the PEAR.Functions.FunctionCallSignature sniff with the "Multi-line function call not indented correctly" error.

My current line of thinking for a solution is:

  • Check if the associative array declaration is found within nested parenthesis (like with a function call)
  • If so, add a new line before the array keyword or [ array opener.
  • I'm also thinking of removing the indenting from the fixer there and leaving that completely up to the ArrayIndentation sniff - which didn't yet exist when the fixer was added to the WordPress.Arrays.ArrayDeclarationSpacing sniff => pulled as #1004

Will need to play with this for a bit & do a lot of testing to see if this will work.

@pento

This comment has been minimized.

Show comment
Hide comment
@pento

pento Jun 25, 2017

Member

A variation of this I've run into, which looks to have the same cause:

a( array( 'b' => 'c' ), d( array( 'e' => 'f' ) ) );
Member

pento commented Jun 25, 2017

A variation of this I've run into, which looks to have the same cause:

a( array( 'b' => 'c' ), d( array( 'e' => 'f' ) ) );
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 30, 2017

Contributor

@pento Yes, that definitely is one of the same kind as well.

I'm looking into fixing this, but there are three ways I can go about this and I'd like some opinions on what is preferred:

a( true, array( 'b' => 'c' ), d( array( 'e' => 'f' ), '' ), 123 );

If an associative array is found as a parameter within a function call...

  1. ... throw an error, but do not auto-fix it. (code would remain unchanged)
a( true, array( 'b' => 'c' ), d( array( 'e' => 'f' ), '' ), 123 );
  1. ... make sure that the array parameter starts on a new line, but don't change any of the other arguments (this may be then changed by another sniff anyhow, but I'd need to test that)
a( true,
	array(
		'b' => 'c',
	), d(
		array(
			'e' => 'f',
		), '' ), 123 );
  1. ... change the function call to be a "proper" multi-line function call with each argument on a new line.
a(
	true,
	array(
		'b' => 'c',
	),
	d(
		array(
			'e' => 'f',
		),
		''
	),
	123
);

Opinions ?

Contributor

jrfnl commented Jun 30, 2017

@pento Yes, that definitely is one of the same kind as well.

I'm looking into fixing this, but there are three ways I can go about this and I'd like some opinions on what is preferred:

a( true, array( 'b' => 'c' ), d( array( 'e' => 'f' ), '' ), 123 );

If an associative array is found as a parameter within a function call...

  1. ... throw an error, but do not auto-fix it. (code would remain unchanged)
a( true, array( 'b' => 'c' ), d( array( 'e' => 'f' ), '' ), 123 );
  1. ... make sure that the array parameter starts on a new line, but don't change any of the other arguments (this may be then changed by another sniff anyhow, but I'd need to test that)
a( true,
	array(
		'b' => 'c',
	), d(
		array(
			'e' => 'f',
		), '' ), 123 );
  1. ... change the function call to be a "proper" multi-line function call with each argument on a new line.
a(
	true,
	array(
		'b' => 'c',
	),
	d(
		array(
			'e' => 'f',
		),
		''
	),
	123
);

Opinions ?

@JDGrimes

This comment has been minimized.

Show comment
Hide comment
@JDGrimes

JDGrimes Jun 30, 2017

Contributor

Perhaps best to just throw an error here. Although option 3 is kind of the goal end result, I think that we probably don't want the sniff to be that opinionated about unrelated stuff. I'd possibly go for option 2 if other sniffs would indeed clean it up a bit.

Contributor

JDGrimes commented Jun 30, 2017

Perhaps best to just throw an error here. Although option 3 is kind of the goal end result, I think that we probably don't want the sniff to be that opinionated about unrelated stuff. I'd possibly go for option 2 if other sniffs would indeed clean it up a bit.

@jrfnl jrfnl changed the title from `WordPress.Arrays.ArrayIndentation` fails on some files to `WordPress.Arrays.ArrayDeclarationSpacing` fails on some files Jun 30, 2017

@jrfnl jrfnl changed the title from `WordPress.Arrays.ArrayDeclarationSpacing` fails on some files to `WordPress.Arrays.ArrayDeclarationSpacing` fails on some files (fixer conflict) Jun 30, 2017

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 30, 2017

Contributor

Just ran a number of tests with option 2 and 3:

Option 2 - would still result in a conflict

Option 3 - can solve the conflict, but only if we also add a new line before the function call close parenthesis.

On a related note: I believe we should ignore associative arrays which are default values in a function declaration for the fixer part (and maybe even for the error throwing too ? )

function foo( $config = array( 'b' => 'c' ), 123 ) {}
Contributor

jrfnl commented Jun 30, 2017

Just ran a number of tests with option 2 and 3:

Option 2 - would still result in a conflict

Option 3 - can solve the conflict, but only if we also add a new line before the function call close parenthesis.

On a related note: I believe we should ignore associative arrays which are default values in a function declaration for the fixer part (and maybe even for the error throwing too ? )

function foo( $config = array( 'b' => 'c' ), 123 ) {}
@JDGrimes

This comment has been minimized.

Show comment
Hide comment
@JDGrimes

JDGrimes Jun 30, 2017

Contributor

On a related note: I believe we should ignore associative arrays which are default values in a function declaration for the fixer part (and maybe even for the error throwing too ? )

+1

Contributor

JDGrimes commented Jun 30, 2017

On a related note: I believe we should ignore associative arrays which are default values in a function declaration for the fixer part (and maybe even for the error throwing too ? )

+1

@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Jun 30, 2017

Member

In an ideal world, I'd want to see 3 as the end result too.

And +1 for ignoring assoc arrays when they are default values (though there is an argument that default values should only ever be null, and that logic like default values should be handled inside the function, not as part of its signature).

Member

GaryJones commented Jun 30, 2017

In an ideal world, I'd want to see 3 as the end result too.

And +1 for ignoring assoc arrays when they are default values (though there is an argument that default values should only ever be null, and that logic like default values should be handled inside the function, not as part of its signature).

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 30, 2017

Contributor

@JDGrimes @GaryJones Thanks for your input. I've got option three working, so I'll tidy up, run some more tests and will pull it if I don't encounter any more issues with it.

(though there is an argument that default values should only ever be null, and that logic like default values should be handled inside the function, not as part of its signature).

That... is a completely different and unrelated issue IMO and pretty opinionated at that. Thinking of WP core and the "average" level of abstraction in themes and plugins, I don't think that a sniff for something like that would be appropriate for WPCS anyhow. At least not for the foreseeable future.

Contributor

jrfnl commented Jun 30, 2017

@JDGrimes @GaryJones Thanks for your input. I've got option three working, so I'll tidy up, run some more tests and will pull it if I don't encounter any more issues with it.

(though there is an argument that default values should only ever be null, and that logic like default values should be handled inside the function, not as part of its signature).

That... is a completely different and unrelated issue IMO and pretty opinionated at that. Thinking of WP core and the "average" level of abstraction in themes and plugins, I don't think that a sniff for something like that would be appropriate for WPCS anyhow. At least not for the foreseeable future.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 30, 2017

Contributor

Ok, done some more investigation and the root cause of the issue is that the core ruleset excludes two checks from the PEAR.Functions.FunctionCallSignature sniff.

	<rule ref="PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket">
		<severity>0</severity>
	</rule>
	<rule ref="PEAR.Functions.FunctionCallSignature.CloseBracketLine">
		<severity>0</severity>
	</rule>

These exclusions were added at the same time as the WPCS version of that sniff was removed in favour of the upstream sniff in 2014 by @westonruter If I understand correctly, this was to allow for code like:

foreach ( $core_options as $option_name => $real_value ) {
    add_filter( "pre_option_{$option_name}", function( $value ) use ( $real_current_blog_id, $real_value ) {
        if ( get_current_blog_id() == $real_current_blog_id ) {
            return $real_value + 1;
        } else {
            return $value;
        }
    } );
}

Removing the exclusions solves the issue without any further code changes in the WordPress.Arrays.ArrayDeclarationSpacing sniff and would result in the above snippet being fixed to the below which, to be honest, looks better than before to me:

foreach ( $core_options as $option_name => $real_value ) {
	add_filter(
		"pre_option_{$option_name}", function( $value ) use ( $real_current_blog_id, $real_value ) {
			if ( get_current_blog_id() == $real_current_blog_id ) {
				return $real_value + 1;
			} else {
				return $value;
			}
		}
	);
}

Refs:


[Edit]:
Oh and the snippet I used earlier as an example would be fixed - without conflicts - like this when the two exclusions would be removed:

a(
	true, array(
		'b' => 'c',
	), d(
		[
			'e' => 'f',
		], ''
	), 123
);
Contributor

jrfnl commented Jun 30, 2017

Ok, done some more investigation and the root cause of the issue is that the core ruleset excludes two checks from the PEAR.Functions.FunctionCallSignature sniff.

	<rule ref="PEAR.Functions.FunctionCallSignature.ContentAfterOpenBracket">
		<severity>0</severity>
	</rule>
	<rule ref="PEAR.Functions.FunctionCallSignature.CloseBracketLine">
		<severity>0</severity>
	</rule>

These exclusions were added at the same time as the WPCS version of that sniff was removed in favour of the upstream sniff in 2014 by @westonruter If I understand correctly, this was to allow for code like:

foreach ( $core_options as $option_name => $real_value ) {
    add_filter( "pre_option_{$option_name}", function( $value ) use ( $real_current_blog_id, $real_value ) {
        if ( get_current_blog_id() == $real_current_blog_id ) {
            return $real_value + 1;
        } else {
            return $value;
        }
    } );
}

Removing the exclusions solves the issue without any further code changes in the WordPress.Arrays.ArrayDeclarationSpacing sniff and would result in the above snippet being fixed to the below which, to be honest, looks better than before to me:

foreach ( $core_options as $option_name => $real_value ) {
	add_filter(
		"pre_option_{$option_name}", function( $value ) use ( $real_current_blog_id, $real_value ) {
			if ( get_current_blog_id() == $real_current_blog_id ) {
				return $real_value + 1;
			} else {
				return $value;
			}
		}
	);
}

Refs:


[Edit]:
Oh and the snippet I used earlier as an example would be fixed - without conflicts - like this when the two exclusions would be removed:

a(
	true, array(
		'b' => 'c',
	), d(
		[
			'e' => 'f',
		], ''
	), 123
);
@GaryJones

This comment has been minimized.

Show comment
Hide comment
@GaryJones

GaryJones Jun 30, 2017

Member

Oh and the snippet I used earlier as an example would be fixed - without conflicts - like this when the two exclusions would be removed:

Not quite the same as your initial example of 3 - breaks on function calls and array keys, but not on parent function parameters. I'd prefer it in a single line compared to this.

Member

GaryJones commented Jun 30, 2017

Oh and the snippet I used earlier as an example would be fixed - without conflicts - like this when the two exclusions would be removed:

Not quite the same as your initial example of 3 - breaks on function calls and array keys, but not on parent function parameters. I'd prefer it in a single line compared to this.

@JDGrimes

This comment has been minimized.

Show comment
Hide comment
@JDGrimes

JDGrimes Jun 30, 2017

Contributor

I have no problem with the updated fix. Personally, if a function call has multiline parameters, I always put each parameter on its own line. I don't know if there is a sniff that enforces that, but I think code like your second example is unreadable.

Contributor

JDGrimes commented Jun 30, 2017

I have no problem with the updated fix. Personally, if a function call has multiline parameters, I always put each parameter on its own line. I don't know if there is a sniff that enforces that, but I think code like your second example is unreadable.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jun 30, 2017

Contributor

The thing is, if exclusions are causing the conflicts in fixers, then we should address that problem and not try to work around it.

That means in practice any of the following solutions:

  • Remove exclusions & live with the consequences
  • If possible/relevant, pull fixes upstream if that would ease the pain (though that's awkward now with PHPCS 2.x vs 3.x)
  • Or find or write a better sniff for the sniff (or parts thereof) where we needed the exclusions

Adjusting an unrelated sniff to work round exclusions made to another sniff feels like the wrong way to go.

Contributor

jrfnl commented Jun 30, 2017

The thing is, if exclusions are causing the conflicts in fixers, then we should address that problem and not try to work around it.

That means in practice any of the following solutions:

  • Remove exclusions & live with the consequences
  • If possible/relevant, pull fixes upstream if that would ease the pain (though that's awkward now with PHPCS 2.x vs 3.x)
  • Or find or write a better sniff for the sniff (or parts thereof) where we needed the exclusions

Adjusting an unrelated sniff to work round exclusions made to another sniff feels like the wrong way to go.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 8, 2017

Contributor

@westonruter Have you got an opinion on this as you added the original exclusions ?

Contributor

jrfnl commented Jul 8, 2017

@westonruter Have you got an opinion on this as you added the original exclusions ?

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jul 9, 2017

Member

If these exclusions are removed, would it not cause these very common core WP patterns to be marked as invalid?

	$atts = shortcode_atts( array(
		'order'      => 'ASC',
		'orderby'    => 'menu_order ID',
		'id'         => $post ? $post->ID : 0,
		'itemtag'    => $html5 ? 'figure'     : 'dl',
		'icontag'    => $html5 ? 'div'        : 'dt',
		'captiontag' => $html5 ? 'figcaption' : 'dd',
		'columns'    => 3,
		'size'       => 'thumbnail',
		'include'    => '',
		'exclude'    => '',
		'link'       => ''
	), $attr, 'gallery' );

and

	$link = sprintf( '<a href="%1$s" title="%2$s" rel="author">%3$s</a>',
		esc_url( get_author_posts_url( $authordata->ID, $authordata->user_nicename ) ),
		/* translators: %s: author's display name */
		esc_attr( sprintf( __( 'Posts by %s' ), get_the_author() ) ),
		get_the_author()
	);

and

		$set = wp_parse_args( $settings, array(
			'wpautop'             => true,
			'media_buttons'       => true,
			// ...
		) );
Member

westonruter commented Jul 9, 2017

If these exclusions are removed, would it not cause these very common core WP patterns to be marked as invalid?

	$atts = shortcode_atts( array(
		'order'      => 'ASC',
		'orderby'    => 'menu_order ID',
		'id'         => $post ? $post->ID : 0,
		'itemtag'    => $html5 ? 'figure'     : 'dl',
		'icontag'    => $html5 ? 'div'        : 'dt',
		'captiontag' => $html5 ? 'figcaption' : 'dd',
		'columns'    => 3,
		'size'       => 'thumbnail',
		'include'    => '',
		'exclude'    => '',
		'link'       => ''
	), $attr, 'gallery' );

and

	$link = sprintf( '<a href="%1$s" title="%2$s" rel="author">%3$s</a>',
		esc_url( get_author_posts_url( $authordata->ID, $authordata->user_nicename ) ),
		/* translators: %s: author's display name */
		esc_attr( sprintf( __( 'Posts by %s' ), get_the_author() ) ),
		get_the_author()
	);

and

		$set = wp_parse_args( $settings, array(
			'wpautop'             => true,
			'media_buttons'       => true,
			// ...
		) );
@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 9, 2017

Contributor

@westonruter Yes it would, though we could leave the exclusions for "normal" runs and only remove them for the fixer by adding phpcs-only="true" to the exclusions.
See: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml#selectively-applying-rules

The alternative would be option 1 from #968 (comment) - throw an error (or warning), but don't auto-fix associative arrays which are parameters in a function call.

Or write a completely different sniff for function call signatures as the PEAR one basically presumes either single-line calls or multi-line calls where each parameter is placed on a new line, but does not allow for a mix.

As they currently are, the WP coding standards allow multi-line function calls with an arbitrary number or parameters per line (which is not in line with the PEAR sniff we're using).

Whether this is a conscious choice or a hiatus in the Coding Standards should inform the direction to take.

Contributor

jrfnl commented Jul 9, 2017

@westonruter Yes it would, though we could leave the exclusions for "normal" runs and only remove them for the fixer by adding phpcs-only="true" to the exclusions.
See: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Annotated-ruleset.xml#selectively-applying-rules

The alternative would be option 1 from #968 (comment) - throw an error (or warning), but don't auto-fix associative arrays which are parameters in a function call.

Or write a completely different sniff for function call signatures as the PEAR one basically presumes either single-line calls or multi-line calls where each parameter is placed on a new line, but does not allow for a mix.

As they currently are, the WP coding standards allow multi-line function calls with an arbitrary number or parameters per line (which is not in line with the PEAR sniff we're using).

Whether this is a conscious choice or a hiatus in the Coding Standards should inform the direction to take.

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jul 9, 2017

Member

I don't think we'd want there to be warnings or errors ever to surface due to these sniffs, since this coding style is pervasive. For example: https://codex.wordpress.org/Function_Reference/add_shortcode

So excluding them and removing them from the fixer seems like the best way to go in the short term, if I understand the options sufficiently.

Member

westonruter commented Jul 9, 2017

I don't think we'd want there to be warnings or errors ever to surface due to these sniffs, since this coding style is pervasive. For example: https://codex.wordpress.org/Function_Reference/add_shortcode

So excluding them and removing them from the fixer seems like the best way to go in the short term, if I understand the options sufficiently.

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 9, 2017

Contributor

So excluding them and removing them from the fixer

I'm not sure I understand what you mean by that. Could you be a bit more specific ?

Contributor

jrfnl commented Jul 9, 2017

So excluding them and removing them from the fixer

I'm not sure I understand what you mean by that. Could you be a bit more specific ?

@westonruter

This comment has been minimized.

Show comment
Hide comment
@westonruter

westonruter Jul 9, 2017

Member

What you said:

we could leave the exclusions for "normal" runs and only remove them for the fixer

Member

westonruter commented Jul 9, 2017

What you said:

we could leave the exclusions for "normal" runs and only remove them for the fixer

@jrfnl

This comment has been minimized.

Show comment
Hide comment
@jrfnl

jrfnl Jul 9, 2017

Contributor

Ok. PR to that effect has been pulled as #1020

Contributor

jrfnl commented Jul 9, 2017

Ok. PR to that effect has been pulled as #1020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment