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

Universal/DisallowAlternativeSyntax: bug fix - handle if/elseif statements in one go #161

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 14, 2022

Follow up to PR #90 - note: this bug has not been in a tagged release, only in develop after #90.

As things were, the sniff would examine - and potentially fix - each control structure keyword which could be used with alternative syntax individually.

For potentially multi-part control structures, like if - elseif - else, this could lead to parse errors in the fixed code as mixing curly braces with alternative syntax within the same control structure "chain" is not allowed.

This only comes into play when the $allowWithInlineHTML property has been set to true as that's the only time when an if would potentially be treated differently from the associated else (one containing inline HTML being left alone, the other not containing inline HTML being "fixed").

While try - catch and do - while are also potentially multi-part control structures, these do not allow for using the alternative control structure syntax, so for the purposes of this sniff, if - elseif - else chains are the only ones we need to take into account.

Also take note of the fact that else if (with a space between) is not allowed when using alternative syntax, so we don't need to take the keyword potentially being split into account.

This commit adjusts the sniff to handle these chains in one go and to prevent the potential parse error, by:

  • No longer sniffing for T_ELSEIF or T_ELSE tokens. Note: this does affect how the metrics are recorded. Previously a metric would be recorded for each keyword. Now a metric will be recorded only for the first keyword in a chain. This doesn't make a difference for non-chained control structures, but it does make a difference for chained if/else sets. Also note: the metric will only be recorded for control structure keywords which support alternative syntax, not for all control structures. This remains the same as before.
  • When examining the control structure for inline HTML, the sniff will now loop from the first matching control structure keyword till the end* and remembering each control structure keyword (opener/closer) encountered along the way in case of a chain. For multi-part chains, once an inline HTML token has been encountered, subsequent "leafs" will no longer be examined for inline HTML. This should yield a small performance improvement.
  • The error messages will now be thrown via a loop based on the remembered information about each keyword encountered in a chain.
  • Along the same lines, the fixer will now make all fixes for a "chain" in one go based on the remembered information about each keyword encountered in the chain. This should also help prevent potential fixer conflicts between sniffs.

Includes a range of additional unit tests for this issue.

Also includes additional unit tests to safeguard that the behaviour of PHPCS regarding parse errors in chained control structures will not change, as the sniff has a build-in presumption about the PHPCS behaviour with control structures using alternative syntax, so if at any point in the future, the PHPCS token stream for this would change, the sniff will need adjusting.

…ments in one go

As things were, the sniff would examine - and potentially fix - each control structure keyword which could be used with alternative syntax individually.

For potentially multi-part control structures, like `if` - `elseif` - `else`, this could lead to parse errors in the fixed code as mixing curly braces with alternative syntax within the same control structure "chain" is not allowed.

This only comes into play when the `$allowWithInlineHTML` property has been set to `true` as that's the only time when an `if` would potentially be treated differently from the associated `else` (one containing inline HTML being left alone, the other not containing inline HTML being "fixed").

While `try` - `catch` and `do` - `while` are also potentially multi-part control structures, these do not allow for using the alternative control structure syntax, so for the purposes of this sniff, `if` - `elseif` - `else` chains are the only ones we need to take into account.

Also take note of the fact that `else if` (with a space between) is not allowed when using alternative syntax, so we don't need to take the keyword potentially being split into account.

This commit adjusts the sniff to handle these chains in one go and to prevent the potential parse error, by:
* No longer sniffing for `T_ELSEIF` or `T_ELSE` tokens.
    Note: this does affect how the metrics are recorded. Previously a metric would be recorded for each keyword. Now a metric will be recorded only for the _first_ keyword in a chain.
    This doesn't make a difference for non-chained control structures, but it does make a difference for chained `if/else` sets.
    Also note: the metric will only be recorded for control structure keywords which support alternative syntax, not for _all_ control structures. This remains the same as before.
* When examining the control structure for inline HTML, the sniff will now loop from the first matching control structure keyword till the `end*` and remembering each control structure keyword (opener/closer) encountered along the way in case of a chain.
    For multi-part chains, once an inline HTML token has been encountered, subsequent "leafs" will no longer be examined for inline HTML. This should yield a small performance improvement.
* The error messages will now be thrown via a loop based on the remembered information about each keyword encountered in a chain.
* Along the same lines, the fixer will now make all fixes for a "chain" in one go based on the remembered information about each keyword encountered in the chain.
    This should also help prevent potential fixer conflicts between sniffs.

Includes a range of additional unit tests for this issue.

Also includes additional unit tests to safeguard that the behaviour of PHPCS regarding parse errors in chained control structures will not change, as the sniff has a build-in presumption about the PHPCS behaviour with control structures using alternative syntax, so if at any point in the future, the PHPCS token stream for this would change, the sniff will need adjusting.
@jrfnl jrfnl added this to the 1.0.0-alpha4 milestone Nov 14, 2022
@jrfnl jrfnl merged commit cecabbd into develop Nov 14, 2022
@jrfnl jrfnl deleted the universal/disallowalternativesyntax-bug-fix-2 branch November 14, 2022 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant