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

Drop support for php < 8.1 #80

Merged
merged 1 commit into from
Dec 15, 2022
Merged

Conversation

greg0ire
Copy link
Member

No description provided.

@greg0ire greg0ire force-pushed the php8-syntax branch 4 times, most recently from 61dc00b to bf6b424 Compare November 29, 2022 12:19
* @return bool
*/
public function isA($value, $token)
public function isA(mixed $value, int|string $token)
{
return $this->getType($value) === $token;
Copy link
Member Author

@greg0ire greg0ire Nov 29, 2022

Choose a reason for hiding this comment

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

This is weird… isA() can be called with mixed, and then calls getType() which is typed as string. Which type declaration should get fixed? In the tests, there are calls to isA() with ints or floats:

$this->assertTrue($this->concreteLexer->isA(11, 'int'));
$this->assertTrue($this->concreteLexer->isA(1.1, 'int'));

Weirdly, static analysis does not have anything to say about this.
Note that this method is not used at all in the ORM or the ODM

Copy link
Member

Choose a reason for hiding this comment

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

Do we even expect something different than a string here? My preference would be to change the declaration of isA().

Copy link
Member

Choose a reason for hiding this comment

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

static analysis has nothing to say because phpstan runs at level 7, not at max level (i.e. 9). Until level 8, mixed behaves like Typescript's any: everything is valid with it. It is only at level 9 that it is treated like Typescript's unknown

@greg0ire greg0ire marked this pull request as ready for review November 29, 2022 12:32
derrabus
derrabus previously approved these changes Nov 29, 2022
composer.json Outdated Show resolved Hide resolved
tests/Doctrine/Common/Lexer/ConcreteLexer.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member Author

I'm waiting on the outcome of the discussion on #79 to decide what to do with this. If we still support PHP 7.1 on 2.0, it might be acceptable to bump to 2.0 on doctrine/orm "quickly" (in 2.15)

@greg0ire greg0ire marked this pull request as draft December 10, 2022 10:52
@greg0ire
Copy link
Member Author

Blocked by #96

@greg0ire greg0ire changed the base branch from 2.0.x to 3.0.x December 14, 2022 20:04
@greg0ire greg0ire marked this pull request as ready for review December 14, 2022 20:16
SenseException
SenseException previously approved these changes Dec 14, 2022
composer.json Outdated Show resolved Hide resolved
@greg0ire greg0ire merged commit 84a527d into doctrine:3.0.x Dec 15, 2022
@greg0ire greg0ire deleted the php8-syntax branch December 15, 2022 16:57
@greg0ire greg0ire added this to the 3.0.0 milestone Dec 15, 2022
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.

None yet

4 participants