Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
comment.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
24 Apr 2013 at 17:00 UTC
Updated:
29 Jul 2014 at 22:13 UTC
Jump to comment: Most recent file
Comments
Comment #1
shanethehat commentedI'll try this
Comment #2
shanethehat commentedComment #3
shanethehat commentedComment #4
andypostwhy not inject request here too?
subrequest === redirect?
Comment #5
dawehnerMissing spaces.
What is the expected return value in this case?
Comment #6
shanethehat commentedComment #7
shanethehat commentedComment #9
dawehnerJust in general you don't need to inject the request into the controller. What you can do instead id to add Request $request on the actual controller method. This will be passed in magically/automatically.
Comment #10
mparker17I'll help!
Comment #11
mparker17Okay, let's try this.
I noticed that "kernel" was misspelled in multiple places, including the DI tag, so I've fixed it and ensured that the DI tag being used is correct.
Also, class properties are supposed to be in camelCase, so I've done that too.
Comment #12
dawehnerPlease always adapt the original code: hook_menu and remove the existing code.
Comment #13
mparker17hook_menu entry removed. And, I made the tests pass.
Moved the Controller into a Controller sub-namespace as per @Crell's recommendation in IRC.
Comment #14
dawehnerYou can also just use $node->access('view') instead
Comment #15
mparker17Setting to needs work; I'll try to fix it tonight.
Comment #16
juampynr commentedReplaced entity_page_access($node, 'view') by $node->access('view').
The permalink request still works as expected after this change.
Comment #17
mparker17Ah, thanks @juampy! Unassigning from myself.
Comment #18
andypostPlease place it in \Drupal\comment\Controller\CommentController as #1907960-187: Helper issue for "Comment field" does
Comment #19
andypostRelated #1978948: Convert comment_approve() to a Controller
Comment #20
mparker17Renamed controller to Drupal\comment\Controller\CommentController
Comment #21
mparker17Sigh, the comment at the top of the file is wrong.
Comment #22
mparker17Lets try this again...
Comment #23
andypostNow you need to drop
comment_permalink()old function and updatecomment_menu()with route_nameStill think that redirect is better...
A kind of
return new RedirectResponse(url('/node/' . $node->id(), array('absolute' => TRUE)));should be enough
Comment #24
mparker17Okay, I'll do that.
Comment #25
dawehner@andypost
Let's please not change the functionality. This was discussed in #26966: Fix comment links when paging is used. and saving another bootstrap is a valid point.
Comment #26
mparker17Unfortunately, I haven't had the chance to work on this recently. Unassigning from myself - hopefully someone else can pick it up. Sorry :(
Comment #27
dawehnerAddressed the main point of andypost's comment.
Comment #28
andypostAwesome!
Comment #29
tim.plunkettI don't see how this is in scope, but if the typehint is still used it should be \Drupal\comment\CommentInterface
This should be _entity_access: 'comment.view'
This should be \Drupal\comment\CommentInterface
Missing a blank line
This should be handled by the route requirements
Comment #30
dawehnerThanks for the review!!
I would like to, but how do we define two keys _entity_access in the routing yml file :(
Comment #31
star-szrThis needs a reroll.
Comment #32
tim.plunkettRerolled, it just conflicted with drupal_container() being replaced
Comment #33
andypostLet's get this in
Comment #34
andypostController already in #1978948: Convert comment_approve() to a Controller
Comment #35
tim.plunkettRerolled.
Comment #37
star-szr#35: comment-1978914-35.patch queued for re-testing.
Comment #38
twistor commentedLooks purty.
Comment #39
alexpottCommitted 6ac2f8e and pushed to 8.x. Thanks!
Comment #41
andypostFollow-up #2062051: The request does not contain the _account on comment permalink pages