I noticed this bug while testing the tagging feature in Private Messages Rules. Here are the steps to re-create:
1. Give the authenticated user role the 'tag private messages' permission. Do not give this user the 'filter private messages' permission. Create "person1" as an authenticated user.
2. Enable the "Tags" column to show up at /messages. Create a tag called "Tag1".
3. Login as person1 and tag a message as "Tag1".
4. Return to the /messages page and click on the "Tag1" link shown next to the message in the "Tags" column.
5. You will be taken to the "/messages?tags=Tag%201" page and the filter widget will be displayed (even though person1 does not have permission to see it).
6. If you try to actually use the filter widget (by entering some filter criteria and clicking the "Filter" button), you will get the following error messages:
Recoverable fatal error: Object of class stdClass could not be converted to string in privatemsg_filter_create_get_query() (line 461 of /Users/benkaplan/git/drupal/sites/all/modules/privatemsg/privatemsg_filter/privatemsg_filter.module).
--Ben
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | privatemsg_tag_filter_fixes_6.patch | 10.76 KB | berdir |
| #11 | privatemsg_tag_filter_fixes_d6.patch | 10.76 KB | berdir |
| #8 | privatemsg_tag_filter_fixes3.patch | 11.76 KB | berdir |
| #3 | privatemsg_tag_filter_fixes2.patch | 6.76 KB | berdir |
| #1 | privatemsg_tag_filter_fixes.patch | 3.5 KB | berdir |
Comments
Comment #1
berdirNice catch. These are actually two separate bugs, that second always happens when you filter by tags through the form.
- The patch explicitly gives users with the tag permission also access to filter by tags (only by tags, the current code allows everyting, it just doesn't show the form) but hides the form if you do so.
- Also fixes the mentioned error.
- Adds some comments.
Comment #2
BenK commentedHey Berdir,
The patch works great, but I noticed a few things:
1. If you try to filter by a role in the Participants autocomplete field (assuming you have the permission), you get the following error:
Warning: array_flip(): Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load() (line 167 of /Users/benkaplan/git/drupal/includes/entity.inc).
Recoverable fatal error: Object of class stdClass could not be converted to string in DatabaseStatementBase->execute() (line 1962 of /Users/benkaplan/git/drupal/includes/database/database.inc).
2. When a user without the "filter" permission clicks a tag on the /messages page, they are taken to page filtered by that tag... with the filter form hidden to them. This all works great. But because the form is now hidden, it's a bit unclear that they are actually on a page that is filtered. It looks exactly like their regular /messages page and they might even be worried that some of their messages are now gone. So can we print a title at the top of the page that says something like "Messages filtered by [Tagname]"? Or else maybe print a drupal_set_message saying that the page has been filtered by the tag? Anything to make it clear what they are looking at would be fine.
3. Does the "save filter" button actually do anything currently? I might just be looking in the wrong place, but I don't see any place where the filters are actually saved.
4. When selecting a tag to filter by, is there any way to filter by no tags (like just a blank space that is selectable at the bottom of the list). I know a user can click the "Reset" button, but I kind of expected to be able to choose "nothing" from the form.
I know I'm venturing away from the initial thread topic in my last couple of comments. Let me know if you want me to open a new issue for those things. I just thought it was convenient to mention them here.
Thanks,
Ben
Comment #3
berdir1. Fixed the error. But filtering by recipient types other than users is currently not supported and would require quite an effort. So it currently simply ignores them.
2. Added a message, but this is quite a hack :)
3. They are saved in the session. When you just click filter, the filter arguments are passed with GET and are only active as long as you stay on the current messages page (paging works, though). Clicking Save filter keeps the filter active until resetted manually.
4. You can unselect selected tags by clicking them with CTRL pressed. We could probably add a empty option, but it's kind of default to only have them in simple (single value) selects. compare the default and advanced issue search form for example.
The patch also fixes the problem that the paging doesn't reflect the filter but this requires #922380: Custom PagerDefault count queries getter method is protected.
Comment #5
BenK commentedShould I test new patch even though it failed auto testing?
Comment #6
berdirIt fails because of the missing core patch, so you can test it if you apply that patch.
Comment #7
BenK commentedHey Berdir,
I took a look at the latest patch. Additionally, the core patch you referenced was recently committed by Dries, so I had a chance to test with the patch in place. Here's what I found:
A) If a user doesn't have "Tag" permission, then the links to go to the 'filtered by tag' page don't work. Why would such users have tags if they can't tag things themselves? Well, because tags can now be added automatically via Rules. So either we should make it so the tags for these users are not links, or else the links should actually work (bring users to a "filtered by" page without the tag form at the bottom of the page).
B) If you visit the 'filtered by' page and then remove a tag using the tagging form, then the drupal_set_message says the page is filtered by the tag, but it's not. The complete message list is shown instead.
C) If you click "Save filter" everything works properly, but now the following error message is displayed at the top of the page:
Additionally, when I did this, the actual tag name within the drupal_set_message is missing. So the set message actually reads: "Messages filtered by tag . Remove filter."
D) When visiting the "filtered by" page by clicking on a tag link, the drupal_set_message is displayed twice if you re-filter by any criteria. Basically, it looks like this:
E) Change this string: 'Messages filtered by tag Notice. Remove filter.' To this:
'Messages tagged with Notice are currently displayed. Click here to remove this filter.'
Most of these issues are quite minor so we should be pretty close to being done with this....
--Ben
Comment #8
berdirA) The column is not supposed to be visible for them in the first place in my opinion :) For example, they can't see tags on the view thread page either so it's only consistent. Having read-only access to tags is a separate feature. Changed.
B) & C) & D) The permission check there was wrong. The message is supposed to show *only* when the filter form is not displayed. Fixed.
E) Changed.
Also added some tests. They don't test everything but they verify that the links work, that the filter widget doesn't show unless you have the permission, that paging works and that
Comment #9
BenK commentedThis works great. All errors are fixed. This is RTBC! :-)
--Ben
Comment #10
berdirCommited, we probably need to backport at least the visual stuff to 6.x-2.x too.
Comment #11
berdir.. and another backport patch..
Comment #12
berdirSame patch without d6 suffix.
Comment #13
berdirCommited.