[Autocomplete] Add conflict on doctrine/orm:^3.0 #1459
Conversation
|
I don't think we should add a composer conflict there, as the Autocomplete does not require doctrine-orm. Probably more a runtime message (if really it cannot be used with ORM 3.0) We should neither restrict any dependencies based on the internal toolkit used to test/develop it, as it would bring limitations in userland based on something they probably do not use or need. With the change you suggest, would this component be usable in production in an app using Doctrine ORM 3 ? If yes, let's create a PR on that, wdyt ? What do you guys think about that ? @weaverryan @kbond is there already a position about Doctrine 3 in the symfony/symfony repo ? |
Sorry if was unclear, the error occurs when you select an autocomplete field and start filtering, so it's not that it's happing in the testsuite (only). Therefor it would currently break applications with an autocomplete field implemented and upgrading to Doctrine ORM 3... That's why I went for the 'easy' fix of adding the conflict which would then prevent this problem and we would then have time to add proper support for it (and then remove the conflict again).
Sure, only thing is that our testsuite ( |
|
Thanks for this! I'm also leaning toward "let's just fix this now" if reasonable. For the EntityMetadata, it looks like it might make sense to change the return value from an array to FieldMapping, no? If we ignore BC on that method for a moment, would that fix things? If so, I might be ok with introducing that small BC break. The thing is: if you're calling this method directly (I really should have made it internal) and you're on orm 3, then us returning an array doesn't make sense and will likely break your app. What the method was intended to do was "return the metadata from Doctrine, whatever that format". |
Sorry if i missread it first :) Doctrine ORM 3.0 released just a couple of days ago, right ? So you're right we must do something now : either adding a conflict or fixing it, before anyone accuses us to "hide" some incompatibilities. Is your "hacky" workaround really "hacky" ... or is it something we'd have to do anyway to support ORM 3 ? |
|
A conflict will not work: If you add the conflict in 2.15.0, Composer will always fallback to installing 2.14.0 as it didn't declare the conflict (if it's within the allowed version range) So fixing it now or throwing a runtime exception is the only possible way forward. |
|
Good call @wouterj, I wasn’t aware of that… Then the easiest way will be to introduce the ‘small’ BC break by changing the return type from ‘array’ to ‘FieldMapping’ as @weaverryan suggested. Hopefully its the only change needed. Will take a look at it tomorrow, but feel free to beat me to it 😉 |
|
closed in favor of #1468 |
…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
Since doctrine/orm:3.0 is released and there is no direct dependency defined in the
Autocomplete-component ondoctrine/orm, you're able to install it in your project. However when doing so it will result in an error when using the autocomplete:A hacky workaround fix in
\Symfony\UX\Autocomplete\Doctrine\EntityMetadata:47to keep BC, could be:But also since
zenstruck/foundrydoesn't supportdoctrine/orm:^3.0yet, we would not be able to run the testsuite and see any other incompatibilities in the component. So we need to take a closer look for all implications.So for now it's probably wise to add a conflict on
doctrine/orm:^3.0until we are able to confidently add support for it. Alternatively we could also add a direct dependency ondoctrine/orm:^2.9.4(instead of only a dev dep)?doctrine/annotations:^1.0which requiresdoctrine/lexer:^1 || ^2which does not allowdoctrine/orm:^3.0.