Skip to content

Allow null as a return value for association's target#192

Merged
alcaeus merged 1 commit into2.2.xfrom
malarzm-patch-1
Jun 22, 2021
Merged

Allow null as a return value for association's target#192
alcaeus merged 1 commit into2.2.xfrom
malarzm-patch-1

Conversation

@malarzm
Copy link
Member

@malarzm malarzm commented Jun 15, 2021

This came up in doctrine/mongodb-odm#2325 (cc @franmomu @alcaeus ). For the background:

  1. ODM has a concept of a discriminator map for references which allows to specify multiple classes a reference can point to, this setting must not be combined with targetDocument
  2. In ODM you can omit targetDocument and discriminatorMap altogether to have a reference to any mapped document. Behind the scenes ODM is using FQCN as a discriminator's value

The change as-is is needed to prevent a break when doctrine/persistence becomes typed. The other solution I see is allowing the method to return an array of FQCNs that would allow ODM to reflect discriminator maps in the method's return value. This would cause a minor inconvenience for ORM I believe, but nothing unseen before (I'm looking at isSingleValuedAssociation and similar thing for identifiers).

@alcaeus
Copy link
Member

alcaeus commented Jun 16, 2021

No objection to the change in general, but I feel like the two PHPStan errors point to potential downstream breaks. @greg0ire @franmomu what do you think?

@greg0ire
Copy link
Member

I think it looks a bit like: doctrine/orm#8767 (comment)

Now that we are able to add type information through phpdoc and check it with SA tools, there is going to be a phase where we notice all the inconsistencies in how we implement our interfaces.

It is still compatible to say the implementation in the ORM still returns something non-nullable, so here the solution to the breakage will be to override the signature to just say that.

@franmomu
Copy link
Contributor

It is still compatible to say the implementation in the ORM still returns something non-nullable, so here the solution to the breakage will be to override the signature to just say that.

I agree with this, I guess overriding it in the ORM would be enough, I don't think many packages rely on ClassMetadata of persistence.

By the way, apparently PersistenObject should have been removed long time ago.

Copy link
Member

@alcaeus alcaeus left a comment

Choose a reason for hiding this comment

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

LGTM with static analysis failures addressed: we need to explicitly check for null before using instanceof

@malarzm malarzm requested a review from alcaeus June 21, 2021 18:23
@malarzm malarzm added this to the 2.2.2 milestone Jun 21, 2021
@alcaeus alcaeus self-assigned this Jun 22, 2021
@alcaeus alcaeus added the Bug Something isn't working label Jun 22, 2021
@alcaeus alcaeus merged commit ee36ff3 into 2.2.x Jun 22, 2021
@alcaeus alcaeus deleted the malarzm-patch-1 branch June 22, 2021 11:39
@alcaeus
Copy link
Member

alcaeus commented Jun 22, 2021

Thanks @malarzm!

@VincentLanglet
Copy link
Contributor

@greg0ire I now have a phpstan error, using orm, because class-string|null can be returned.

Does https://github.com/doctrine/orm/blob/2.9.x/lib/Doctrine/ORM/Mapping/ClassMetadataInfo.php#L3414 can really return null ? If not, phpdoc should be added.

@stof
Copy link
Member

stof commented Aug 11, 2021

Based on the mapping validation in the same class, it cannot be null

@VincentLanglet
Copy link
Contributor

I opened doctrine/orm#8907 then

derrabus added a commit to derrabus/persistence that referenced this pull request Dec 19, 2021
* 2.3.x: (24 commits)
  Bump PHPStan and Psalm (doctrine#214)
  Update `ObjectRepository`, more type inference. (doctrine#213)
  PHPStan 1.0.1
  Reuse workflows from upstream
  Specify proxy type
  Undeprecate detach
  Fix class name
  Test on PHP 8.1
  Deprecate short namespace alias syntax
  Normalize continuous integration workflow
  Add explicit `@return` type next to `#[ReturnTypeWillChange]`
  Describe criteria as array with string keys
  Allow null as return value for type of field
  Mark doctrine/annotations dependency as optional
  Allow null as a return value for association's target (doctrine#192)
  Add .github, .doctrine-project.json and phpstan-baseline.neon to export ignore
  Reordered property type resolution to resolve typed properties before untyped ones
  Merge 2.2.x into 2.3.x (doctrine#191)
  Don't pass null to strpos() (doctrine#190)
  Add ReturnTypeWillChange to extended reflection classes
  ...
derrabus added a commit that referenced this pull request Dec 19, 2021
* 2.3.x: (24 commits)
  Bump PHPStan and Psalm (#214)
  Update `ObjectRepository`, more type inference. (#213)
  PHPStan 1.0.1
  Reuse workflows from upstream
  Specify proxy type
  Undeprecate detach
  Fix class name
  Test on PHP 8.1
  Deprecate short namespace alias syntax
  Normalize continuous integration workflow
  Add explicit `@return` type next to `#[ReturnTypeWillChange]`
  Describe criteria as array with string keys
  Allow null as return value for type of field
  Mark doctrine/annotations dependency as optional
  Allow null as a return value for association's target (#192)
  Add .github, .doctrine-project.json and phpstan-baseline.neon to export ignore
  Reordered property type resolution to resolve typed properties before untyped ones
  Merge 2.2.x into 2.3.x (#191)
  Don't pass null to strpos() (#190)
  Add ReturnTypeWillChange to extended reflection classes
  ...
derrabus added a commit to derrabus/persistence that referenced this pull request Dec 19, 2021
* 2.3.x: (24 commits)
  Bump PHPStan and Psalm (doctrine#214)
  Update `ObjectRepository`, more type inference. (doctrine#213)
  PHPStan 1.0.1
  Reuse workflows from upstream
  Specify proxy type
  Undeprecate detach
  Fix class name
  Test on PHP 8.1
  Deprecate short namespace alias syntax
  Normalize continuous integration workflow
  Add explicit `@return` type next to `#[ReturnTypeWillChange]`
  Describe criteria as array with string keys
  Allow null as return value for type of field
  Mark doctrine/annotations dependency as optional
  Allow null as a return value for association's target (doctrine#192)
  Add .github, .doctrine-project.json and phpstan-baseline.neon to export ignore
  Reordered property type resolution to resolve typed properties before untyped ones
  Merge 2.2.x into 2.3.x (doctrine#191)
  Don't pass null to strpos() (doctrine#190)
  Add ReturnTypeWillChange to extended reflection classes
  ...
derrabus added a commit to derrabus/persistence that referenced this pull request Dec 19, 2021
* 2.3.x: (24 commits)
  Bump PHPStan and Psalm (doctrine#214)
  Update `ObjectRepository`, more type inference. (doctrine#213)
  PHPStan 1.0.1
  Reuse workflows from upstream
  Specify proxy type
  Undeprecate detach
  Fix class name
  Test on PHP 8.1
  Deprecate short namespace alias syntax
  Normalize continuous integration workflow
  Add explicit `@return` type next to `#[ReturnTypeWillChange]`
  Describe criteria as array with string keys
  Allow null as return value for type of field
  Mark doctrine/annotations dependency as optional
  Allow null as a return value for association's target (doctrine#192)
  Add .github, .doctrine-project.json and phpstan-baseline.neon to export ignore
  Reordered property type resolution to resolve typed properties before untyped ones
  Merge 2.2.x into 2.3.x (doctrine#191)
  Don't pass null to strpos() (doctrine#190)
  Add ReturnTypeWillChange to extended reflection classes
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants