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

Add config option for number of spaces around conditions in PEAR.ControlStructures.MultiLineCondition #454

Open
aik099 opened this issue Jan 22, 2015 · 10 comments

Comments

@aik099
Copy link
Contributor

aik099 commented Jan 22, 2015

The "SpacingAfterOpenBrace" error in "PEAR.ControlStructures.MultiLineCondition", that was introduced only in 2.2.0 release now checks that there must be no space between opening parenthesis of IF statement and 1st condition.

This conflicts with another sniff that allows for any X number of spaces (0 by default) that should be after opening parenthesis.

Error message: First condition of a multi-line IF statement must directly follow the opening parenthesis.

@gsherwood
Copy link
Member

None of the included standards are conflicting That check was specifically added because it was missing from the PEAR standard and so wasn't fixing code correctly.

Are you using 2 sniffs in a custom standard that conflict? If so, which 2 sniffs and what settings? What code are you running the standard over?

@aik099
Copy link
Contributor Author

aik099 commented Jan 22, 2015

Yes. It's PSR2.ControlStructures.ControlStructureSpacing sniff with requiredSpacesAfterOpen set to 1 plus this PEAR.ControlStructures.MultiLineCondition.

This way ControlStructureSpacing sniff wants 1 space after if (, but MultiLineCondition wants 0 space after if (. I guess this would end up in infinite fixing loop if I was fixing code.

@aik099
Copy link
Contributor Author

aik099 commented Jan 22, 2015

I guess Squiz standard can be affected as well (if somebody wants to customize it).

@gsherwood
Copy link
Member

You can't use the PEAR sniff if you want to enforce a single space after the braces. PEAR disallows this, so sniffs in there are going to enforce 0 spaces.

I think what you really need is an option in the PEAR sniff like the PSR sniff has. Does this sound right?

@aik099
Copy link
Contributor Author

aik099 commented Jan 22, 2015

I think what you really need is an option in the PEAR sniff like the PSR sniff has. Does this sound right?

Yes, option for specifying how much spaces needs to be after ( and before ) in control structure conditions is always a good idea. For now I've disabled that particular error in my ruleset.xml.

You can't use the PEAR sniff if you want to enforce a single space after the braces. PEAR disallows this, so sniffs in there are going to enforce 0 spaces.

If PEAR has a rule, that no space should be after if ( then more general control structure sniff should check that and not the MultiLineCondition sniff, which only cares about IFs with multi-line conditions.

@dereuromark
Copy link
Contributor

The other issue is that valid PSR-2 code like this is now falsely reported:

if (
    (count($this->_values) && $data instanceof Query) ||
    ($this->_query && is_array($data))
) {
    ...
}

Please revert this.
The PSR-2 states:

Opening parentheses for control structures MUST NOT have a space after them, and closing parentheses for control structures MUST NOT have a space before.

That means a real "space", not a newline-whitespace:

// invalid PSR-2
if ( $foo ) {
}

Note that you do NOT check on closing parentheses and whitespace before that one either (which would currently be ok, and would be inconsistent to the breaking chance introduced in 2.2.0).

To sum things up:
You need to either force

// multiline phpcs 2.2.0 (fixed)
if ($foo && 
    $bar) {

or continue to allow (IMO PSR-2 conform)

// multiline without 2.2.0 regression
if (
    $foo &&
    $bar
) {

And also note that the first is not good because the indentation level is not clear from that:

// multiline phpcs 2.2.0 (fixed)
if ($foo && 
    $bar) {
    $foo = $bar; // This line has now the same indentation
}

That proofs for me that this is indeed the wrong interpretation of what FIG implicitly meant here.
The first only works when one uses {} on new lines consistently also for control structures (not just for classes and methods), which PSR2 does not allow:

if ($foo && 
    $bar)
{
    $foo = $bar; // This line has now has correct indentation
}

@gsherwood
Copy link
Member

@dereuromark The change you are talking about is here: ccf1f70

It is completely unrelated to the change @aik099 is talking about, which is only in the PEAR standard.

You may want to submit a different report for the issue you are having. Then we can have a discussion about if newlines are the same thing as spaces or not.

@dereuromark
Copy link
Contributor

I am sorry if I confused the issues. If you want to can port the relevant information to a different issue.

@TomasVotruba
Copy link
Contributor

Got same issue in 2.2.0 with negation in front:

if ( ! $this->isDbSchemaPrepared) {

(PEAR.ControlStructures.MultiLineCondition.SpacingAfterOpenBrace)

Temp solution:

<rule ref="PEAR.ControlStructures.MultiLineCondition.SpacingAfterOpenBrace"><severity>0</severity></rule>

@aik099
Copy link
Contributor Author

aik099 commented Jan 23, 2015

In my ruleset.xml I've disabled that particular error altogether because check itself is misplaced (already one by other sniffs) as I've mentioned above.

@gsherwood gsherwood changed the title New error in "PEAR.ControlStructures.MultiLineCondition" sniff causes issues Add config option for number of spaces around conditions in PEAR.ControlStructures.MultiLineCondition Mar 3, 2015
@gsherwood gsherwood added this to Backlog in PHPCS v3 Development May 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants