bug: Exclude $this from TernaryToNullCoalescingFixer

[kylekatarnls]

Fix #7051

[Wirone]

@kylekatarnls please make CI green, no one is going to do code review without it 🙂. You should run ./php-cs-fixer fix locally to apply Fixer's CS.

[kylekatarnls]

I like Star Wars, but Yoda-style advantage is not worth the readability compromise for me 🤣

[SpacePossum]

nice work @kylekatarnls 👍

suggestion:

        // search what is inside the isset()
        $issetTokens = $this->getMeaningfulSequence($tokens, $startBraceIndex, $endBraceIndex);

        if (\count($issetTokens) === 1) {
            if ($issetTokens[0]->isGivenKind(T_VARIABLE) && '$this' === strtolower($issetTokens[0]->getContent())) {
                return; // do not fix `isset($this) ? x : y` as it will break code
            }
        } elseif ($this->hasChangingContent($issetTokens)) {
            return; // some weird stuff inside the isset
        }

        // search what is inside the middle argument of ternary operator

and adding the test will maybe explain why my suggestion:

['<?php $x = isset($THIS) ? $this : null;'],

[kylekatarnls]

OK for the case sensitivity. Done + test case added.

I note that the case-sensitivity is not taken into account neither for issetCode !== ternaryFirstOperand check, is it intentional that:

• isset($var) ? $var : becomes $var ?? but
• isset($var) ? $VAR : remains unmodified?

I see you also propose to check token value rather than generated code, is it for performance reason? Because we run $issetTokens->generateCode() later in the function anyway and the case where it's $this is likely super rare so I think it's not worth the extra reading complexity. I changed the code already to save $issetTokens->generateCode() result in a variable so you can see more easily what I mean. WDYT?

[SpacePossum]

OK for the case sensitivity. Done + test case added.

Thank you!

I note that the case-sensitivity is not taken into account neither for issetCode !== ternaryFirstOperand check, is it intentional that:

It is case sensitive now and should be I think. Example

<?php

$var = 2;
$VAR = 1;

$a = isset($var) ? $VAR : 1;

var_dump($a);

$a = $var ?? 1;

var_dump($a);

int(1)
int(2)

so changing the code would yield different results at runtime so must be avoided.

I see you also propose to check token value rather than generated code, is it for performance reason?

Partly, but also it is more easy to understand/debug as we try to work with tokens where ever possible. However this is just a preference here of mine, so others may think different.

and the case where it's $this is likely super rare so I think it's not worth the extra reading complexity.

I see your point, I reason however that because it is so rare we should use as little cpu cycles as possible to detect it.

I think this PR looks good 👍

Can a member kick of the CI for this PR?

[Wirone]

I believe this requires integration test, so fixer can be run on actual code, not on tokens only. Also, we probably should cover more complex scenarios like isset($this->foo), isset($this['foo']), because in those cases $this should be kept in isset() too.

[kylekatarnls]

I believe this requires integration test, so fixer can be run on actual code, not on tokens only. Also, we probably should cover more complex scenarios like isset($this->foo), isset($this['foo']), because in those cases $this should be kept in isset() too.

I don't think this is an issue. We do an exact check of what is inside the isset() code: '$this' === $issetCode so the risk of regression here is super-low, it would mean the calculation of $endBraceIndex is broken which would have a way bigger impact than just this edge-case about $this.

That's also why IMHO an integration test is overkill for this. But I ticked Allow edits by maintainers so feel free to add/edit anything you want.

[Wirone]

But I ticked Allow edits by maintainers so feel free to add/edit anything you want.

I still have ~100 issues to verify (along with other 100+ already verified, but not resolved), and ~60 PRs to handle, I would really love if you could cover mentioned scenarios on your own 😅.

[kylekatarnls]

But I ticked Allow edits by maintainers so feel free to add/edit anything you want.

I still have ~100 issues to verify (along with other 100+ already verified, but not resolved), and ~60 PRs to handle, I would really love if you could cover mentioned scenarios on your own 😅.

Same, spare time already full of OSS work. But first let's clarify as I think I should not waste more energy until the exact issue is clearly understood, $this->foo, $this['foo'], and $thisIsSparta are out of scope, thus integrations tests for the very edge-case of $this should just be a nice-to-have I think, not blocking merging the fix.

[Wirone]

I still think it's more like a bug that $this works differently in isset() than regular variables, but considering current behavior this PR looks OK.

@kubawerlos will you take a look?

[Wirone]

PS. I've deleted @kubawerlos duplicated comment 🙂.

[Wirone]

Thank you @kylekatarnls 🍻