-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Psr0Fixer - Ignore filenames that are a reserved keyword or predefined constant #1541
Psr0Fixer - Ignore filenames that are a reserved keyword or predefined constant #1541
Conversation
@@ -161,6 +160,12 @@ public function supports(\SplFileInfo $file) | |||
return false; | |||
} | |||
|
|||
$tokens = token_get_all(sprintf('<?php %s', $filenameParts[0])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if anyone knows a better way to check if a string is predefined constant or keyword in PHP, I'm happy to update :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm afraid not so, it provides the [int] type/kind - [int] value
, but I need to check against the string as used in the code itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In tests you have list of 65 keywords. but there is 66 keywords. They are listed in method I referred to.
Create similar method to check if token is predefined constant.
Then, leave here that tokenizer call (just change it to Tokens::fromCode
and in if
below check $tokens[1]->isKeyword()
and your new method $tokens[1]->isPredefinedConstant()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
should these be added to the list of predefined constants or do you want those on PHP7 only http://php.net/manual/en/reserved.other-reserved-words.php ?
btw. the same checks added to the class should be done for (parts of) the namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think they should
lets see what Travis thinks, http://php.net/manual/en/reserved.other-reserved-words.php is still a TODO |
@@ -28,8 +28,11 @@ public function process(Tokens $tokens) | |||
{ | |||
foreach ($tokens->findGivenKind(T_ARRAY) as $index => $token) { | |||
$nextIndex = $tokens->getNextMeaningfulToken($index); | |||
$nextToken = $tokens[$nextIndex]; | |||
if (null === $nextIndex) { | |||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when does this happen? new test is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PSR0 fixer generates <?php class array
and ask the tokenizer for tokens of that code, then this will break down.
Can be tested by adding;
array(
'<?php class array',
array()
),
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how it behaves after https://github.com/FriendsOfPHP/PHP-CS-Fixer/pull/1541/files#r45877095 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kind of works, however 'CT_ARRAY_TYPEHINT'
is not in https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/1.11/Symfony/CS/Tokenizer/Token.php#L247 so the psr0 test fails. Should the custom token not be in that list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
I'd prefer to ignore all these cases on all PHP versions since we should be able to fix code that will run ok on later PHP versions. |
I prefer that as well, however since we don't know have the tokens kind constants on newer versions we cannot filter on those. |
Please fix Travis |
all green now :) |
@@ -367,6 +395,13 @@ public function isNativeConstant() | |||
return $this->isArray && in_array(strtolower($this->content), $nativeConstantStrings, true); | |||
} | |||
|
|||
public function isMagicConstant() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to be tested
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please see SpacePossum@6812475
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test for __class__
. Should work, but I want to be sure. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure thing :)
SpacePossum@8573283
this way both lower and upper case for all magic constants are tested.. lets see what travis thinks..
👍 |
{ | ||
$this->assertFalse($this->getBraceToken()->isMagicConstant()); | ||
$token = new Token(array(T_CLASS_C, '__CLASS__')); | ||
$this->assertTrue($token->isMagicConstant()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to have the lowercase variant here, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once that's done, 👍 from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added in SpacePossum@96a7ac9 with some more test cases.
Thank you @SpacePossum. |
… predefined constant (SpacePossum) This PR was squashed before being merged into the 1.10 branch (closes #1541). Discussion ---------- Psr0Fixer - Ignore filenames that are a reserved keyword or predefined constant The PSR0 fixer should not try to fix files that are named a predefined constant or keyword, like `interface.php` Commits ------- 09777a8 Psr0Fixer - Ignore filenames that are a reserved keyword or predefined constant
The PSR0 fixer should not try to fix files that are named a predefined constant or keyword,
like
interface.php