Hello

I am in the process of migrating a drupal 5 site to drupal 6. I have privatemsg installed but the changes to the url structure in the module conflict with my site.

I uses /messages for a primary section on the site and so as it stands it is incompatible with this module.

I'd prefer not to make changes to the module of course, so i'm looking into ways I can resolve this. So far I think the best bet is to use custom_url_rewrite / url_alter.module but would appreciate any other suggestions.

It would be great if one could configure the url path that privatemsg uses in the admin settings.

Thanks

Ben

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

smoothify’s picture

Version: » 6.x-1.x-dev
litwol’s picture

While i agree that this feature would be really *cool* to have, i think this is such an edge case that it makes me want to find an alternative solution than including the feature in pmsg core. Not to say i am rejecting it outright; playing devil's advocate and hoping for more people to chippin.

Berdir’s picture

Well, you can easily change the menu patches with hook_menu_alter, however, there are links to these all over the place in the pmsg source code, and while most of these can probably be changed, it is going to be rather complicated.

litwol’s picture

does it *make sense* to convert all of the '/messages/' to a global CONST with variable override through admin?

Berdir’s picture

constants can't be changed through the UI :)

Not sure, it would need a helper function and every link would need to go through that. This would also make it possible to move it below user/. Not sure if its worth the effort....

litwol’s picture

You mean we cant do ?:

define('PRIVATEMSG_URL_PREFIX', variable_get('privatemsg_messages_url', 'messages') );
Berdir’s picture

No, we cant. variable_get() cannot be used outside functions, because it's not ready at that point, as the database connection has not yet been initialized.

