Add sniff to disallow alternative PHP open tags. #585
Conversation
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
4 times, most recently
from
40e6635
to
e2b8b0c
Compare
|
Ok, PHP 5.2 was playing up - apparently the Token parser changed in 5.3. Should be all fixed now and ready for merge. [Edit] and then with the addition of PHP7 to travis, PHP7 started playing up too 😢 Oh well... all fixed again |
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
2 times, most recently
from
9a5f949
to
d42e161
Compare
|
Any feedback on this ? |
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
from
d42e161
to
31ac198
Compare
|
Could these be marked as fixable errors, and thus make them fixable, or does that require finding the fixing the matching closing tags too? |
Good point - only just started looking at fixable things. Will see if I can make this work. Will need some help in the form of a tester as fixer doesn't work on Windows, so can't test it myself (and it looks like the unit tests don't test the fixing ?)
No need - syntax-wise PHP will parse just fine with mixed type open and close tags. However, leaving the code with mixed tags seems to me less tidy and I can imagine that this could cause confusion for people later on. |
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
from
10aeb65
to
2472016
Compare
|
Added a new commit which should make this fixable and adjusts both the open as well as the close tags if matching ones were found. Testing appreciated. |
|
Since it now handles closing tags as well, would calling it |
|
Do note - it does not check for mismatches tags or alternative closing tags. It only covers the closing tags in the fix when alternative open tags have been found. |
|
Renamed the sniff. |
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
from
22edf08
to
2c4194a
Compare
|
Rebased to force a travis check against the new WPCS codestyle rules. |
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
from
698b381
to
f29c3c6
Compare
| $tokens = $phpcsFile->getTokens(); | ||
| $openTag = $tokens[ $stackPtr ]; | ||
| $asp_enabled = (boolean) ini_get( 'asp_tags' ); | ||
| $short_enabled = (boolean) ini_get( 'short_open_tag' ); |
There was a problem hiding this comment.
Why do we base the behavior of the sniff on the ini settings of the PHP build that is running PHPCS? If you ask me that kind of defeats the purpose of the sniff, which is to check that you are using cross-site compatible PHP tags (at least that's what I think one purpose of it is). So wouldn't it be better to check for all tags by default, but allow this to be configured in custom rulesets if folks want to allow some of them? Or am I missing something here? At the very least the settings should be retrieved only once, for better performance.
There was a problem hiding this comment.
Why do we base the behavior of the sniff on the ini settings of the PHP build that is running PHPCS?
If the PHP build on which PHPCS is run has short_open_tag on and asp_tags on, it will recognize the tags as T_OPEN_TAG/ T_OPEN_WITH_ECHO.
If these are turned off, the tags will be seen as T_INLINE_HTML. _This also goes for any code between the alternative open tags - it will not be tokenized properly! - it will just be seen as T_INLINE_HTML _
Same goes, of course, for the PHP build on which the code will be run eventually - if the ini settings are on, these tags work, if they are off, they are ignored and just printed as HTML.
So, what I've done, is, make the sniff recognize these kind of tags independently of the system settings. That way the tags are 'caught' independently of the system PHPCS is being run on.
If the settings are on, you will get an error as we know for sure that alternative open tags are used.
If the settings are off and changes are that these tags are being used, you will get a warning saying this needs manual inspection.
So wouldn't it be better to check for all tags by default, but allow this to be configured in custom rulesets if folks want to allow some of them?
How the content of the token is tested needs to be different for the different tokens to make sure that they are being picked up on properly, so I use the ini settings to only run the tests which actually makes sense for the system running PHPCS (efficiency).
Letting this be configurable does not make sense as that would in practice be the same as just turning the test off.
At the very least the settings should be retrieved only once, for better performance.
Good point. I will set up some properties for this and fill these in the register() method (only once).
There was a problem hiding this comment.
Just thinking - I should actually make the test for T_INLINE_HTML less precise as the tag does not have to be at the start of the token content. Let me fiddle with this a bit.
Fixes WordPress#580 This should be removed again if the upstream issue squizlabs/PHP_CodeSniffer#1063 would get traction and the associated PR would be merged *and* the minimum PHPCS version for WPCS would become higher than the version in which the PR is merged ;-)
Only implemented for when we are sure we've found alternative opening tags. The 'maybe' causes which need manual inspection should not be auto-fixable.
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
2 times, most recently
from
5d5ec8f
to
82697f2
Compare
|
Triggered by @JDGrimes's remark from yesterday, I've had another good look at this PR and have rewritten a large part of it. I believe for the better, hope you agree. I've also now send in the upstream PR for the same: squizlabs/PHP_CodeSniffer#1084, but get the impression not much is happening or being done with PRs upstream atm. I guess they're more focused on PHPCS 3.0. |
… New & improved version. Includes additional unit tests.
jrfnl
force-pushed
the
feature/issue-580-alternative-opentags
branch
from
82697f2
to
34d436f
Compare
Fixes #580
This should be removed again if the upstream issue squizlabs/PHP_CodeSniffer#1063 would get traction and the associated PR (not yet send in) would be merged and the minimum PHPCS version for WPCS would become higher than the version in which the PR is merged ;-)