Use proper menu router paths for comment/*
sun - October 16, 2009 - 17:15
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Contextual links, D7 API clean-up |
Description
For #473268: D7UX: Put edit links on everything, we need proper argument loader path structures.
Comment module currently uses:
comment/edit/%comment
comment/delete/%comment
...This needs to be converted into:
comment/%comment/edit
comment/%comment/delete
...Attached patch already converts comment/delete accordingly. We at least need comment/edit to be converted equally. The "reply" and others are not necessarily required.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| drupal-comment-paths.patch | 3.97 KB | Idle | Failed: Failed to apply patch. | View details | Re-test |
#1
Applied previous patch and made changes as described. Left comment/node/%node as is for now.
#2
Subscribing.. will try to review in a bit.
#3
The last submitted patch failed testing.
#4
Reviewing now (and will try to fix the test while I review, unless someone else is).
#5
Let's try this again. Fixed comment_approve as it used to accept $cid as its argument, and should now be passed the $comment object. Also changed all items to local tasks and weighted them.
Not sure what is the best order for the local tasks but the order I chose is View | Edit | Approve | Delete
#6
#7
Looks good to me, although:
-function comment_approve($cid) {- // Load the comment whose cid = $cid
- if ($comment = comment_load($cid)) {
+function comment_approve($comment) {
+ if ($comment) {
The
if ($comment)part is no longer necessary anymore since the menu system will take care of never even hitting this callback function in the case where the comment doesn't exist. So, this function can actually be simplified (similar to how the patch simplifies the callback for comment deletion).One other thing - the immediate effect of this patch is pretty odd (see attached screenshot). However, we can probably let that slide since #473268: D7UX: Put edit links on everything would automatically take care of fixing it :)
#8
+++ modules/comment/comment.module 16 Oct 2009 18:26:30 -0000@@ -166,20 +166,44 @@ function comment_menu() {
+ 'title' => 'Approve a comment',
Should be 'Approve comment' to be consistent with the other links. See also David's screenshot in the previous comment.
For consistency with the node system and all the rest of Drupal, it should actually be just 'Edit', 'Approve', 'Delete' instead of 'Edit comment', 'Approve comment' and 'Delete comment'.
Otherwise, this patch looks very straightforward.
#9
Code looks good to me. Nice bonus that comment_delete_page() is no longer needed. I expect regression failures (like in forum.module), but those can be fixed once bot reports them.
#10
Re-roll per #7 (removing all if/else logic from comment_approve) and #8 (Clean up titles of local tasks).
#11
#12
whoops, I guess I should wait for someone else to mark it as RTBC. ;)
#13
Very nice job, q0rban!
#14
Looks great. Committed!
#15
This introduced a CSRF vulnerability. Some douchebag can put <img src="http://localhost/core/comment/1/approve" /> in their comment.
We need a confirm form around the approval link.
#16
Also, we should give this local task a proper access callback. "Approve" should only show up if the comment is unpublished. The link does this, the local task does not.
#17
Tagging.
#18
csrf for menu-links #144538: Image with /logout URL as source and depends on #204077: Allow menu links pointing to dynamic paths
#19
Not sure whether this works, but it should do at least 90% of what's needed.
And, I think I already mentioned elsewhere that this wasn't really introduced by this patch... so not sure whether it's superior to discuss this publicly, but anyway.
#20
The last submitted patch failed testing.
#21
Yeah, the CSRF vulnerability was not introduced by this patch - I think all this patch did was change the URL at which it happens :) It looks like it was introduced (in Drupal 7 only) via #66264: Remove CSRF vulnerability from comment module and a fix is already being worked on there.
In this issue, however, we still have two followup bugs visible in the screenshot from #7:
1. The title of the page should not be "Comment permalink".
2. It's not really clear that we want all these things showing up as visible local tasks, which was a side effect of the committed patch... we don't (currently) do that with most delete links in Drupal, for example.
#22
We also need to roll back using %node menu loaders for edit and delete links I think, since that prevents using contextual links without generating a database query per path (due to menu access checks).
#23
@catch: The loaded %node as well as %comment should be cached. Both need to rely on node access grants.
#24
Doh, mis-typed. %node is of course , but %comment has no static caching at all, see #611590: Statically cache comments for the other option.