Edit: Correction: It might work and be customizable through settings.inc. However, a constant does not help in every case, simple example: To move the messages pages into the local tabs below /user/*uid* ( we have several requests to make this possible), you need to be able to specify something like "/user/%user/messages", which can be used directly in hook_menu, but when generating the links, %user needs to be replaced by $user->uid.

smoothify’s picture

I have tried using the constant method suggested by litwol and the concept does work.

Apart from the issue of changing every reference to the path 'messages' or 'messages/...' there are a couple of issues that would need addressing.

One is the reference to arg(1) in privatemsg_view_access() and the other is the 'page arguments' that use numerical values. E.g. if you used user/messages instead of messages, the extra argument would break things.

I have an idea for both so I will create a patch for this against the dev version shortly.

smoothify’s picture

FileSize
16.29 KB

Ok here is the patch as promised.

Summary:

*new constant created for url prefix with value retrieved from new url prefix variable
*field added to the privatemsg_settings page.

*made sure private_message_settings_submit fires after system_settings_submit.
*rebuild menu *only* if url prefix is changed.

*modified all menu items to use the new constant where appropriate
*modified all references to the path 'messages' or 'messages/..' in the module and additional modules.

*new arg count constant created that calculates number of arguments (count the /) in the prefix
*all "page arguments" that use fixed integers now use the constant value created
*privatemsg_view_access changed to use new constant

One additional change, unrelated to this issue:
*remove duplicate call to drupal_rebuild_theme_registry in submit handler (system_settings_submit already does this)

So far in testing, this seems fine for me, and all message related functions work with the new paths including the autocompletes.

litwol’s picture

Status: Active » Needs review

+define('PRIVATEMSG_URL_PREFIX', variable_get('privatemsg_url_prefix', 'user/messages') );

Our default is not 'messages' (without 'user/')

smoothify’s picture

Sorry, that one slipped through while debugging :)

Corrected patch applied below.

NaheemSays’s picture

I wonder how much of a clash there will be with #298502: Better access to another user's messages

Berdir’s picture

I don't think it's a good idea to use functions in the global scope, we do not have control over that. I want to test and play with that after I'm home...

talino’s picture

My 2c's worth : I use /messages on my site already and this cannot be changed. A setting for the base URL for Privatemsg would be most welcome because until it is implemented I unfortunately cannot use it. Edge cases are apparently not that uncommon :)

I tried editing the code but there are a zillion references to /messages, seemingly unrelated to hook_menu.

Thanks.

Berdir’s picture

First: I was wrong, you can use variable_get() outside functions, I doubt it's a good idea but it works.

Here is a updated patch...

Changes

- Introduced a new function privatemsg_get_dynamic_url_prefix() which can replace %user in the constant. This allows to define user/%user as prefix which converts the Messages link into a local task under "My account".

- Note that the constant still needs to be used for hook_menu

- Setting the prefix to user/%user will make the Inbox/Sent links unavailable, because they are now in the third-level local task. So this needs the patch at #469350: UI Improvements: Move secondary tabs to primary tabs and obviously, these two patches conflict.

- I removed the prefix from alll MENU_CALLBACK's. Not 100% sure about this.

- This does now also cover pm_email_notify.module. While doing this, I found out that the patch at #559866: PM Email Notify - broken link in default notification email is incorrect. I will re-open the other issue, note that it is fixed here too.

Berdir’s picture

Above patch contains a debug drupal_set_message()...

litwol’s picture

Status: Needs review » Needs work

due to "- Note that the constant still needs to be used for hook_menu"

NaheemSays’s picture

Can I mention my unease at having a method in the main module to use a different url scheme? seems like a recipe for pain for other modules that try to integrate etc... I would leave such customisations to other modules and not have it in privatemsg.

Berdir’s picture

You can and I thought about this too.

However, i think it is easier that way than the other way around. Because if it is another module, there are two issues:

a) it needs to change every link in privatemsg.module (and others), that is sometimes actually quite hard, for example inside theme functions... (it's easy to override those, but not from another module)

b) If you create a new module and hardcode those links, it will not work in both cases. You need to use some sort of a function, if it is from privatemsg.module or privatemsg_whatever.module

The default is still messages/, unless you change that, hardcoded links from other modules will still work.

Berdir’s picture

Status: Needs work » Needs review
FileSize
16.94 KB

Re-rolled the patch to work with the changes from #469350: UI Improvements: Move secondary tabs to primary tabs.

Removed the constant, the patch does now use variable_get() directly. because it has to be possible to change the value during the same request when you rebuild the menu after submitting the form.

Berdir’s picture

Category: support » feature

Changing to correct category..

Michelle’s picture

I spent way too much time last night trying to use hook_menu_alter to add a tab to the user page for private messages and move what are now primary tabs in 'messages' off the root of the site to secondary tabs under ''user/%user/private-messages'. I had some success but am still struggling. When I mentioned it to litwol on IRC, he said there's been some discussion of this so I went searching. This issue seems to be the one I need, but I'm getting confused trying to follow the issue. Is this the patch I need? If so, is it likely to be added anytime soon? It's been over a month since the last activity.

I think this would be a very good feature to have. All the other user related contrib modules I use put their interfaces on the user page so having privatemsg sitting off the root is weird and confusing to users. Having a way to move it without custom coding would be awesome. :)

Thanks,

Michelle

Berdir’s picture

Yes, this is the issue you are looking for. As you have found out, moving the path in hook_menu_alter is not enough, you also need to update all links. The patch takes care of that.

It won't make its way into the 1.0 release but I will look into it again once 1.0 is finally released (should happen soon).

The patch probably requires a re-roll, but worked when I wrote the last version.

Berdir’s picture

Version: 6.x-1.x-dev »
FileSize
17.9 KB

Attaching a first re-roll of this for the 2.x branch. Not very well tested and a path below /user/%user needs to be handled more intelligently and has to be merged somehow with the admin messages link.

Bilmar’s picture

subscribing

mtsanford’s picture

My idea: Have the links themed using theme functions. This way an admin can have the paths anyway they want using hook_menu_alter and theme overrides. There is precedent for themeing in links.

I'd like to see this customizable as well. Not just as an URL root, but *entirely* customizable. I think the above suggestion would make that possible. Michelle #22 mentioned many modules add a tab off user/. That is one case. Others (like me) may want the privatemsg pages somewhere entirely different, like sitting next to a "my friends" tab made available by the User Relationships module, and have the "read message" and "write new message" pages at an entirely different paths, not sitting as a tabs next to "messages".

litwol’s picture

If you want the whole thing alterable then feel free to write your own module and alter pmsg's links using menu/router _alter. otherwise what you propose is abit much for the module.

mtsanford’s picture

My idea: Have the links themed using theme functions. This way an admin can have the paths anyway they want using hook_menu_alter and theme overrides. There is precedent for themeing in links.

I'd like to see this customizable as well. Not just as an URL root, but *entirely* customizable. I think the above suggestion would make that possible. Michelle #22 mentioned many modules add a tab off user/. That is one case. Others (like me) may want the privatemsg pages somewhere entirely different, like sitting next to a "my friends" tab made available by the User Relationships module, and have the "read message" and "write new message" pages at an entirely different paths, not sitting as a tabs next to "messages".

I haven't tested all links, but using hook_menu_alter() and custom_url_rewrite_outbound() appears to do what I want.

Come to think of it, the same approach could be taken original poster, and Michelle #22, though of course requiring some custom coding.

(It looks like in D7 there is a drupal_alter('url_outbound', ...), which opens up all kinds of neat possibilities for modules that can rearrange pages and have internally generated links move with them, fixing one of my gripes about D6 :) )

Status: Needs review » Needs work

The last submitted patch, privatemsg_url_prefix_6.patch, failed testing.

crea’s picture

Subscribing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
19.73 KB

Reroll of the above patch.

@mtsanford

The theme() idea doesn't sound bad, I know that User Relationships is doing it in the same way. Not really sure about it, especially if we wouldn't need it in D7 anymore. However, I think there are url's that don't go through url() but drupal_goto() and stuff like that.

Anyway, you are invited to both write a patch for a theme() approach and/or create a documentation page below http://drupal.org/handbook/modules/privatemsg explaining your url_rewrite_outbound approach :)

robby.smith’s picture

subscribing

Berdir’s picture

#31: privatemsg_url_prefix_7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, privatemsg_url_prefix_7.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
19.99 KB

Re-roll.

EndEd’s picture

#35: privatemsg_url_prefix_8.patch queued for re-testing.

EndEd’s picture

+1

Status: Needs review » Needs work

The last submitted patch, privatemsg_url_prefix_8.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
20.12 KB

Re-roll of the latest patch.

Still not sure what to do with this...

arbel’s picture

Hi,

I'm trying to to add the messages tab to the user profile page, i've run into this issue, and i'm not quite sure this is what I"m looking for.

I've noticed that if I add the Read all private messages permission, the tab appears on the users profile page, but then the user can see everyones messages.

Does this patch allow me to add the tab without the giving the user access to all the messages?

Thanks

Idan

Berdir’s picture

Yes, that is the idea. You can specifiy user/%uid/messages (see the description) as the path prefix and it should show up as a local task at /user. However, there are a few cases that need to be handled better.

So, please test the patch and report an flaws and issues you can find.

tylor’s picture

This is really nice and we would love to see it applied. I have rerolled the patch against HEAD and poked around; didn't get a chance to test email notification and filters but everything else seems to be working as expected (create messages, view messages, block users, tag messages, add attachments).

Berdir’s picture

Thanks for re-rolling. Can you please share how you configured it? Just something else than /messages (like /whatever) or did you try to put it below /user? Because I'm pretty sure the latter doesn't really work well yet. For example, it might conflict with the read all permission.

Also, what would really help to get this committed would be some tests because I think it's something that easy gets broken by a change. Any chance you're up to the task of writing some tests? :) (Usually, when I ask that question, the issue gets stalled for weeks ;))

I have something like this in mind:
- A separate test class in privatemsg.module that depends on the modules that actually have to change when using this feature (privatemsg/privatemsg_filter/pm_block_user)
- A helper function that receives the current prefix as an argument, enables that in the admin UI and does some basic testing (send/read message, inbox/sent/list, blocked users page)
- 2-3 actual testSomething() functions that don't do anything more than calling the helper function with the prefix (one for a different "normal" path, can be random, and one for the special case user/%user/something)

EndEd’s picture

Subscribing

tylor’s picture

Stalled for just one week ;)

Here is a new patch with a test class which is hopefully similar to what you described in your message. I found a few issues while writing it and have included these in the patch (I guess this is why one writes tests):
- redirect was not working properly
- "You have a new message" link in new message block was not prefixed properly
- email messages didn't not contain correct recipient UID for !message link
- need to set prefix entry field as required on admin so it actually has a value, otherwise it acts buggy

Additionally, I noticed that the messages tab shows when you are viewing other user profiles (and prefix is set to /user/%user/messages) and that the user/uid/message is still available (and doubling up the tabs) when using a prefix like /user/%user/pm. I thought I would wait for some more direction and get some feedback before coming up with a fix for these, how should this be handled?

Finally, this patch does not test that the messages tab is properly removed from the main navigation links when %user is set in the prefix. Is this worth testing?

Hoping this helps move the issue along :)

Berdir’s picture

Awesome :)

Yes, that happens when I write tests as well :)

Additionally, I noticed that the messages tab shows when you are viewing other user profiles (and prefix is set to /user/%user/messages) and that the user/uid/message is still available (and doubling up the tabs) when using a prefix like /user/%user/pm. I thought I would wait for some more direction and get some feedback before coming up with a fix for these, how should this be handled?

Good question, I'm not sure..

In case the prefix starts with /user/%user/, maybe the easiest would be to not define the read all menu item but define a custom access callback to which we can pass the $user object and a permission and then...

- If the user object is the one of the current user, simply check the permission
- If it is not, then check if the user has read all permission

That is then necessary for every menu item below /messages as well. Note that the view message access callback already has an existing access callback, not sure what to do there..

Finally, this patch does not test that the messages tab is properly removed from the main navigation links when %user is set in the prefix. Is this worth testing?

I guess this makes sense.. should be enough to verify that there is no Messages link on the front page... we could also check in both cases if /messages returns 404.

Reviewing your patch now...

+++ privatemsg.module	6 Sep 2010 22:53:37 -0000
@@ -251,7 +284,8 @@ function privatemsg_user_access($permiss
-    if (arg(0) == 'messages' && variable_get('privatemsg_display_disabled_message', TRUE) && !$disabled_displayed) {
+    $arg = substr_count(variable_get('privatemsg_url_prefix', 'messages'), '/');
+    if (arg($arg) == 'messages' && variable_get('privatemsg_display_disabled_message', TRUE) && !$disabled_displayed) {

I don't think this check works if the prefix is not something like /bla/messages.

Maybe we could test if $_GET['q'] starts with privatemsg_get_dynamic_url_prefix()?

+++ privatemsg.test	6 Sep 2010 22:53:38 -0000
@@ -885,3 +885,119 @@ class PrivatemsgLinksTestCase extends Dr
+  }
+  ¶
+  function checkPrefix($url_prefix) {

There are quite a few trailing whitespaces.

If you install dreditor (link at the end), you can see them yourself when you click on Review.

+++ privatemsg.test	6 Sep 2010 22:53:38 -0000
@@ -885,3 +885,119 @@ class PrivatemsgLinksTestCase extends Dr
+    $this->drupalPost(str_replace('%user', $author->uid, $url_prefix) . '/new', $edit, t('Send message'));

Is there a reason why we can't use the existing function to get the dynamic url here?

+++ privatemsg.test	6 Sep 2010 22:53:38 -0000
@@ -885,3 +885,119 @@ class PrivatemsgLinksTestCase extends Dr
+    $this->drupalGet(str_replace('%user', $author->uid, $url_prefix) . '/sent'); // From privatemsg_filter.
+    $this->assertText($edit['subject'], t('Message found on message sent page.'));

Minor... can you place the comment above?

+++ privatemsg.test	6 Sep 2010 22:53:38 -0000
@@ -885,3 +885,119 @@ class PrivatemsgLinksTestCase extends Dr
+    $captured_emails = $this->drupalGetMails();
+    $this->drupalSetContent($captured_emails['0']['body']);

Nothing wrong with that, just wanted to say that this is quite a nice trick :)

+++ privatemsg.test	6 Sep 2010 22:53:38 -0000
@@ -885,3 +885,119 @@ class PrivatemsgLinksTestCase extends Dr
+  function testPrefixUser() {
+    $this->checkPrefix("user/%user/pm");
+  }
+
+}
\ No newline at end of file
Index: privatemsg.theme.inc

Another minor issue :) I think privatemsg.module is also missing a ending newline.

Nice work on the tests, thanks a lot!

Powered by Dreditor.

Berdir’s picture

Status: Needs review » Needs work
tylor’s picture

Status: Needs work » Needs review
FileSize
29.23 KB

Here is an updated patch. It adds a check that the user is not viewing someone else's messages from a profile unless they have the read all permission, hides the user/%user/messages menu item if a prefix with %user is set, and fixes the code style issues pointed to in your feedback.

Let me know :)

Berdir’s picture

I'll try out the patch soon, code looks good to me.

Berdir’s picture

This is looking quite good..

A few changes:

- Removed the constants as they weren't used anymore.

- Added an additional argument to privatemsg_user_access() that denies access if viewing at another users messages. Used for write new message and blocked users (being able to control the users another user has blocked would be a separate feature and could be re-added then)

- If using user/%user/something, actually pass the value of %user to the callback for a list/inbox/sent so that it can be used as a replacement for the current /user/%user/messages.

- privatemsg_get_dynamic_url_prefix() now uses the the uid of the user that you're viewing at, if %user is used. This means that if you are viewing at /user/123/messages, clicking on a subject to view a message, you'll remain in the profile of that user.

- Some cleanup, pass $account instead of $uid to the page callback.

- Moved the option into the listing fieldset in the admin settings.

I'd like to see two things before this can be considered RTBC:

- A review (from BenK if possible), especially of the admin setting. I think we can do better on the title and description than what we currently have.

- A few more tests would be awesome specifically for /user/%user/something, especially viewing messages of other users (messages of other users are displayed, write new is hidden, clicking on a subject remains in the current user profile).

BenK’s picture

Subscribing to help with this per Berdir's request!

tylor’s picture

Fixed a few bugs and added a few more tests:

Fixes:
- Fixed unread message after login (was linking to the wrong place).
- Added check that arg is_numeric in privatemsg_get_dynamic_url_prefix(), catches errors when seeing a PM link from other pages.
- Fixed typo in privatemsg_user_access() for permission 'read all private messages'.
- Ran through coder, fixed relevant style issues (mostly missing 'if' control spaces created by me ;).
- Changed wording for admin description and title.

New tests:
- Check that unread login message links to the correct place.
- Check that read all works on user/%user/messages and user/%user/foo.
- Check that messages for a user stay under user/%user/view/%mid.

Berdir’s picture

Great!

Will look at the patch when I find the time. Just a minor note.

- While it's probably not really a problem in this case, just want to say this anyway: is_numeric() is not the right function to check if you have an integer. See http://ch.php.net/manual/en/function.is-numeric.php. Stuff like "-123.346e12" will be accepted by that function but we obviously don't want that. I usually use if ((int)$variable > 0) because is_int() doesn't work either.

tylor’s picture

Good point, here's another patch with just that change.

Berliner-dupe’s picture

Hi,

sorry for my dim-witted question but with patch from #54 i can change the path from privatemsg to example .... user/benny/message ?

Berdir’s picture

Exactly. Use "user/%user/message" as the path if you want that. It will automatically show up as a local task in the user profile.

Berliner-dupe’s picture

Hi Berdir,

thank you for reply - very nice - this i need for my project.

Danke und schöne Grüße in die Schweiz ;-)

Matthias

tylor’s picture

@Berliner please test and let us know if anything breaks! ;)

Berdir’s picture

Status: Needs review » Needs work

Ok. Overall, the code looks good to me, below are a bunch of minor issues with coding style, missing documentation and a few things that can be slightly simplified. If you can fix them, great, if not, I'll do it myself (probably in a few days).

I think this is ready to be commited after that re-roll.

+++ privatemsg.admin.inc	16 Sep 2010 20:51:28 -0000
@@ -198,10 +207,15 @@ function privatemsg_admin_settings() {
+function privatemsg_admin_settings_submit($form, &$form_state) {
+  // Only rebuild menu if url prefix has changed

- No docblock
- comment is missing a trailing .

+++ privatemsg.module	16 Sep 2010 20:51:29 -0000
@@ -129,34 +129,38 @@ function _privatemsg_format_participants
+  $arg_count = substr_count($url_prefix, '/') + 1;
+  $user_arg_position = array_search('%user', explode('/', $url_prefix));

I think a comment that explains what these lines exactly do would be nice.

+++ privatemsg.module	16 Sep 2010 20:51:29 -0000
@@ -129,34 +129,38 @@ function _privatemsg_format_participants
-    'type'             => MENU_NORMAL_ITEM,
+    'type'             => strpos($url_prefix, '%user') === FALSE ? MENU_NORMAL_ITEM : MENU_LOCAL_TASK,

I think we can replace the strpos() call with $user_arg_position, it's not necessary to check this multiple times.

+++ privatemsg.module	16 Sep 2010 20:51:29 -0000
@@ -215,20 +219,47 @@ function privatemsg_menu() {
+  if (strpos($url_prefix, '%user') === FALSE) {

Same here.

+++ privatemsg.module	16 Sep 2010 20:51:29 -0000
@@ -215,20 +219,47 @@ function privatemsg_menu() {
+     if ($user_arg_position && ((int)arg($user_arg_position)) > 0) {

Only the second part of this condition is necessary.

+++ privatemsg.module	16 Sep 2010 20:51:29 -0000
@@ -241,17 +272,25 @@ function privatemsg_menu() {
-function privatemsg_user_access($permission = 'read privatemsg', $account = NULL) {
+function privatemsg_user_access($permission = 'read privatemsg', $account = NULL, $deny_if_other = FALSE) {

Documentation for the new param is missing.

+++ privatemsg.module	16 Sep 2010 20:51:29 -0000
@@ -241,17 +272,25 @@ function privatemsg_menu() {
+  // Disallow anonymous access, regardless of permissions

Missing point.

+++ privatemsg.module	16 Sep 2010 20:51:29 -0000
@@ -241,17 +272,25 @@ function privatemsg_menu() {
+  // Check that we are not viewing another user's private messages under their account page.
+  $url_prefix = variable_get('privatemsg_url_prefix', 'messages');
+  $user_arg_position = array_search('%user', explode('/', $url_prefix));
+  // Check if user either does not have read all permission or deny_if_other flag is set.

Comments should be wrapped at 80 characters.

+++ privatemsg.pages.inc	16 Sep 2010 20:51:29 -0000
@@ -14,17 +14,13 @@
-function privatemsg_list_page($argument = 'list', $uid = NULL) {
+function privatemsg_list_page($argument = 'list', $account_check = NULL) {

Documentation needs to be updated.

+++ privatemsg.test	16 Sep 2010 20:51:30 -0000
@@ -885,3 +885,161 @@ class PrivatemsgLinksTestCase extends Dr
+class PrivatemsgURLPrefixCase extends DrupalWebTestCase {

I think this should be named ...TestCase instead of just ...Case.

+++ privatemsg.test	16 Sep 2010 20:51:30 -0000
@@ -885,3 +885,161 @@ class PrivatemsgLinksTestCase extends Dr
+    return array
+    (
+      // 'name' should start with what is being tested (menu item) followed by what about it
+      // is being tested (creation/deletion).
+      'name' => t('Privatemsg URL prefixes.'),
+      // 'description' should be one or more complete sentences that provide more details on what
+      // exactly is being tested.
+      'description' => t('Test URL prefixes functionality for privatemsg.'),
+      // 'group' should be a logical grouping of test cases, like a category.  In most cases, that
+      // is the module the test case is for.
+      'group' => t('Privatemsg'),
+    );

I guess you copied this from the first class, but the documentation here is imho not necessary, we should just leave this away.

Also, the opening ( should be on the same line as array.

+++ privatemsg.test	16 Sep 2010 20:51:30 -0000
@@ -885,3 +885,161 @@ class PrivatemsgLinksTestCase extends Dr
+  function checkPrefix($url_prefix) {

Short docblock would be nice here.

+++ privatemsg.test	16 Sep 2010 20:51:30 -0000
@@ -885,3 +885,161 @@ class PrivatemsgLinksTestCase extends Dr
+    $this->assertText($author->name, t('Author name found on blocked user page.'));
+    ¶
+    // Since this user doesn't have read all, test that they are actually denied on other user's message page.
+    if (strpos($url_prefix, '%user') !== FALSE) {

A few empty lines with whitespaces. You can see them easily with dreditor.

Powered by Dreditor.

tylor’s picture

Added the documentation fixes as noted in #59, renamed some variables for clarity, and fixed a bug with the block links and send message to user links when viewed on the profile page (making sure that these links properly point to the current user's PM space).

tylor’s picture

Status: Needs work » Needs review

Bump test bot.

Berdir’s picture

Awesome, thanks a lot for your hard work on this!

Below are just a few notes for things which I'll fix myself before committing.

Another test would be great, but I'm going to commit this during the next few days if nobody reports an issue.

+++ privatemsg.module	21 Sep 2010 21:38:45 -0000
@@ -236,22 +269,38 @@ function privatemsg_menu() {
+ *   User account to check permissions. If null, default to current user.

NULL

+++ privatemsg.module	21 Sep 2010 21:38:45 -0000
@@ -236,22 +269,38 @@ function privatemsg_menu() {
+  // Check that we are not viewing another user's private messages under ¶

trailing whitespace

+++ privatemsg.pages.inc	21 Sep 2010 21:38:46 -0000
@@ -11,24 +11,20 @@
+ * @param $account_check
+ *   Account UID to check if current user has access.

remove UID, it is an account object.

Powered by Dreditor.

EndEd’s picture

I dont know if its a problem with this patch but after i use it i can see the link to send a message in Author pane anymore :/

Berdir’s picture

Strange, that should work because it uses privatemsg_get_link().

BenK’s picture

Status: Needs review » Needs work

At Berdir's request, I did some testing of the patch in #60. I noticed that if you set the prefix to "user/%user/something", then the "Send this user a message" link will only show if a user has "Read All Messages" permission. In fact, no other links on the profile page will show unless the user has this permission.

I chatted in IRC with Berdir about this and he's currently working on a fix...

--Ben

BenK’s picture

Berdir also asked me to take a look at the label and description on the admin setting. Here is what is currently displayed:

Label: URL prefix

Description: 'Specify the base URL prefix for all privatemsg pages, %user will be replaced with the current user ID (for example, "user/%user/messages"). Default is "messages".'

Here's my suggested revision:

Label: Base URL path

Description: 'Specify the base URL path to be used for all Private Message pages. Note that %user can be used as a placeholder for the current user's ID. Possible paths include "user/%user/messages" (pages appear as children of the user account page) or "messages" (the default path).'

--Ben

BenK’s picture

Also, I've done some testing of the patch in conjunction with the pm_email_notify, pm_block_user, and privatemsg_filter sub-modules.... everything seems to be working fine! :-)

--Ben

BenK’s picture

Sorry for the multitude of comments, but one more thought: If you use 'user/%user/messages' as your URL prefix, then the "Messages" link is removed from the navigation menu. But I think some sites may want to have this 'Messages' link (myself included) even though 'user/%user/messages' is the prefix. The problem is that you can't easily create this navigation link manually because the path involves a token.

So what about leaving the 'Messages' link in place even if the site admin specifies 'user/%user/messages' as the URL prefix? Sites that didn't want this link could easily disable it from the Navigation menu settings page.

--Ben

Berdir’s picture

Status: Needs work » Needs review
FileSize
38.43 KB

Ok, here is an updated patch.

- The problem with the links is that privatemsg_user_access() currently has code that does some checking of the current URL. If you're viewing at another profile, the function always returns FALSE even for links that point to your own privatemsg pages. I first thought of adding yet another argument to that function, but that results in really ugly code. What I have done now is splitting up the function into privatemsg_user_access() and privatemsg_menu_access(). Only menu_access does the URL checks now and on the other side, that function can always safely use global $user, so both functions only have two arguments. That's much nicer than a single function with 4 arguments. I think this also fixes #63.

- Another minor thing I fixed is that the title calback function doesn't take into account on which profile you are and always displays the number of your own unread messages.

- Also changed the setting label/description.

- Changed the things I noted in #62.

Too tired to extend the tests now...

Berdir’s picture

Version: » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

Ok, I decided to commit this, this has waited long enough :)

We need to revisit this in 7.x-1.x-dev, maybe it can be solved through hook_url_outbound/inbound_alter() there.

smoothify’s picture

Thanks Berdir et al for the work into this, great to see it committed! :)

Funnily enough the site migration that I needed this for has been delayed too and so I'm just about to revisit this and so its perfect timing in the end.

Thanks again

Ben

Michelle’s picture

Thanks! I haven't gotten to it, yet, on my site, either. Glad to know this will be waiting for me when I do. :)

Michelle

tylor’s picture

Awesome! Thanks Berdir! Glad to see this in :)

oversleep’s picture

Looking through the various versions, it seems like this feature is only enabled for 6.x-2.x.

Specifically, the added function "privatemsg_menu_access" is not included in later releases. There also seems to be no admin setting to update the URL prefix, even in the 7.x-1.x-dev release that is listed on the project page as "Contains all new features of 6.x-2.x".

Hopefully this isn't actually the case, but if anyone else wants to move the base directory to "[user-id]/messages", they might want to save themselves the headache of thinking that it's currently possible (as of the 7.x-1.x stable and 7.x-2.x-dev releases at least).

---

My site uses Views blocks that draw the user ID from the URL, so Privatemsg would naturally need to implement the user ID in its URLs as well.

Attempted workarounds thus far:

1) Pathauto/Path alias: doesn't redirect correctly using "[user:uid]" replacement;
2) hook_outbound_alter()/hook_inbound_alter() actually re-write the URL which removes the UID.

Given the number of requests by various people here, and the fact that there is already a patch for 6.x-2.x, it's surprising that this simple, crucial feature seems to have been dropped (or not implemented at all) for 7.x.

Any functioning solutions out there in the wild would be highly appreciated, both for myself and anyone in the future who happens past this thread in search of an answer that actually works. I'd be thrilled if I was missing something that was hiding in plain sight.

-Mania-’s picture

+1 to have this in 7.x as well!

nijk’s picture

I would love to see this feature implemented in D7. To be honest I'm amazed that such a good module doesn't allow for customisation of URL aliases. Hope to see an improvement in this area soon as the issue seems to have been around for quite some time...

rfay’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
21.57 KB

Here's a patch for 7.x-1.x, basically a cherry-pick of the original commit, 55290827, with changes to make it work with D7. Seems to be working for me.

I'm going to set it to "needs review", but it will fail testing because testing is broken on privatemsg, see #2505645: Request: Turn off automated testing until it works

Status: Needs review » Needs work

The last submitted patch, 77: privatemsg.configurable_menu_location_561036_77.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 77: privatemsg.configurable_menu_location_561036_77.patch, failed testing.

rfay’s picture

Status: Needs work » Needs review
FileSize
25.67 KB

Here's an improved patch - I had lost a bit of privatemsg_menu_access() in the previous one.

This also makes a slight improvement to the user cancellation test, which was failing locally - probably because of a slower machine. The existing test was manufacturing its own user cancellation link instead of using the one sent by mail, which is what should be used. So I used @Berdir's excellent Stackexchange answer from 2011 to make it use the actual link, and then the tests passed locally.

rfay’s picture

This patch fixes the privatemsg URL token, which was incorrect.

rfay’s picture

Sorry, found I'd omitted URL options. Here's the patch.

rfay’s picture

Improved patch, still missed some urls

Status: Needs review » Needs work

The last submitted patch, 84: privatemsg.configurable_menu_location_561036_84.patch, failed testing.

rfay’s picture

Try to fix test...

rfay’s picture

Status: Needs work » Needs review
rfay’s picture

Found another couple of places that need the URL rewrite. Full patch attached.

Status: Needs review » Needs work

The last submitted patch, 88: privatemsg.configurable_menu_location_561036_88.patch, failed testing.

rfay’s picture

And current version, haven't fixed tests yet.

ivnish’s picture

Status: Needs work » Closed (outdated)
andypost’s picture

Status: Closed (outdated) » Needs work