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

DX: Prevent having deprecated fixers listed as successors of other deprecated fixers #7967

Merged
merged 3 commits into from
Apr 25, 2024

Conversation

Wirone
Copy link
Member

@Wirone Wirone commented Apr 23, 2024

Revived #7960, credits to @kubawerlos.

In terms of this discussion, I believe the point was valid but maybe not articulated correctly. As you can see, AbstractFixerTestCase::testDeprecatedFixersDoNotHaveDeprecatedSuccessor() iterates over the result of DeprecatedFixerInterface::getSuccessorsNames() and then calls TestCaseUtils::getFixerByName() (previously this method was a part of TestCase). The problem here is that getSuccessorsNames()'s return type is defined as list<string> and there is not any mechanism that would verify if successors are valid references to existing fixers. For example, when we do:

public function getSuccessorsNames(): array
{
    return array_merge(array_keys($this->proxyFixers), ['foo']);
}

the docs are generated properly, Auto Review tests pass, everything looks OK, but then testDeprecatedFixersDoNotHaveDeprecatedSuccessor() fails with TypeError: PhpCsFixer\Tests\Test\TestCaseUtils::getFixerByName(): Return value must be of type PhpCsFixer\Fixer\FixerInterface, null returned.

image

With the explicit check, as suggested in the original PR, it just looks better and is more descriptive:

image

PS. Maybe we should have auto-review test that would verify if defined successors are valid fixers? (should be done in separate PR, if needed)

kubawerlos and others added 3 commits April 23, 2024 22:24
I know the other `Utils` class is deprecated, but in this case it feels OK to have it here, at least for now. This way it can be used only where actually needed, instead of being available in every test case.
@Wirone Wirone self-assigned this Apr 23, 2024
@coveralls
Copy link

Coverage Status

coverage: 96.113% (+0.08%) from 96.03%
when pulling 18f160a on Wirone:codito/revive-7960
into 37d8d45 on PHP-CS-Fixer:master.

@Wirone Wirone enabled auto-merge (squash) April 23, 2024 21:17
}
}

if (!\array_key_exists($name, $fixers)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isset is more performant I think:

Suggested change
if (!\array_key_exists($name, $fixers)) {
if (!isset($fixers[$name])) {

Copy link
Member Author

@Wirone Wirone Apr 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, "nice to have" suggestions don't work nicely with auto-merge 😅.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion - ignore this single usage, confirm what is more performant, apply locally with CI check

@Wirone Wirone merged commit 6ec7005 into PHP-CS-Fixer:master Apr 25, 2024
28 checks passed
@Wirone Wirone deleted the codito/revive-7960 branch April 25, 2024 07:20
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

5 participants