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.
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.
Comment | File | Size | Author |
---|---|---|---|
#37 | comment-menu-placeholders.patch | 5.35 KB | Gábor Hojtsy |
#34 | comment-menu-placeholders.patch | 5.35 KB | Gábor Hojtsy |
#33 | comment-menu-placeholders.patch | 5.2 KB | Gábor Hojtsy |
#27 | nix-comment-context.patch | 1.21 KB | Gábor Hojtsy |
#19 | drupal.comment-approve.patch | 2.69 KB | sun |
Comments
Comment #1
q0rban CreditAttribution: q0rban commentedApplied previous patch and made changes as described. Left comment/node/%node as is for now.
Comment #2
David_Rothstein CreditAttribution: David_Rothstein commentedSubscribing.. will try to review in a bit.
Comment #4
David_Rothstein CreditAttribution: David_Rothstein commentedReviewing now (and will try to fix the test while I review, unless someone else is).
Comment #5
q0rban CreditAttribution: q0rban commentedLet'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
Comment #6
q0rban CreditAttribution: q0rban commentedComment #7
David_Rothstein CreditAttribution: David_Rothstein commentedLooks good to me, although:
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 :)
Comment #8
Dries CreditAttribution: Dries commentedShould 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.
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedCode 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.
Comment #10
q0rban CreditAttribution: q0rban commentedRe-roll per #7 (removing all if/else logic from comment_approve) and #8 (Clean up titles of local tasks).
Comment #11
q0rban CreditAttribution: q0rban commentedComment #12
q0rban CreditAttribution: q0rban commentedwhoops, I guess I should wait for someone else to mark it as RTBC. ;)
Comment #13
sunVery nice job, q0rban!
Comment #14
Dries CreditAttribution: Dries commentedLooks great. Committed!
Comment #15
webchickThis 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.
Comment #16
webchickAlso, 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.
Comment #17
sunTagging.
Comment #18
andypostcsrf for menu-links #144538: User logout is vulnerable to CSRF and depends on #204077: Allow menu links pointing to dynamic paths
Comment #19
sunNot 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.
Comment #21
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, 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.
Comment #22
catchWe 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).
Comment #23
sun@catch: The loaded %node as well as %comment should be cached. Both need to rely on node access grants.
Comment #24
catchDoh, mis-typed. %node is of course , but %comment has no static caching at all, see #611590: Statically cache comments for the other option.
Comment #25
sun.core CreditAttribution: sun.core commentedWhat's left here?
Comment #26
catchThis is still critical due to the following issues:
Visiting comment/1/edit causes the same comment to be loaded eight times from the database due to using %comment router paths with no static caching.
We aren't using contextual links for these paths any more, but they still use MENU_CONTEXT_INLINE in the hook_menu() definition. If a module added contextual links for comments from contrib then this would additionally trigger 2-3 uncached comment_load() calls per comment.
The title of the comment/1/reply path is still "Comment permalink".
Despite not using contextual links on comments any more in core, we still have MENU_CONTEXT_INLINE in the hook_menu() definition. I'm also still not comfortable having %comments style router paths without a static cache due to menu system / contextual links performance issues.
Comment #27
Gábor Hojtsy#611590: Statically cache comments would still be relevant then, no? We seem to be still needing the caching then because we load the comment in the menu system.
Here is a patch which removes the context flags from the menu items, which is as you requested as far as I get.
On the title of the comment/1/reply page, this type of path does not seem to be in the code. I'm seeing comment/reply/%node, which then has a different title.
I've also moved the CSRF patch to where it belongs, the crosslinked issue at http://drupal.org/node/66264#comment-2559194
Comment #28
sunThis means that the "approve" link is only output on the comment/%comment page...? That would be a regression.
Powered by Dreditor.
Comment #29
catchYes we can either introduce a static cache (although that risks memory bloat when viewing lots of comments - on those pages they only get loaded once now we're not using contextual links on them), or we can move back to anonymous placeholders - we should pick one though.
Comment #30
Gábor Hojtsy@sun: better ideas then?
@catch: in user module, we went (back?) to number placeholders to help performance.
Comment #31
catch@gabor, yeah I think that's better than potentially loading 300 comments and all their fields into memory - some people write very long comments and core memory usage is horrible at the moment.
Comment #32
sunThe 'edit' link is the only comment link that actually needs a $comment object to determine whether a user is permitted to edit a comment. All other links are purely permission based, as far as core is concerned.
Since the 'edit' link belongs to non-administrative, user-facing links, it is generated in http://api.drupal.org/api/function/comment_links/7
Ideally, we'd replace the %comment arguments with anonymous placeholders and remove 'delete' and 'approve' from comment_links(), providing them via contextual links only.
Comment #33
Gábor HojtsyOk, here is a patch that attempts to consolidate all opinions.
1. Replace %comment with anonymous placeholders except in the edit case. Should we replace there for consistency as well and put a load in the access check?
2. Adds a title setting page callback to the edit page, so it does not say "Comment permalink" anymore. (Re: multiple notes about that above).
3. Removed contextual link constants. Even though @sun keeps mentioning ideally these links should show up as contextual links, this issue is not about that. The premise of this issue is to clean up menu definitions for these items. No contextual links are showing up for comments now, so removing these constants is merely cleaning up cruft. Merely removing the context constant would have made the approve link to appear, so I made it a menu callback instead.
Works in manual testing. Note that this patch changes the signature of comment_approve() and comment_permalink() in the interest of performance. The other two affected functions got wrappers to fix the title situation and react to nonexistent comments properly out of a form callback respectively.
Comment #34
Gábor HojtsyNote that the edit link still has %comment, so that we only need to load the comment once. If we'd go for anonymous placeholder there, the comment would be loaded both in the access callback AND in the page callback. Having %comment there avoids that double load, but makes our callbacks look strange in comparison. Maybe we need a comment to that effect? Added that.
Comment #35
catchAll looks solid to me. The %comment path for edit makes sense since that's a common-ish path to view, and no-one's going to add menu links pointing there, comment explains why we do it. Haven't run tests, so please wait for the test bot to come back, but RTBC.
Comment #36
sunThis should be
return MENU_NOT_FOUND;
Powered by Dreditor.
Comment #37
Gábor HojtsyThanks, right. Patch fixed.
Comment #38
catchAll looks fine (again).
Comment #39
webchickCommitted to HEAD. Thanks!
Comment #42
hailander CreditAttribution: hailander commentedwe are working on the same issue , is there any way to do this in Drupal6 without hacking drupal core