-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[DoctrineBridge] Deprecate auto-mapping of entities in favor of mapped route parameters #54455
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
Conversation
ec6d225
to
9d3ad33
Compare
I just confirmed on a real app that this solves all ambiguities while providing nice DX. |
068c12f
to
b8d9631
Compare
rebase needed and 👍 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
b8d9631
to
61f5dad
Compare
This comment was marked as outdated.
This comment was marked as outdated.
61f5dad
to
6342009
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
e0159b9
to
aabe821
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nicolas-grekas I absolutely love the solution you've come up with, this is awesome! 😃
I tested this by backporting the changes to a 6.4 project, everything seems to be working great there, and the old, now deprecated, automapping is still disabled as expected.
aabe821
to
e9e8e50
Compare
…etween a route parameter and its corresponding request attribute (nicolas-grekas) This PR was merged into the 7.1 branch. Discussion ---------- [Routing] Add `{foo:bar}` syntax to define a mapping between a route parameter and its corresponding request attribute | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | no | New feature? | yes | Deprecations? | no | Issues | - | License | MIT While trying to improve the DX of auto-mapping Doctrine entities and from the discussion on the related PR #54455, I realized that it would help a lot if we were able to express a mapping between route parameter and request attributes directly into route definitions. This PR adds the ability to define a route with such a mapping: ```php #[Route('/conference/{slug:conference}')] ``` On the router side, the side-effect of this is just that a new `_route_mapping` array is returned by the matcher, and nothing else. Then, in HttpKernel's RouterListener, we use that parameter to map route parameters to request attributes. On their turn, argument resolvers will just see a request attribute named `conference`. But they can also now read the `_route_mapping` attribute and decide to do more tailored things depending on the mapping. For example, one could define this route: ```php #[Route('/conference/{id:conference}/{slug:conference}')] ``` This would be turned into a request attribute named `conference` with `['id' => 'the-id', 'slug' => 'the-slug']` as content. This mapping concern already leaks into many value resolver attributes (see their "name" property). For the entity value resolver, this feature will allow deprecating auto-mapping altogether, and will make things more explicit. Commits ------- 1e091b9 [Routing] Add `{foo:bar}` syntax to define a mapping between a route parameter and its corresponding request attribute
…d route parameters
Thank you @nicolas-grekas. |
In https://symfony.com/blog/new-in-symfony-7-1-mapped-route-parameters it says
I counter that argument. Deprecating a feature that works since years is not a great DX. This one feels different than other deprecations, as it requires so many route to be manually adjusted. I understand it if there are multiple Entities and parameters, but if only one {id} is set and only one Entity is used, isn't it obvious which one is meant? |
the usage of |
The |
@nicolas-grekas thanks for the update. I agree that {document} is better to understand and should be used. But in the past {id} was documented and is likely used in tenth of thousands of routes out there. |
Follow up at #57211 |
@nicolas-grekas @kevinpapst from the point of view of generating URLs in templates: <a href="{{ path('document_show', {id: document.id}) }}"> ... </a> This looks better than the following: <a href="{{ path('document_show', {document: document.id}) }}"> ... </a> A But maybe I'm missing something here. |
…ities (nicolas-grekas) This PR was merged into the 7.1 branch. Discussion ---------- [DoctrineBridge] Revert deprecating by-{id} mapping of entities | Q | A | ------------- | --- | Branch? | 7.1 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT As discussed in #57211 Introduced in #54455 Commits ------- b81a62c [DoctrineBridge] Revert deprecating by-{id} mapping of entities
Hey, thanks for this feature! I've tried implementing it but came around an interrogation: what happens if I want to find multiple entities... from their #[Route(path: '/voir/{id:bill}/{id:receipt}', name: 'show_bill_and_receipt')]
public function showBillAndReceipt(Bill $bill, Receipt $receipt): Response
{
// Here the hypothetical queries ran by the Doctrine Bridge would have been:
// $bill= $billRepository->findOneBy(['id' => 'the bill's id']);
// $receipt= $receiptRepository->findOneBy(['id' => 'the receipt's id']);
} And to create the related link in the UI: {{ path('show_bill_and_receipt', {'id:bill': bill.id, 'id:receipt': receipt.id})} }} This way I thought I could have benefitted from the best of Mapped Route Parameters and Doctrine Bridge. Do you think of a way I could make this work without using the Sorry if I'm not in the right Conversation to ask this question, if you want I can open an Issue dedicated to this question. |
@Narthall just drop the #[Route(path: '/voir/{bill}/{receipt}', name: 'show_bill_and_receipt')]
public function showBillAndReceipt(Bill $bill, Receipt $receipt): Response |
If the bill and receipt are linked, the more standard way would be #[Route(path: '/voir/{id:bill}', name: 'show_bill_and_receipt')]
public function showBillAndReceipt(Bill $bill): Response
{
$receipt = $bill->getReceipt();
} which guarantees that you get the related receipt. If you really want to fetch both from the url, you could do #[Route(path: '/voir/{bill_id:bill}/{receipt_id:receipt}', name: 'show_bill_and_receipt')]
public function showBillAndReceipt(Bill $bill, Receipt $receipt): Response
{
} |
|
Auto-mapping of entities on controllers is a foot-gun when multiple entities are listed on the signature of the controller.
This is described extensively by e.g. @stof in the linked issue and in a few others.
The issue is that we try to use all request attributes to call a
findOneBy
, but when there are many entities, there can be an overlap in the naming of the field/associations of both entities.In this PR, I propose to deprecate auto-mapping and to replace it with mapped route parameters, as introduced in #54720.
If we go this way, people that use auto-mapping to e.g. load a
$conference
based on its{slug}
will have to declare the mapping by using{slug:conference}
instead. That makes everything explicit and keeps a nice DX IMHO, by not forcing a#[MapEntity]
attribute for simple cases.