Problem/Motivation

When calling $entity->toUrl('add-page') or $entity->toUrl('add-form'), \Drupal\Core\Entity\Entity::uriRouteParameters() incorrectly adds the entity ID as a route parameter. This is incorrect because adding an entity cannot possible require an entity ID, but it is not super problematic.

When calling $entity->toUrl('add-form') for an entity type with bundles, Entity::uriRouteParameters() fails to add the bundle to the route parameters which leads to an exception during URL generation.

Proposed resolution

In \Drupal\Core\Entity\Entity::uriRouteParameters():

  • Do not add the entity ID as a route parameter for the add-page and add-form link templates.
  • Add the bundle for entity types with bundles for the add-form link template.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#10 2751835-10-entity-url-add.patch5.89 KBtstoeckler
#5 2751835-5.patch5.85 KBtstoeckler
#5 2751835-5--tests-only.patch4.38 KBtstoeckler
#5 2751835-4-5-interdiff.txt5.06 KBtstoeckler
#4 2751835-4.patch1021 byteststoeckler
#3 2751835-3.patch905 byteststoeckler
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Issue summary: View changes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Active » Needs review
FileSize
2751835-3.patch905 bytes

Here we go.

Extracted this from #2723579-16: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider but realized we don't need to check for a bundle entity type, a bundle key is completely sufficient.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
FileSize
2751835-4.patch1021 bytes

Updated patch from #2723579: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider. No interdiff because I literally copied the part of the patch from #2723579-21: [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider. Stil postponed for test coverage, but could use a review nonetheless.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Title: [PP-1] Entity::urlRouteParameters() is broken for add-page and add-form link templates » Entity::urlRouteParameters() is broken for add-page and add-form link templates
FileSize
2751835-4-5-interdiff.txt5.06 KB
2751835-5--tests-only.patch4.38 KB
2751835-5.patch5.85 KB

Now with test coverage, now that #2751395: Rewrite EntityUrlTest has been committed.

I added the add-form and add-page link relations to the language check in Entity::toUrl() simply to avoid further code divergence. I.e. I would have to separate the tests for collection and add-page then (add-form is already a special flower) instead of using a data provider. I can also revert that, if people don't like that. I added a comment to explain why I think we are doing that. I'm not really sure, though. I looked into #2428103: String formatter should link to its translation. where this code was introduced and the only comment is #2428103-18: String formatter should link to its translation.:

I'm not really sure about the check for the link relation

...so yeah... :-)

The last submitted patch, 5: 2751835-5--tests-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 5: 2751835-5.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Status: Needs work » Needs review
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
Issue summary: View changes
tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler as a volunteer commented
FileSize
2751835-10-entity-url-add.patch5.89 KB

This needed a re-roll after #2751583: Entity::urlRouteParameters() calls getRevisionId() for non-revisionable entities.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented

That totally looks great for me!

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityUrlTest.php
@@ -77,6 +77,20 @@ class EntityUrlTest extends UnitTestCase {
   /**
+   * Indicator that the test entity type has no bundle key.
+   *
+   * @var false
+   */
+  const HAS_NO_BUNDLE_KEY = FALSE;
+
+  /**
+   * Indicator that the test entity type has a bundle key.
+   *
+   * @var true
+   */
+  const HAS_BUNDLE_KEY = TRUE;

Nice!

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Test and logic looks great.

I thought about asking for the checks to be made into a method - but I think the current patch makes sense as the concerns are slightly different each time - the array just happnes to be the same.

+++ b/core/lib/Drupal/Core/Entity/Entity.php
@@ -222,7 +222,9 @@ public function toUrl($rel = 'canonical', array $options = []) {
+    if (!in_array($rel, ['collection', 'add-page', 'add-form'], TRUE)) {

@@ -302,10 +304,14 @@ public function url($rel = 'canonical', $options = array()) {
+    if (!in_array($rel, ['collection', 'add-page', 'add-form'], TRUE)) {

Committed and pushed 53704b73f5f8add5e9a65d22f24fc500c1875fd9 to 8.2.x and 54e08c9 to 8.1.x. Thanks!

  • alexpott committed 53704b7 on 8.2.x 
    Issue #2751835 by tstoeckler: Entity::urlRouteParameters() is broken for...

  • alexpott committed 54e08c9 on 8.1.x 
    Issue #2751835 by tstoeckler: Entity::urlRouteParameters() is broken for...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.