Posted by sun on October 16, 2009 at 5:15pm
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | comment.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
| Issue tags: | Contextual links, D7 API clean-up, Performance |
Issue Summary
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 |
Comments
#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: User logout is vulnerable to CSRF 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.
#25
What's left here?
#26
This 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.
#27
#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
#28
+++ modules/comment/comment.module 4 Feb 2010 18:12:03 -0000@@ -249,7 +249,6 @@ function comment_menu() {
$items['comment/%comment/approve'] = array(
...
- 'context' => MENU_CONTEXT_INLINE,
This means that the "approve" link is only output on the comment/%comment page...? That would be a regression.
Powered by Dreditor.
#29
Yes 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.
#30
@sun: better ideas then?
@catch: in user module, we went (back?) to number placeholders to help performance.
#31
@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.
#32
The '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.
#33
Ok, 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.
#34
Note 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.
#35
All 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.
#36
+++ modules/comment/comment.admin.inc 8 Feb 2010 14:04:31 -0000
@@ -234,6 +234,16 @@ function comment_multiple_delete_confirm
+function comment_confirm_delete_page($cid) {
...
+ drupal_not_found();
+++ modules/comment/comment.pages.inc 8 Feb 2010 14:04:34 -0000
@@ -103,13 +103,16 @@ function comment_reply($node, $pid = NUL
+function comment_approve($cid) {
...
+ drupal_not_found();
This should be
return MENU_NOT_FOUND;
Powered by Dreditor.
#37
Thanks, right. Patch fixed.
#38
All looks fine (again).
#39
Committed to HEAD. Thanks!
#40
Automatically closed -- issue fixed for 2 weeks with no activity.
#42
we are working on the same issue , is there any way to do this in Drupal6 without hacking drupal core