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 new sniff to check for NewExecutionDirectives. #169

Merged
merged 1 commit into from Aug 14, 2016

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Aug 14, 2016

This sniff checks for valid execution directives set with declare().
Ref: http://php.net/manual/en/control-structures.declare.php

The sniff contains three distinct checks:

  • Check if the execution directive used is valid. PHP currently only supports three execution directives.
  • Check if the execution directive used is available in the PHP versions for which support is being checked. In the case of the encoding directive on PHP 5.3, support is conditional on the --enable-zend-multibyte compilation option. This will be indicated as such.
  • Check whether the value for the directive is valid.

Includes unit tests.

This sniff checks for valid execution directives set with `declare()`.
Ref: http://php.net/manual/en/control-structures.declare.php

The sniff contains three distinct checks:
* Check if the execution directive used is valid. PHP currently only supports three execution directives.
* Check if the execution directive used is available in the PHP versions for which support is being checked. In the case of the `encoding` directive on PHP 5.3, support is conditional on the `--enable-zend-multibyte` compilation option. This will be indicated as such.
* Check whether the value for the directive is valid.

Includes unit tests.
@wimg wimg merged commit 33c6a28 into PHPCompatibility:master Aug 14, 2016
@jrfnl jrfnl deleted the feature/new-declare-directives branch August 15, 2016 00:24
@MarkMaldaba
Copy link
Contributor

Check if the execution directive used is valid. PHP currently only supports three execution directives.

@jrfnl - Just a general question, as I've seen this kind of thing in a few of your recent commits (which have been absolutely amazing, by the way, thank you so much for your hard work!):

Should the PHPCompatibility sniff be checking for invalid/incorrect code (as opposed to use of deprecated/not implemented features)?

My gut feeling is that this is out-of-scope. Using this sniff as an example, here's what I would expect to be reported:

  • declare(ticks=1); -- No warning, as valid in all versions of PHP
  • declare(encoding='ISO-8859-1'); -- Warning on PHP < 5.3 (not implemented), No warning on PHP >= 5.3 (supported)
  • declare(invalid=true); -- No warning, as not valid in any version of PHP

I have seen a few similar examples in other sniffs where invalid syntax/semantics are being reported and to me this seems slightly out-of-scope for the project (as it doesn't affect compatibility - the result is the same in all PHP versions).

Thoughts?

@jrfnl
Copy link
Member Author

jrfnl commented Aug 18, 2016

Should the PHPCompatibility sniff be checking for invalid/incorrect code (as opposed to use of deprecated/not implemented features)?

Good question. IMHO I agree that in most cases it is out-of-scope, but there are some grey areas where PHP might not give an error/warning, but things will not work if used incorrectly.
Generally, I would expect a seasoned programmer will never see those warnings anyways as they won't do those things wrong, but for less experienced programmers it might provide them with a helpful hint.

As a rule of thumb, I've basically only done something in that line of thinking if it was an easy thing to add and felt intuitive to also check for, but that last part is of course personal.

@wimg Care to weight in ?

@jrfnl jrfnl added this to the 7.0.3 milestone Apr 30, 2017
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.

None yet

3 participants