Skip to content

Add missing descriptors for SmallFloatType and EnumType #656

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

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

Conversation

janedbal
Copy link
Contributor

@janedbal janedbal commented Apr 24, 2025

@janedbal janedbal force-pushed the smallfloat-descriptor branch from 2082cc3 to 2e8b2b6 Compare April 24, 2025 14:03
path: src/Type/Doctrine/Descriptors/SmallFloatType.php

-
message: '#^Class Doctrine\\DBAL\\Types\\EnumType not found\.$#'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first, I tried adding new doctrine versions to composer.json, but it would require bigger effort to adjust this repo to support those everywhere, so I just referenced those not-yet-existing classes.

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.

  • tests describing what this actually fixes would be nice


namespace PHPStan\Type\Doctrine\Descriptors;

class SmallFloatType extends FloatType
Copy link
Member

Choose a reason for hiding this comment

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

No inheritance please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? I believe SmallFloatType IS FloatType in terms of type descriptor.

use PHPStan\Type\StringType;
use PHPStan\Type\Type;

class EnumType implements DoctrineTypeDescriptor
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I wrong or without this type the data is considered as mixed ?

Now with this, it will report error foo be 'A'|'B' but is string on level 8 because the values option is not considered in the following code

#[ORM\Column(name: 'foo', type: Types::ENUM, options: ['values' => ['A', 'B'])]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Am I wrong or without this type the data is considered as mixed ?

You are correct.

@janedbal
Copy link
Contributor Author

tests describing what this actually fixes would be nice

It is pretty painful to test new doctrine here (due to all the compatibility hacks). But added.

@janedbal janedbal force-pushed the smallfloat-descriptor branch from eca8f6c to a59b33b Compare June 13, 2025 13:06
Copy link
Contributor

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

The enum type will introduce new false-positive.

Before

/**
   * @Column(type="enum", options={"values"={"a", "b", "c"}})
   * @var 'a'|'b'|'c'
   *
   public $enum;

Was reported level 9 (mixed is not 'a'|'b'|'c')
and now will be reported on a lower level (string is not 'a'|'b'|'c')

Would it be possible to read the options ?

@janedbal
Copy link
Contributor Author

Would it be possible to read the options ?

Yes, implemented.

@VincentLanglet
Copy link
Contributor

VincentLanglet commented Jun 13, 2025

Would it be possible to read the options ?

Yes, implemented.

Great.

Could add a test for the EntityColumnRule too

class EntityColumnRuleTest extends RuleTestCase

(I assume we just need to add an enum field in the BrokenEntity https://github.com/phpstan/phpstan-doctrine/blob/a1a9efb64708580a9d8b0d150340f7777d2b8aa0/tests/Rules/Doctrine/ORM/data/MyBrokenEntity.php)

I assume that

/**
   * @Column(type="enum", options={"values"={"a", "b", "c"}})
   * @var 'a'|'b'|'c'
   *
   public $enum;

shouldn't be reported but

/**
   * @Column(type="enum", options={"values"={"a", "b", "c"}})
   * @var string
   *
   public $enumWithUnprecisedPHPDoc;

should be reported

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.

Doctrine DBAL 4.2 ships an Types::ENUM which should be preferred over implementing custom types for ENUM fields.
3 participants