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

Enum support in query type inference #348

Merged
merged 6 commits into from
Jul 13, 2022
Merged

Conversation

arnaud-lb
Copy link
Contributor

@arnaud-lb arnaud-lb commented Jul 3, 2022

Fixes #344

Doctrine behavior

For reference:

Selecting an enum column directly returns an enum case (an instance of the enum class).

Using an enum column in an expression returns the raw value returned by the database driver. This is consistent with other column types (e.g. using a datetime column in an expression returns a string).

phpstan-doctrine behavior

In this PR I add support for enums as described above.

Since we know all the possible values of an enum, we can refine the database column type to a union of these values.

Examples

With the following classes:

#[Entity]
class X {
    #[Column]
    public Foo $f;
}

enum Foo: string {
    case A = 'a';
    case B = 'b';
}
--  Returns results of type array{f: Foo}
SELECT x.f FROM X

-- Returns results of type array{1: 'a'|'b'|''}
SELECT COALESCE(x.f, '') FROM X

Doctrine 2.11 issues

Enum support was introduced in Doctrine 2.11, but there was issues such as doctrine/orm#9622.

Here I add support for enums as implemented in Doctrine >= 2.12.

We have multiple options to handle Doctrine 2.11:

  • Do nothing: Ignore Doctrine 2.11 issues. For users with Doctrine 2.11, type inference will be lying.
  • Add special cases for 2.11
  • Conflict with 2.11

I chose the third option here. I think that option two would be acceptable as well, but this adds complexity just for a minor version. WDYT ?

@arnaud-lb arnaud-lb changed the title Enum support in query type inference [wip] Enum support in query type inference Jul 3, 2022
@arnaud-lb arnaud-lb force-pushed the enums branch 2 times, most recently from 02fe1c7 to 1137fc0 Compare July 6, 2022 20:35
@ondrejmirtes
Copy link
Member

About these options:

We have multiple options to handle Doctrine 2.11:

  1. Do nothing: Ignore Doctrine 2.11 issues. For users with Doctrine 2.11, type inference will be lying.
  2. Add special cases for 2.11
  3. Conflict with 2.11

I think option 1) is the best. Conflict is too aggressive, if the user isn't using enums at all, the conflict wouldn't make sense for them 😊 So I think we can ignore the issue, it's not our bug to fix 😊

@arnaud-lb arnaud-lb force-pushed the enums branch 5 times, most recently from b744b58 to 31c033b Compare July 6, 2022 21:41
Comment on lines +3 to +8
if (\PHP_VERSION_ID < 80100) {
if (interface_exists('BackedEnum', false)) {
return;
}

interface BackedEnum extends UnitEnum
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +20 to +22
bootstrapFiles:
- stubs/runtime/Enum/UnitEnum.php
- stubs/runtime/Enum/BackedEnum.php
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes supporting PHP versions bellow 8.1 much easier

Specifically, this allows code such as these to pass analysis:

is_subclass_of($className, BackedEnum::class)
/** @return class-string<BackedEnum> */

Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to ignore these runtime stubs in .gitattributes :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done 👍

Comment on lines +232 to +234
* @return iterable<string,array{Type,string,2?:string}>
*/
public function getTestData(): array
public function getTestData(): iterable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this data provider to yield test cases so that I could return some cases conditionally.

Best reviewed with ?w=1 :D

Comment on lines +514 to +516
// https://github.com/doctrine/orm/issues/9622
if (!$this->isDoctrine211()) {
yield 'enum' => [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PHP 8.1 build with lowest dependencies happens to run Doctrine 2.11

@arnaud-lb arnaud-lb changed the title [wip] Enum support in query type inference Enum support in query type inference Jul 6, 2022
@arnaud-lb arnaud-lb marked this pull request as ready for review July 6, 2022 21:51
composer.json Outdated
@@ -27,6 +27,7 @@
"doctrine/persistence": "^1.3.8 || ^2.2.1",
"nesbot/carbon": "^2.49",
"nikic/php-parser": "^4.13.2",
"ocramius/package-versions": "*",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need this to skip some test when running Doctrine 2.11

Copy link
Member

Choose a reason for hiding this comment

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

There's now a native Composer feature for this so we don't need a package I think? https://getcomposer.org/doc/07-runtime.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, didn't know that. I've updated the PR to use this instead.

Copy link
Member

Choose a reason for hiding this comment

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

We can remove this from composer.json now? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, gone too fast and forgot this. I've removed it now

Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

Otherwise 👍 :)

Comment on lines +20 to +22
bootstrapFiles:
- stubs/runtime/Enum/UnitEnum.php
- stubs/runtime/Enum/BackedEnum.php
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to ignore these runtime stubs in .gitattributes :)

composer.json Outdated
@@ -27,6 +27,7 @@
"doctrine/persistence": "^1.3.8 || ^2.2.1",
"nesbot/carbon": "^2.49",
"nikic/php-parser": "^4.13.2",
"ocramius/package-versions": "*",
Copy link
Member

Choose a reason for hiding this comment

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

There's now a native Composer feature for this so we don't need a package I think? https://getcomposer.org/doc/07-runtime.md

@ondrejmirtes ondrejmirtes merged commit e6a149b into phpstan:1.3.x Jul 13, 2022
@ondrejmirtes
Copy link
Member

Thank you!

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.

Enum type not correctly detected in DQL query result
2 participants