Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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 |
---|---|---|---|
#53 | interdiff.txt | 655 bytes | pcambra |
#53 | comment-approve-1978948-53.patch | 5.46 KB | pcambra |
#51 | interdiff.txt | 1.91 KB | pcambra |
#51 | comment-approve-1978948-51.patch | 5.45 KB | pcambra |
#49 | comment-approve-1978948-49.patch | 4.93 KB | pcambra |
Comments
Comment #1
andypostThis one a bit depends on #1798296: Integrate CSRF link token directly into routing system but I see no way for now get entity and token same time
Comment #2
rgristroph CreditAttribution: rgristroph commentedThis is a first cut at this.
Perhaps someone could comment on the access related check and exception in the CommentController::commentApprovalPage(). Should that be in some sort of separate access method ?
--Rob
Comment #3
oenie CreditAttribution: oenie commentedRequest will be the first parameter for your method. Replace the drupal_container call.
Add an @throws in the doxygen for the method with info on when it's thrown.
Replace this by returning a RedirectResponse object. (Don't forget to specify it in the doxygen of the function)
Comment #4
oenie CreditAttribution: oenie commentedRestoring the original title.
Comment #5
rgristroph CreditAttribution: rgristroph commentedI think this should address the issues from oenie, except that the first one, I'm not getting the request object from the method call, I get it from \Drupal::request() . Is that right ? Should I change the method to take the request as an argument instead ?
Comment #6
dawehnerThank you!
we should better not remove the weight of the menu link.
Let's not put empty lines/whitespace in here.
Unneeded whitespace here.
You can just put the Request $request in the method and it will automatically passed in, so you don't have to get via \Drupal::request, see #1987816: Convert system_batch_page() to a new style controller
Comment #7
rgristroph CreditAttribution: rgristroph commentedI think I have addressed all of those points with this one.
Does the devel coder review module work for d8 ? I will investigate that so I can check my own code and avoid the whitespace and syntax issues.
--Rob
Comment #8
oenie CreditAttribution: oenie commentedAlmost there ...
Your parameter Comment needs a use Drupal\comment\Plugin\Core\Entity\Comment; at the top of the file. Otherwise it will use a wrong type of Comment object.
While we're changing stuff, this isn't really a 'Page callback:' anymore, so you you can drop that from the documentation.
Comment #9
mparker17I'll help!
Comment #10
mparker17Applied changes specified in #8.
Comment #11
mparker17I'll need to unassign it and mark it "needs review". :S
Comment #13
mparker17Nope! Just because my improperly-named interdiff failed testing doesn't mean that this isn't ready for someone else to review.
Comment #14
oenie CreditAttribution: oenie commented@mparker17: whrn you add an interdiff, be sure to give it anything but .patch as extension.
Otherwise (what happened now) testbot will test it, and it will always fail.
Comment #15
mparker17@oenie, I was aware that I should name the file "interdiff.txt", I just made a mistake.
The real patch will need to be tested and reviewed, however.
Comment #16
rgristroph CreditAttribution: rgristroph commentedI looked over this, and also tested it functionally making comments as anonymous and approving them as admin, and it looks ready to me.
--Rob
Comment #17
oenie CreditAttribution: oenie commented#10: comment-1978948-10-convert-comment-approve-to-a-controller.patch queued for re-testing.
Comment #18
oenie CreditAttribution: oenie commentedAdded a missing use clause. Should fix the last problem.
Comment #19
andypostdrupal_set_message() is lost?
Comment #20
rgristroph CreditAttribution: rgristroph commentedI think I accidently removed the drupal_set_message(), I had some custom stuff in there when I first rolled this to make sure it was the new router that was being called not the old callback, and I probably thought it was a debug message and removed it. Putting it back . . .
--Rob
Comment #21
oenie CreditAttribution: oenie commented@andypost: Good catch, missed that the first reviews.
Rerolling ...
Comment #22
rgristroph CreditAttribution: rgristroph commentedYeah, I reviewed / tested this again and everything is working, looks good, and the message is back.
--Rob
Comment #23
andypostRTBC +1
@rgristroph thanx, after patch will green feel free to rtbc
Comment #24
kgoel CreditAttribution: kgoel commentedI have tested and patch looks good to me.
Comment #25
andypostthe indent should be 2 spaces
doc block is wrong
Comment #26
andypostRelated #1978914: Convert comment_permalink() to a Controller
Comment #27
kgoel CreditAttribution: kgoel commentedComment #28
dawehnerStill too many spaces, sorr y. (yes the rest of teh comment file is also wrong but please still do something same).
Comment #29
kgoel CreditAttribution: kgoel commentedok, one more try
Comment #30
pfrenssenThe text of this @todo is no longer valid. It is no longer using page callbacks or access callbacks.
Comment #31
andypostPatch needs re-roll after #1970360: Entities should define URI templates and standard links and fix for #30
Comment #32
andypost#29: comment-approve-1978948-29.patch queued for re-testing.
Comment #34
andypostShould point to #1798296: Integrate CSRF link token directly into routing system
Comment #35
pcambraHere's re-roll and a try to improve the comment as for #30 & #34
Comment #37
pcambraThis should be in much better shape now
Comment #38
andypost@pcambra PLease re-roll patch without bits of #1981314: Drop procedural usage of fields in field module
Comment #39
pcambraOuch, interdiff is fine, here's the patch with the correct rebase, sorry.
Comment #40
kgoel CreditAttribution: kgoel commented@pcambra - patch doesn't have updated @todo doc.
Comment #41
pcambra@kgoel I do see the todo comment changed in #39, it might not be the best comment though, I'll wait for review of that.
Comment #42
andypostShould be fine except
CommentInterface
Comment #43
pcambraAddressed #42
Comment #44
dawehnerI think andypost talked about this piece of code here.
Comment #45
pcambraOops sorry, fixed.
Comment #47
dawehner#45: comment-approve-1978948-45.patch queued for re-testing.
Comment #48
tim.plunkettThese should start with /
In most places we've been putting single quotes around these, let's do that here and be consistent
Please swap the order, __construct should be the first method
public function
This should be rewrapped, the following lines of an @todo are indented 2 extra spaces. See the original comment in the removed function.
Crell point out in IRC that we now have $comment->permalink(), and it might make sense to use that here instead of hardcoding 'node/' again
Comment #49
pcambraI think I've addressed Tim's comment in #48, including using permalink method instead hardcoding node/ path
Comment #50
Crell CreditAttribution: Crell commentedYou are losing $permalink_uri['options'] here.
What you need to do is pass the generator into this controller as a dependency, then:
It's a bit verbose. Sorry. :-(
Comment #51
pcambraThanks for the input @Crell, here's a version of the patch addressing #49 *I think* :).
Comment #52
andypostA bit more
Comment #53
pcambraChanged that, thanks!
I'm wondering if
arguments: ['@url_generator']
should be added to the routing.yml file or that's just for services.Comment #54
andypostGreat!
suppose @argument only for services
Comment #55
Crell CreditAttribution: Crell commentedYes, the routing file doesn't do @arguments. That's specific to the container.
Comment #56
alexpottCommitted e49b129 and pushed to 8.x. Thanks!