Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment #103
Problem/Motivation
Moving comment reply to a new route controller
Proposed resolution
Conversion of routes to controllers and forms
Remaining tasks
None
User interface changes
Previously this registered a menu entry foe comment/reply/%node.
I don't see a use case to have a comment/reply/%node menu link, as it is dynamic anyway, so the menu link is being removed.
API changes
Comment Breadcrumb Builder is now a service.
Related Issues
#2061917: Add Test for CommentBreadcrumbBuilder
Original report by @shanethehat
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Meta issue: #1971384: [META] Convert page callbacks to controllers
Comment | File | Size | Author |
---|---|---|---|
#118 | drupal8.comment-module.1978958-118.patch | 15.71 KB | jibran |
#118 | interdiff.txt | 9.52 KB | jibran |
#115 | drupal8.comment-module.1978958-115.patch | 15.97 KB | jibran |
#115 | interdiff.txt | 750 bytes | jibran |
#113 | drupal8.comment-module.1978958-113.patch | 16.03 KB | jibran |
Comments
Comment #1
kgoel CreditAttribution: kgoel commentedGoing to work on this.
Comment #2
kgoel CreditAttribution: kgoel commentedComment #3
vijaycs85Issuing initial patch...
Comment #5
cosmicdreams CreditAttribution: cosmicdreams commented_content instead of _controller since it's returning a renderable array
use a RedirectResponse instead.
RedirectResponse instead.
Use a RedirectResponse instead
Use a RedirectResponse instead.
Comment #6
cosmicdreams CreditAttribution: cosmicdreams commentedtaking this one over.
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedHere's the corrections in action.
Comment #9
dawehner#7: 1978958_7.patch queued for re-testing.
Comment #10
dawehnerso hook_menu() is still important to keep as you need it in order to have menu links.
Should be "Contains \"...
There should be an empty line between the two ones.
What about "Controller for comment pages"
Let's tell people that it's a number.
The documentation should also document that it might be a redirect response instead.
Are you sure the $pid will be passed in like that? The parent comment id is loaded all the time if it exists, so what about make this a parameter so it get's converted by the parameter converter automatically?
You can replace $_POST by the $request variable injected into the method.
As it's an interface you should use $node->id() better.
Let's remove this empty line.
Let's put an empty line here.
Comment #12
kgoel CreditAttribution: kgoel commentedComment #14
kgoel CreditAttribution: kgoel commentedComment #16
dawehnerThis could be $request directly.
No need for this check. $request->request->get('op', ''); does the same.
comment_add is basically using the entity manager, should we leverage that?
This should be using url() with absolute => TRUE
This empty line looks unneeded.
comment_load could use an injected comment storage controller.
node_view could be replaced by an injected node entity render controller.
Comment #17
jibranAll Injections and then some.
I also want to convert this to service but waiting for #1947536: Convert drupal_get_breadcrumb() and drupal_set_breadcrumb() to a service change notice.
Comment #18
dawehneri guess pid is a int ?
It also returns a redirect response.
Should be public.
Comment #20
jibranFixed #18 and renamed controller file.
Comment #23
jibranThis should be green.
Comment #24
dawehnerThere should be some description as well.
Comment #25
jibranThanks @dawehner for the comment suggestions.
Comment #26
dawehnerNeeds proper indentation.
Comment #27
jibran:)
Comment #28
jibranRelated #2026063: Convert drupal_set_breadcrumb to breadcrumb builder service in comment_reply()
Comment #29
dawehnerLet's make a new line per service :(
The new breadcrumb handling is in, i guess this line is not right anymore.
Comment #30
dawehnerSo this is not really a blocker at the moment?
Comment #31
jibranI think #29-1 is not a RTBC blocker. For #29-2 see issue in #28. So if there is no real point to address can I get RTBC here :).
Comment #32
dawehnerI just did it.
Comment #33
jibranThank you very much for RTBC and fixing the issue. :)
Comment #34
alexpottSince 1978948
Comment #35
jibranreroll
Comment #36
jibranRemoved
core/modules/comment/comment.pages.inc
yay!!!Comment #37
jibransome minors
Comment #38
ParisLiakos CreditAttribution: ParisLiakos commentedyay for .inc removal
Comment #39
jibran#37: 1978958-37.patch queued for re-testing.
Comment #41
jibranreroll after permalink issue.
Comment #43
jibranReroll after #2024833: Adopt load() and loadMultiple() on entity storage controllers and fixed invalid PHP syntax.
Comment #44
jibraninterdiff.
Comment #45
jibranRTBC as per #38.
Comment #46
YesCT CreditAttribution: YesCT commentedThis issue was RTBC and passing tests on July 1, the beginning of API freeze.
Comment #47
larowlanThis will collide with #731724: Convert comment settings into a field to make them work with CMI and non-node entities but I don't believe we should postpone it on that basis alone
Comment #48
jibran#43: 1978958-43.patch queued for re-testing.
Comment #49
dawehnerIt feels wrong to implement an already deprecated api in a new patch.
Comment #50
jibran@dawehner see #28. This is a simple conversion issue.
Comment #51
larowlanWe have a comment breadcrumb builder in #731724: Convert comment settings into a field to make them work with CMI and non-node entities, moving and adding it here would reduce the size of that patch. Also double standards, adding it there was in scope, same should apply here imo </rant>
Comment #52
jibranHere is the patch for @dawehner's feelings :D. @larowlan now you have to reroll #731724: Convert comment settings into a field to make them work with CMI and non-node entities because of your rant :D
Comment #53
larowlanThanks jibran, believe it or not that is actually better :)
The patch over there (at 400kb) has suffered from lack of reviews since November. Making it smaller is a win in my book.
jibran++
Back to RTBC unless bot disagrees.
Comment #54
PanchoIndentation and missing quotes (_entity_access):
Also, the 'comment_reply' route isn't registered in comment_menu().
How could it pass the tests? Obviously comment replying is uncovered, so needs a test...
Missing backslash, should be "@return array|\Symfony\Component\HttpFoundation\RedirectResponse"
Also not sure if we really should replace comment_add() with the bulky entityManager call.
@dawehner had asked about this in #16:
However, comment_add() is not even deprecated, and other conversion issues (such as #731724: Convert comment settings into a field to make them work with CMI and non-node entities) continue using it. So for now we IMHO should leave it in, be it just so it can be found when converting these altogether in a consistent (and maybe improved) way at a later point.
Comment #55
jibran1) Fixed.
2) Fixed. Test pending.
3) Fixed.
4) I made the change because @dawehner asked for it so I let him answer this.
Comment #57
dawehnerAll what this lines are doing is to register a menu entry fro comment/reply/%node, the routing is already always covered my the new routing yml. To be honest I don't see a usecase to have a comment/reply/%node menu link, as it is dynamic anyway. Can't we drop it instead?
Comment #59
jibran#55: 1978958-55.patch queued for re-testing.
Comment #61
larowlanAgreed, we don't need a menu item here.
And there are tests, plenty of them.
Comment #62
PanchoRe #55:
I meant the indentation of 'pattern', 'defaults' and 'requirements' being too large, not of 'pid' too small. :)
Also, the extra space after 'pid' should go away.
Re #57: Yes, I think we should drop it.
Re #58/60: Why would the install fail twice? Locally, install succeeded.
Comment #63
PanchoSorry, crossposted.
Comment #64
PanchoQuicky tested the patch manually, and it seemed to work fine.
However, on 'comment/reply/1' (without pid), the title is "Comment permalink" now while it should be "Add new comment" just as with the given pid. This is probably because the 'comment_menu' entry didn't exactly match the route if the pid was ommitted, but as suggested by @dawehner and @larowlan, we're removing it anyway. So we need to manually set the title.
Rolled all other stuff in, except for the last point in #54 regarding comment_add(). That should be @dawehner's decision.
Comment #65
PanchoOne more question: Is getReply() really the right, meaningful name for this?
Shouldn't it be either of getReplyForm(), or in line with the other ones in that controller, commentReply()?
Comment #66
dawehnerPersonally i think it is fine to remove the usage of such non-apiish helper functions while doing the conversions.
I like your suggest of your getReplyForm method name suggestion.
Comment #67
PanchoOK, so I renamed the method to getReplyForm().
Also, the $request parameter should be preceding the optional $pid parameter, so it can be given a default of NULL. While technically the NULL is passed in by the router anyway, it's just more obvious what's happening.
While looking more closely at the code it became obvious that quite many lines are redundant. Also the many levels of indentations were quite hard to follow.
So I just reorganized the code a bit saving 20 lines of redundant functional code, and making the remaining code easier to grasp.
Now that it is easier to get an overview of the code, it becomes clear there is further room for improvement. However, I just reorganized/refactored the code - whatever would actually be changing code, should be covered by a followup.
Also rewrote most of the documentation, as the "we should do" style taken over from comment_reply() was wordy, slightly annoying and sometimes inconcise.
Tests remain passing, but another quick review would be fine.
Comment #69
jibran#67: 1978958-67.patch queued for re-testing.
Comment #70
andypost+1 rtbc
Comment #71
catch#67: 1978958-67.patch queued for re-testing.
Comment #73
jibranConflict in
core/modules/comment/comment.pages.inc
because of #2041287: Convert $node->nid to $node->id() and $node->isNew(). But we have already deleted that file. Here is a reroll.Comment #74
jibranReplaced
user_access
with$account->hasPermission
.Comment #75
alexpottThe string translation service can injected too
Comment #76
jibranNow with string translation service injection.
Comment #77
catchNo injection for t() here either. I didn't get a chance to review the breadcrumb patch before it was committed - is it not possible to use injected services in BreadcrumBuilder?
If not injection, this could at least use Drupal::
Comment #78
larowlanEach breadcrumb builder is a service (defined in x.services.yml) you can nominate the args to inject
Comment #79
jibranFixed #77.
Comment #80
dawehnerInstead of call it the manager let's just use the TranslatorInterface
In general i am wondering whether someone actually reviewed the changes in comment controller and all the logic which got changes by pancho in #1978958-67: Convert comment_reply() to a Controller. Andypost's comment is not really clear about that (#1978958-70: Convert comment_reply() to a Controller). Personally I don't have enough idea about how it should exactly work.
Mh, it seems odd that we have a message telling that an unpublished comment does actually not exist anymore. In general I would propose to use COMMENT_NOT_PUBLISHED because using "!" assumes internal value of the constant.
Instead of call it the manager let's just use the TranslatorInterface
Comment #81
jibranThere is no
TranslatorInterface
see #2049709: TranslationManager::translate() should be on an interface. As @alexpott said in #1938888-50: Convert user_admin_permissions to a new-style Form object. So can we finalize how to move forward here.
We can use in-depth review of #70.
Comment #82
vijaycs85Addressing 'COMMENT_NOT_PUBLISHED' part of #80 and other item in #80 addressed in #81
Comment #84
jibranThis is now $account = $request->attributes->get('_account');
Comment #85
wamilton CreditAttribution: wamilton commentedRan a little bit of the test suite to verify suggestion from @jibran. His fix fixes the tests I ran, so I'll let the bot handle the rest.
Comment #86
andypostThe title should now moved to _title according to https://drupal.org/node/2067859
Comment #87
jibranReroll after #1939994: Complete conversion of nodes to the new Entity Field API and #2057155: Add a generateFromRoute() method on the url generator to accept options like url() also accommodated #2036351: Convert CSRF tokens to a service. Fixed #86.
Comment #89
andypostJust add a $build['#title'] = $this->translation->translate('Add new comment')
Comment #90
jibranHere we go.
Comment #92
larowlanFWIW _title in the yml will be used for breadcrumbs when #2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}. is in
Comment #93
disasm CreditAttribution: disasm commentedFrom what I can tell, drupal_set_title can not override #title in a render array. I solved this by checking if the op is preview in the CommentController and setting the title there. Even if we did move this to _title in the route at some point, drupal_set_title in an entity form controller is pretty hacky.
Comment #94
disasm CreditAttribution: disasm commentedComment #96
andypostAlso needs check for 'node' in attributes
Comment #97
disasm CreditAttribution: disasm commentedAddressing #96. Also, adding drupal_set_title back into the FormController because it looks like it's dependent on it in other places.
Comment #98
disasm CreditAttribution: disasm commentedComment #99
jibranThis RTBC for me but I have worked a lot on this patch let's wait for @andypost. According to #92 we have to add
_title
back to yml file.@andypost
$attributes['_route'] == 'comment_reply'
has a default argument $node which will be always set in attributes.Comment #100
dawehnerIt would be cool to use $attributes[RouteObjectInterface::ROUTE_NAME] instead.
Comment #101
disasm CreditAttribution: disasm commentedImplementing request in #101.
Comment #102
dawehnerI would love to see some comment in the issue summary why this can be removed as it is.
Comment #103
jibran#2061917: Add Test for CommentBreadcrumbBuilder is postpone on this. @dawehner see #57 and #61.
Comment #104
disasm CreditAttribution: disasm commented@dawehner I paraphrased your comment in #57 issue summary template. Now we just need to get this committed!
Comment #106
disasm CreditAttribution: disasm commentedadding isset back in to stop exceptions.
Comment #107
jibran#106: drupal8.comment-module.1978958-106.patch queued for re-testing.
Comment #109
jibranIt needs a reroll after #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface.
Comment #110
disasm CreditAttribution: disasm commentedreroll!
Comment #111
dawehnerBack to RTBC.
Comment #112
jibranReroll after #2057589: [Docs clean-up] Rename ControllerInterface to ContainerInjectionInterface.
Comment #113
jibranSorry I messed up the reroll. Should be fine now.
Comment #115
jibranI can't believe it I have messed up re-roll twice :(
Comment #116
dawehnerThe rerole looked fine
Comment #117
webchickOverall looks good. I notice comment/reply/%node is moving from a MENU_NORMAL_ITEM to a callback, but that's probably fine.
A couple of things:
Should these not be $this->t()?
And that Drupal::currentUser()?
Comment #118
jibranhmmm here comes the
ControllerBase
.Comment #119
tim.plunkettThanks!
Comment #120
alexpottCommitted a080a52 and pushed to 8.x. Thanks!
Comment #121
jibranYay!! it is committed to 8.x. Thank you all.
Comment #122
andypostThanx a lot! Now we can finish comment field conversion
Comment #123.0
(not verified) CreditAttribution: commentedNeeds issue summary