Problem/Motivation

#1837388: Provide a ParamConverter than can upcast any entity. added the ability to upcast entities based on other route parameters.

Proposed resolution

Use that for comment.reply

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#6 comment-2326707-6.patch6.46 KBtim.plunkett
#1 comment-2326707-1.patch6.5 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
comment-2326707-1.patch6.5 KB
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

- { name: breadcrumb_builder, priority: 100 }
Unrelated?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented 
+++ b/core/modules/comment/comment.services.yml
@@ -3,7 +3,6 @@ services:
     class: Drupal\comment\CommentBreadcrumbBuilder
     tags:
       - { name: breadcrumb_builder, priority: 100 }
-    arguments: ['@entity.manager']

Are you using something other than dreditor to review? I didn't remove this line, the line just starts with a hyphen.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

Pro tip: don't review patches on your phone

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: comment-2326707-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Reviewed & tested by the community
FileSize
comment-2326707-6.patch6.46 KB

Rerolled for #2314599: Use title/url instead of l() for building breadcrumb.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/comment/comment.routing.yml
@@ -51,13 +51,17 @@ entity.comment.delete_form:
-  path: '/comment/reply/{entity_type}/{entity_id}/{field_name}/{pid}'
+  path: '/comment/reply/{entity_type}/{entity}/{field_name}/{pid}'
...
     pid: ~

+++ b/core/modules/comment/src/CommentBreadcrumbBuilder.php
@@ -20,30 +19,10 @@ class CommentBreadcrumbBuilder implements BreadcrumbBuilderInterface {
+    return $route_match->getRouteName() == 'comment.reply' && $route_match->getParameter('entity');

Just the PID is optional, so why do we check that the entity is there? Is this on 404/403 pages?

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

I didn't write that originally, but I thought of it as protecting against someone altering the comment.reply route to work differently, and having the breadcrumb blow up...

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Sounds reasonable. Nice to see how this new capability cleans up the code.

Committed and pushed to 8.x. Thanks!

  • webchick committed 584911a on 8.0.x 
    Issue #2326707 by tim.plunkett: Use dynamic entity type upcasting for...

Status: Fixed » Closed (fixed)

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