Add NoUnsetOnPropertyFixer #3737
Conversation
|
Hi ! unsetting key of property is not the same as unsetting the property, |
| '<?php unset($this->bar, self::$foo, $bar);', | ||
| ], | ||
| 'It does not replace unsets on arrays' => [ | ||
| '<?php unset($bar->foo[0]);', |
There was a problem hiding this comment.
add '<?php unset($bar->foo{0});', as well.
all hail php...
There was a problem hiding this comment.
I never knew you could do that, and now i wish i didn't know.
There was a problem hiding this comment.
One thing about writing fixers is you come across a lot of syntax you wish didn't exist 😆
There was a problem hiding this comment.
@BackEndTea , don't you worry. simply start using normalize_index_brace and forgot about that syntax in your life ;)
(well, just not in this PR ;P thanks for adding a test 👍)
There was a problem hiding this comment.
you can also mix those up, so please add the test for <?php unset($bar->foo{0}[1]{2}{$a}[$b]);
| */ | ||
| public function getPriority() | ||
| { | ||
| // should be run before CombineConsecutiveUnsetsFixer. |
There was a problem hiding this comment.
btw,
if it's not a sentence, why final dot is there?
BackEndTea
force-pushed
the
feature/no-unset-properties
branch
from
a9745a6
to
f21f318
Compare
|
I updated doc-blocks, and made sure comments stay preserved. One edge case that i dont know how to fix is the following: - unset/*foo*/($a, $this->a);
+ unset(/*foo*/$a); $this->a = null;If there is a comment between the |
|
the move of a bad placed comment to another not so perfect location is not an issue IMHO, as long as we preserve the comment it is up to the end user to fix the mess ;) |
BackEndTea
force-pushed
the
feature/no-unset-properties
branch
from
8911a36
to
c59ecad
Compare
BackEndTea
changed the title
|
The Cirlce CI failure looks unrelated to my PR, and i think this fixer is ready. |
|
while definitely good rule, it is changing code behaviour if one is iterating over instance properties please mark fixer as risky with damn good description when it's risky. |
| @@ -1002,6 +1002,10 @@ Choose from the list of available rules: | |||
|
|
|||
| *Risky rule: modifies the signature of functions; therefore risky when using systems (such as some Symfony components) that rely on those (for example through reflection).* | |||
|
|
|||
| * **no_unset_on_property** | |||
There was a problem hiding this comment.
please add this rule to local .php_cs.dist file
| public function getDefinition() | ||
| { | ||
| return new FixerDefinition( | ||
| 'Properties should be set to null instead of using `unset`.', |
There was a problem hiding this comment.
please update to `null`
|
|
||
| return array_merge( | ||
| [ | ||
| $filteredTokens[0]->isWhitespace() ? new Token([T_WHITESPACE, ' ']) : new Token(''), |
There was a problem hiding this comment.
can we avoid adding an empty token to this?
keradus
force-pushed
the
feature/no-unset-properties
branch
from
90ad623
to
3f685f1
Compare
|
Thank you @BackEndTea. |
|
Thank you @keradus @SpacePossum |
|
thank you @BackEndTea , you did all the work here :D great to see this in! |
Fixes #3695
This adds the fixer as described in #3695