Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Hello, I've recently installed this module and it's great, but I've changed the behaviour of the 'user/%/messages' menu link. I think that it's natural that users could view his own messages in this link, so I've patched the code to get it working. The patch works fine and I'm using it in my little web site. I've very little experience in Drupal development, so I don't know if my approach is correct.
It's my first patch too, sorry if there are many mistakes.
Kind regards.
Comment | File | Size | Author |
---|---|---|---|
#31 | privatemsg-1443172-31-testsfixed.patch | 6.52 KB | jonhattan |
| |||
#30 | interdiff.txt | 1.46 KB | jonhattan |
#30 | privatemsg-1443172-30.patch | 5.49 KB | jonhattan |
#27 | privatemsg-1443172-27.patch | 5.49 KB | ptmkenny |
#22 | privatemsg-fix-user-tab-access-1443172-7.x-1.x-22.patch | 5.45 KB | mstef |
Comments
Comment #1
harrrrrrr CreditAttribution: harrrrrrr commentedthis makes sense
Comment #2
BerdirThe problem is that the page is not a full replacement for /messages, for example, there is no tag or blocked user management. The sole purpose for these pages is to view messages of other users.
6.x-2.x has a feature that allows to move it below the user profile but that's very complicated and I'm not sure about porting it.
For example, there are many links and redirections which point to /messages. Giving users two separate places is IMHO confusing.
Comment #4
BerdirYour patch probably contains windows line endings, the testbot doesn't like that.
Comment #5
igalarza CreditAttribution: igalarza commentedHello Berdir, the patch does a redirect from /user/%/messages to /messages when you are viewing your own user, so it don't use two different places for messages.
I think that the line endings are right now.
Regards
Comment #6
BerdirRemember to set the Status to needs review when posting patches.
Comment #7
igalarza CreditAttribution: igalarza commentedok, sorry. And thank you for your patience.
Comment #8
BerdirAh, didn't see the redirect, sorry. Then I'm fine with this.
Lot's of nitpicking about coding style and how to simplify the function below. It looks like a lot, but it's not, don't worry. I'm just picky about the coding standard and I've been very detailed. In fact, with my suggestions, you should be able to reduce the lines of code in that function to 4-5 ;)
The perfect first sentence in a docblock should be as short as possible, start with a verb in 3rd person and if possible, below 80 characters.
Suggestion:
Checks access for the messages tab on the user profile.
You can then add a more detailed description below that, after an empty line.
Nope, it doesn't :) Since we are using this function only in this case, we can save the arguments completely (both), and just hardcode a privatemsg_user_access('read all private messages') at the end.
This line should end with a .
This also isn't really an API function, so remove this.
No space before the (. Also, I'd suggest to actually pass in the account object of the user we're visiting. Then the access check below becomes really simply. Because all you need is a if ($user->uid && $user->uid == $account->uid). To pass in the account, change the access arguments to array(1).
Comments should always be on their own line, either within or above the if, whatever you prefer. And also end with a .
Either add {} (they should always be used, even for single lines, to prevent confusion or remove the else completely. Because if the mentioned if statement above is true, it will return and not continue in the function anyway.
Same stuff here, start comment with an uppercase letter and end with a .
Also, no space before the semicolon.
Comment #9
igalarza CreditAttribution: igalarza commentedHello again and thanks for your corrections. I'm not used to the drupal code standard and I'm very happy to have this detailed feedback of my little piece of code.
I've tried to follow all your advices and there is the result; maybe my patch still have mistakes, english is not my mother language and sometimes is hard to express what I want to explain and to understand all the details.
I hope that everything is correct now, but I would be happy to change anything you want if remains any problem. Of course you can change yourself anything in the patch if you prefer it.
Kind regards.
Comment #11
igalarza CreditAttribution: igalarza commentedSorry, a stupid mistake.
I think is right now.
Comment #13
igalarza CreditAttribution: igalarza commentedWrong patch, there is the correct one.
Comment #14
BeaPower CreditAttribution: BeaPower commentedsub
Comment #15
mstef CreditAttribution: mstef commentedThis needs to be fixed in 7.x-1.x as well.
I'll write a patch
Comment #16
mstef CreditAttribution: mstef commentedPatch here to fix the access callback for user tabs on the profile. It will allow users to access it if they're viewing themselves, or for anyone, if they have "read all messages' perm.
I also removed privatemsg_list_page() because it's completely unnecessary. %user should be used in hook_menu() to load and pass the user object. All other menu callbacks can call drupal_get_form('privatemsg_list') directly, and bypass the need for that function.
PS: This is for 7.x-1.x.
Comment #18
mstef CreditAttribution: mstef commentedDidn't mean to change the title
Comment #19
-Mania- CreditAttribution: -Mania- commentedAny progress with this?
Comment #20
tymofii.pashkevych CreditAttribution: tymofii.pashkevych commentedI have resolved this issue in my custom module
Comment #21
mstef CreditAttribution: mstef commentedLet's try again with the correct version for my patch
Comment #22
mstef CreditAttribution: mstef commentedComment #24
Loac CreditAttribution: Loac commentedThanks to tymofii.pashkevych. #20 Good solution.
Comment #25
ptmkenny CreditAttribution: ptmkenny commentedPotential solution (patch) here: https://drupal.org/node/1772236
Comment #26
ptmkenny CreditAttribution: ptmkenny commentedClosing as a duplicate of #1772236: Make "Messages" Tab on own user pages available for users and "admins" with "read all messages" permission
Comment #27
ptmkenny CreditAttribution: ptmkenny commentedRe-roll of #13 with the tests from another patch.
Re-opening this thread as the maintainer rejected an alternative patch in favor of this one.
Comment #28
ptmkenny CreditAttribution: ptmkenny commentedThis patch works for
user/UID/messages
but is there a way to make it go to the sent box likeuser/UID/messages/sent
as well?Comment #29
Alcaparra CreditAttribution: Alcaparra commented+1
Comment #30
jonhattanFixed coding standards.
Comment #31
jonhattanTets will fail because of #2927068: Tests are broken. Added a convenience patch with the fix in 2927068
Comment #34
ivnish CreditAttribution: ivnish commentedComment #35
andypostD7 is not yet outdated