Was testing different conditions, and typed in to the browser the URL of a forum comment edit page.

However, I entered the URL incorrectly... instead of this:

CORRECT:
/comment/553223/edit

I flipped the second and third segments:

INCORRECT:
/comment/edit/553223

I got an "Access Denied" page as I expected, but also received an error from Forum Access:

Notice: Trying to get property of non-object in _forum_access_comment_access_callback() (line 118 of /sites/all/modules/forum_access/forum_access.module).

It is passing in the string of "edit" as $comment, which is going to fail. The attached patch checks if $comment is a string, and returns FALSE if it is.

Please review, thanks.

CommentFileSizeAuthor
forum_access-comment_edit_url_error-1.patch700 bytesron_s
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ron_s created an issue. See original summary.

salvis’s picture

salvis
he/his
CreditAttribution: salvis as a volunteer commented

Nice catch, thank you!
I think it would make sense to use !is_numeric() rather than is_string().

A similar issue probably exists for nodes, too.

Please check whether the issue in in D8, too — D8 needs to go first...

ron_s’s picture

ron_s CreditAttribution: ron_s commented

What if you want to use the function in the future to pass in $comment as an object? I think that's the reason why other functions allow it to be is_numeric and then check for is_string. If it's an object, it will continue through without running the conditionals.

If you feel strongly it should be !is_numeric that's fine, can change it, but wanted to explain there was a reason for choosing is_string.

I understand D8 is primary, but it's just not our focus right now. You can check my profile to see how many D7 patches we have submitted in the last few months... there are quite a few. We'll eventually start looking at D8, but it won't be for a bit yet.

  • salvis committed 207cff9 on 7.x-1.x 
    Issue #3003467 by ron_s: Error when forum comment edit URL is incorrect.
salvis’s picture

salvis
he/his
CreditAttribution: salvis as a volunteer commented
Version: 7.x-1.x-dev » 8.x-1.x-dev
Status: Needs review » Patch (to be ported)

Thank you, ron_s, and sorry about taking so long.

salvis’s picture

salvis
he/his
CreditAttribution: salvis as a volunteer commented
Version: 8.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Fixed

AFAICS, D8 gives me a Page Not Found, so it doesn't seem to be an issue for D8.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.