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

feat: support doctrine/orm 3 #556

Closed
wants to merge 1 commit into from
Closed

feat: support doctrine/orm 3 #556

wants to merge 1 commit into from

Conversation

kbond
Copy link
Member

@kbond kbond commented Feb 5, 2024

TODO:

  • sqlite changes. We don't currently test but there are some breaking changes when using ResetDatabase with sqlite

@kbond kbond mentioned this pull request Feb 5, 2024
@nikophil
Copy link
Member

nikophil commented Feb 5, 2024

won't it be a PITA to maintain tests for both versions?

@kbond
Copy link
Member Author

kbond commented Feb 5, 2024

Possibly, but if we bump the min orm to latest 2.x, perhaps it won't be a problem?

@kbond
Copy link
Member Author

kbond commented Feb 7, 2024

I think some of the work done in symfony/maker-bundle#1439 could help us here.

weaverryan added a commit to symfony/ux that referenced this pull request Feb 15, 2024
…harmeling)

This PR was squashed before being merged into the 2.x branch.

Discussion
----------

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

| 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](https://github.com/symfony/ux/blob/2.x/src/Autocomplete/src/Doctrine/EntityMetadata.php#L44) and below:

```php
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
- [x] 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](#1459 (comment)).
- [x] Should we retroactively add the ``@internal`` the `src/Autocomplete/src/Doctrine/EntityMetadata.php` class also (as [mentioned](#1459 (comment)))?
- [ ] Test on Doctrine ORM 3, currently blocked by zenstruck/foundry#556

Commits
-------

20f4dad [Autocomplete] Add support for doctrine/orm:^3.0
@jrushlow
Copy link
Contributor

MakeTestRunner::configureDatabase() has the logic we needed to get sqlite to work with the new orm - https://github.com/symfony/maker-bundle/blob/d9574302ee67c23ae5c803556f2b779c3b0b94a2/src/Test/MakerTestRunner.php#L156

@nikophil
Copy link
Member

closed in favor of #569

@nikophil nikophil closed this Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants