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

[Autocomplete] Add support for doctrine/orm:^3.0 #1468

Merged
merged 1 commit into from Feb 15, 2024

Conversation

evertharmeling
Copy link
Contributor

@evertharmeling evertharmeling commented Feb 7, 2024

Q A
Bug fix? no
New feature? yes-ish?
Issues Replaces #1459
License MIT

Follow up on #1459 as it seemed a bit more complicated :)

Because in the current getPropertyMetadata function, see and below:

public function getPropertyMetadata(string $propertyName): array
{
    if (\array_key_exists($propertyName, $this->metadata->fieldMappings)) {
        return $this->metadata->fieldMappings[$propertyName];
    }

    if (\array_key_exists($propertyName, $this->metadata->associationMappings)) {
        return $this->metadata->associationMappings[$propertyName];
    }

    throw new \InvalidArgumentException(sprintf('The "%s" field does not exist in the "%s" entity.', $propertyName, $this->metadata->getName()));
}

The function combines getting metadata from fieldMappings and associationMappings, that both return an array in Doctrine ORM 2.

This however changes in Doctrine ORM 3 in fieldMappings returning a FieldMapping object and associationMappings in an AssociationMapping object.

To support both Doctrine ORM 2 and 3, I didn't changed the return type of the function for now, as we would otherwise need to bump the major version for the Autocomplete component.

To ease the transition to Doctrine ORM 3 in the future, I've splitted getPropertyMetadata in getFieldMetadata and getAssociationMetadata.

TODO

  • Should we keep the getPropertyMetadata() function as a proxy to the new methods? (I removed it for now as the method shouldn't be used directly as mentioned.
  • Should we retroactively add the @internal the src/Autocomplete/src/Doctrine/EntityMetadata.php class also (as mentioned)?
  • Test on Doctrine ORM 3, currently blocked by feat: support doctrine/orm 3 zenstruck/foundry#556

@carsonbot carsonbot added the Status: Needs Review Needs to be reviewed label Feb 7, 2024
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

Thanks for getting this going - I think I answered the pending questions

src/Autocomplete/composer.json Show resolved Hide resolved
// In doctrine/orm:^3.0; $metadata will be a FieldMapping object
if (!\is_array($metadata)) {
return (array) $metadata;
}
Copy link
Member

Choose a reason for hiding this comment

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

For ORM 3, won't this return [0 => FieldMapping]? But in 2.9, it would be an associative array containing the metadata? Or am I misunderstanding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes in ORM 3, the ClassMetaData::$fieldMappings is of type array<string, FieldMapping> (see) and in ORM 2 it's just an array (mixed in docs, see), it's been defined as a psalm-type (see), that may cause the confusion?

Since we access the ClassMetaData::$fieldMappings with a 'key' it will return a FieldMapping object directly and casting it to an array works for the FieldMapping object as it is defined using public properties.

@weaverryan weaverryan added Status: Needs Work Additional work is needed and removed Status: Needs Review Needs to be reviewed labels Feb 9, 2024
@carsonbot carsonbot added Status: Needs Review Needs to be reviewed and removed Status: Needs Work Additional work is needed labels Feb 15, 2024
@evertharmeling
Copy link
Contributor Author

Thanks for reviewing Ryan and Simon! Sorry for the late response, but the flu hit me and my little one got the chickenpox, it was a messy week 😅

@weaverryan
Copy link
Member

Thanks Evert! We'll test on ORM 3 when Foundry adds support

@weaverryan weaverryan merged commit 2ba75f6 into symfony:2.x Feb 15, 2024
14 of 15 checks passed
@smnandre
Copy link
Collaborator

Thanks for reviewing Ryan and Simon! Sorry for the late response, but the flu hit me and my little one got the chickenpox, it was a messy week 😅

Thank you for this, and i hope everything is better now for you & familly 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Review Needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants