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

Be less restrictive in DiscriminatorColumnMapping phpdoc #11226

Merged
merged 5 commits into from Feb 22, 2024

Conversation

VincentLanglet
Copy link
Contributor

Currently static analysis returns error when writing

$classMetadata->setDiscriminatorColumn((array) $discriminatorColumnMapping);

with errors like the fact that the array need to have keys like length?: int but received length?: int|null.

But since DiscriminatorColumnMapping is using null as default value, it should allow both

  • A missing key
  • A key with null value

* length?: int,
* fieldName?: string|null,
* type?: string|null,
* length?: int|null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DiscriminatorColumnMapping use

public int|null $length = null;

so a null length seems valid.

* type?: string,
* length?: int,
* fieldName?: string|null,
* type?: string|null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an isset check

if (! isset($columnDef['type'])) {
                $columnDef['type'] = 'string';
            }

so null is handled.

* fieldName?: string,
* type?: string,
* length?: int,
* fieldName?: string|null,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is an isset check

if (! isset($columnDef['fieldName'])) {
                $columnDef['fieldName'] = $columnDef['name'];
            }

so null is handled

src/Mapping/ClassMetadata.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

greg0ire commented Feb 6, 2024

This should target 3.1.x I think, since it's no bugfix.

@VincentLanglet
Copy link
Contributor Author

This should target 3.1.x I think, since it's no bugfix.

This would help some projet to add easily support for ORM v3 without having to ignore multiple static analysis error.

Also, the fact we added

$columnDef['options'] ??= [];

is kinda a bugfix.

The phpdoc was saying a null options was accepted, but it wasn't since DiscriminatorColumnMapping::fromMappingArray didn't accept a null options.

@greg0ire
Copy link
Member

greg0ire commented Feb 6, 2024

The phpdoc was saying a null options was accepted, but it wasn't since DiscriminatorColumnMapping::fromMappingArray didn't accept a null options.

🤔 you're right I think, so it's weird that the baseline is not reduced as a consequence.

Since your changes are not very substantial, I'm OK with staying on 3.0.x if that helps.

derrabus
derrabus previously approved these changes Feb 6, 2024
Copy link
Member

@derrabus derrabus left a comment

Choose a reason for hiding this comment

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

👍🏻 once PHPCS is happy.

src/Mapping/ClassMetadata.php Outdated Show resolved Hide resolved
src/Mapping/DiscriminatorColumnMapping.php Outdated Show resolved Hide resolved
@derrabus derrabus added this to the 3.0.1 milestone Feb 6, 2024
@VincentLanglet
Copy link
Contributor Author

👍🏻 once PHPCS is happy.

Done :)

@derrabus derrabus merged commit a5bf9bb into doctrine:3.0.x Feb 22, 2024
64 checks passed
derrabus added a commit to derrabus/orm that referenced this pull request Feb 22, 2024
* 3.0.x:
  Test different ways of settings query parameters
  Be less restrictive in DiscriminatorColumnMapping phpdoc (doctrine#11226)
  Allow (Array)ParameterType in QueryBuilder
derrabus added a commit that referenced this pull request Feb 22, 2024
* 3.1.x:
  Backport QueryParameterTest (#11288)
  Test different ways of settings query parameters
  Be less restrictive in DiscriminatorColumnMapping phpdoc (#11226)
  Allow (Array)ParameterType in QueryBuilder
  Do not implicitly cast glob's return type
  Do not cast file_put_contents's return type
  Do not implicitly cast getLockTime()'s return type
  Do not implicitly cast getLockContent()'s return value
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

3 participants