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

Generic/RequireStrictTypes: various bugfixes #51

Merged
merged 4 commits into from
Dec 5, 2023

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 9, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3720:

Generic/RequireStrictTypes: add extra tests

These tests safeguard the following, which is already handled correctly by the sniff:

  • Execution directives are case-insensitive in PHP.
  • The sniff should ignore docblocks between a PHP open tag and a declare statement.

Generic/RequireStrictTypes: bug fix - limit token search to the statement

While hopefully unlikely in real life, the sniff should handle parse errors correctly.

Along the same lines, live coding should be handled correctly and a declare() statement should only be examined once the statement has been completed.

This updates the sniff to handle both situations correctly.
Without these fixes, no error would be thrown in test case file 5 or 6 (false negative), while an error would be thrown in test case file 7 (false positive).

Includes unit tests.

Includes minor efficiency tweak to only start looking for a "next" token after the current token.

Generic/RequireStrictTypes: bug fix - allow for multi directive statements

PHP allows for multiple directives to be passed in one declare() statement.

The sniff, however, did not allow for this, which could lead to false positives.

Fixed now.

Includes unit tests.

Generic/RequireStrictTypes: add warning for when value is 0

Strict types is disabled when the value for the strict_types directive is 0.

As this sniff is supposed to be about enforcing the use of strict_types, strict_types declarations which turn the feature off should be flagged as well.

Implemented with a separate error code to allow for selectively turning this warning off.

Includes dedicated tests.


Future Scope

This sniff isn't very efficient as it will walk the whole file until a declare() statement is found or until the end of the file is reached.

Declare statements for strict_types MUST be the first statement in a file, so IMO there are two options:

  • Either refactor the sniff to look for the first statement in a file instead of walking a complete file.
  • Or alternatively, keep the token walking, but don't stop on the first declare() statement and flag any strict_types declarations which are not the first statement in a file with a separate error code.

In both cases, "view" files containing a mix of PHP and HTML should be taken into account, similarly, hashbang lines should be taken into account.

Suggested changelog entry

  • Generic/RequireStrictTypes: will now bow out silently in case of parse errors.
  • Generic/RequireStrictTypes: bug fix - allow for multi directive statements
  • Generic/RequireStrictTypes: added new warning for when value is 0
    • The warning can be turned off by excluding the Generic.PHP.RequireStrictTypes.Disabled error code

These tests safeguard the following, which is already handled correctly by the sniff:
* Execution directives are case-insensitive in PHP.
* The sniff should ignore docblocks at the top of the file.
…ment

While hopefully unlikely in real life, the sniff should handle parse errors correctly.

Along the same lines, live coding should be handled correctly and a `declare()` statement should only be examined once the statement has been completed.

This updates the sniff to handle both situations correctly.
Without these fixes, no error would be thrown in test case file 5 or 6 (false negative), while an error would be thrown in test case file 7 (false positive).

Includes unit tests.

Includes minor efficiency tweak to only start looking for a "next" token _after_ the current token.
…ments

PHP allows for multiple directives to be passed in one `declare()` statement.

The sniff, however, did not allow for this, which could lead to false positives.

Fixed now.

Includes unit tests.
Strict types is disabled when the value for the `strict_types` directive is `0`.

As this sniff is supposed to be about enforcing the use of `strict_types`, `strict_types` declarations which turn the feature off should be flagged as well.

Implemented with a separate error code to allow for selectively turning this warning off.

Includes dedicated tests.
@jrfnl jrfnl force-pushed the feature/generic-requirestricttypes-various-bugfixes branch from 54a0dfe to 3ff23a3 Compare December 5, 2023 15:11
@jrfnl jrfnl merged commit ac6abf6 into master Dec 5, 2023
65 checks passed
@jrfnl jrfnl deleted the feature/generic-requirestricttypes-various-bugfixes branch December 5, 2023 15:16
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