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

Widen parameter type #10229

Merged
merged 1 commit into from
Nov 14, 2022
Merged

Conversation

greg0ire
Copy link
Member

@greg0ire greg0ire commented Nov 13, 2022

This exception is used in two places, one where $generatedMode is clearly a string, and another where typing it as a string will cause a type error, because $generatedMode is supposed to be an int there.

Supposed to be an int:

if (isset($mapping['generated'])) {
if (! in_array($mapping['generated'], [self::GENERATED_NEVER, self::GENERATED_INSERT, self::GENERATED_ALWAYS])) {
throw MappingException::invalidGeneratedMode($mapping['generated']);
}
if ($mapping['generated'] === self::GENERATED_NEVER) {
unset($mapping['generated']);
}
}

Supposed to be a string:

if (! defined('Doctrine\ORM\Mapping\ClassMetadata::GENERATED_' . $generatedMode)) {
throw MappingException::invalidGeneratedMode($generatedMode);
}

@mehldau this was introduced in #9118, please review

This exception is used in two places, one where $generatedMode is
clearly a string, and another where typing it as a string will cause a
type error, because $generatedMode is supposed to be an int there.
@SenseException
Copy link
Member

* @psalm-param array<string, mixed> $mapping The field mapping to validate & complete.

Isn't this supposed to be array<string, FieldMapping> or did I miss something? Maybe that's why the int could be removed without PHPStan complaining in the PR?

@greg0ire
Copy link
Member Author

greg0ire commented Nov 13, 2022

There is this comment that suggests FieldMapping should be used more widely, so maybe yeah

EDIT: also, you mean FieldMapping, not array<string, FieldMapping> 😉

@greg0ire
Copy link
Member Author

@SenseException I tried your suggestion, and no, that's not right, because then you get

 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php
 ------ --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
  1669   Offset 'fieldName' on array{type: string, fieldName: string, columnName: string, length?: int, id?: bool, nullable?: bool, notInsertable?: bool, notUpdatable?: bool, ...} in isset() always exists and is not nullable.

At that point, the mapping is supposed to be incomplete.

Copy link
Member

@SenseException SenseException left a comment

Choose a reason for hiding this comment

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

Then let's keep it the way it is.

@greg0ire greg0ire merged commit fc3201b into doctrine:2.13.x Nov 14, 2022
@greg0ire greg0ire deleted the fix-phpdoc-exception branch November 14, 2022 21:15
@greg0ire greg0ire added this to the 2.13.4 milestone Nov 14, 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.

2 participants