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

Comments

tim.plunkett’s picture

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

larowlan
Location 🇦🇺🏝.au GMT+10
commented

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

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
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
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
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 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
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
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 🇨🇦
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.