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

Implement Scope->getPhpVersion() #3642

Open
wants to merge 11 commits into
base: 2.0.x
Choose a base branch
from

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Nov 17, 2024

No description provided.

@staabm
Copy link
Contributor Author

staabm commented Nov 17, 2024

sending it up for review to sync our expectations. if this works for you, I can add it to all rules.

when rules take the php-version from the Scope, we also need to think how we can/want control the PhpVersion in tests, so we can test PhpVersion dependent behaviour

if (!is_int($scalar)) {
throw new ShouldNotHappenException();
}
$ints[] = $scalar;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this piece of code I realized it's time to do an idea I had for a long time. Please follow my train of thought (while I'm typing this out on a train to Prague :))

Often there's not a single PHP version, but a range of them so returning bool from a method like this isn't exactly right:

	public function supportsNullCoalesceAssign(): bool
	{
		return $this->versionId >= 70400;
	}

If we're analysing against both PHP 7.3 and 7.4+, we MIGHT support null coalesce assign.

If we're analysing only against PHP 7.3, we do NOT support null coalesce assign.

If we're analysing only against PHP 7.4+, we DO support null coalesce assign.

Sounds familiar? :) We should return TrinaryLogic instead of bool.

But I don't want the headache of BC breaks. Adding a new method about PHP version to Scope allows us to return something else than PhpVersion. A new unrelated class called something like TrinaryPhpVersion or PhpVersions. And we do not need to add all methods from PhpVersion on it. We can add them slowly as needed.

So in this PR this new class could have just producesWarningForFinalPrivateMethods(): TrinaryLogic correctly computed for all various scenarious of what PHP_VERSION_ID can be in that place.

And the rule would correctly handle yes/no/maybe along with some tests.

The constructor of this class could just accept a list of supported PHP versions like 80000, 80100, 80200.... Because all methods in PhpVersion asking about this number always ask about the minor version, ending in 00. Meaning that if PHP_VERSION_ID is 80210, the 10 at the end would have to be trimmed and exchanged for 00 to correctly answer.

Does this sound compelling to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah I can follow this train :).

when rules take the php-version from the Scope, we also need to think how we can/want control the PhpVersion in tests, so we can test PhpVersion dependent behaviour

this is still a open question though. how to adjust existing tests so we can force the php version?

wdyt about adding RuleTestCase->getPhpVersions() with a default impl and handle it similar to getCollectors?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need anything like that. If we want to test behaviour for a specific PHP version we will just write the condition if (PHP_VERSION_ID > ...) in the tested code.

@staabm staabm force-pushed the scope-phpversion branch 4 times, most recently from 8c3d347 to 215d295 Compare November 17, 2024 17:20
@staabm staabm marked this pull request as ready for review November 17, 2024 17:23
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

src/Php/PhpVersion.php Outdated Show resolved Hide resolved

private function minPhpVersion(int $versionId): TrinaryLogic
{
if ($this->minVersionId >= $versionId) {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't sufficient. Let's say something is supported only between 7.2 and 7.3. If our intervals only allow < 7.1 and > 8.0, it's not okay to collapse all the intervals, it loses the information.

I know this is just theoretical but let's address this now before this becomes a problem in the future.

Copy link
Contributor Author

@staabm staabm Nov 19, 2024

Choose a reason for hiding this comment

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

I just realized we could just use the all-problem-solving Type->isSuperTypeOf, which means the PhpVersions class effectively no longer contains real logic, which requires a separate unit-test, right?

Copy link
Member

Choose a reason for hiding this comment

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

I still want a test that tests the public method for no/maybe/yes. Thanks.

src/Analyser/Scope.php Outdated Show resolved Hide resolved
src/Analyser/MutatingScope.php Outdated Show resolved Hide resolved
src/Php/PhpVersions.php Show resolved Hide resolved
src/Php/PhpVersions.php Show resolved Hide resolved
@staabm staabm force-pushed the scope-phpversion branch 3 times, most recently from f35e891 to f32eec3 Compare November 20, 2024 09:51
*/
public function testRule(int $phpVersion, array $errors): void
{
$this->phpVersionId = $phpVersion;
Copy link
Member

Choose a reason for hiding this comment

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

Please do not touch existing test, I can't read this diff. It'd be better to add a completely new file with the top-level ifs. Thanks.

Copy link
Contributor Author

@staabm staabm Nov 20, 2024

Choose a reason for hiding this comment

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

I now tried to keep the old test as much as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants