Tagging architecture.

I am in favor of approach 2.

We can always optimize it later in the game once we have a very good understanding of this functionality. plus approach 2 sounds more flexible.

I wonder if we should leverage taxonomy module tagging functionality for privatemsg. that way we dont have to deal with categorization and filtering and only provide integration.

what do you think?

1:
As you said, one way to do it would be to use module_exists('pm_tags') all over the place.. Easy to implement, but the own module doesn't really make sense then.. you could also add a configuration option and simply have a include file for all those tag functions..

About the other way, You've already named it: Hook things in. Or: Provide your own hooks which can be used by other (sub-)modules. Possible hooks would be: insert, delete, select, read/view (list, thread, new message, block... ),

This would of course be way more complex but would make it easier to add more sub-modules later. Actually, modules like notification could provide integration with privatemsg without the need to change both modules...

3: I don't think default tags do make sense as they depend on the type (and language!) of messages on a website

module_exists in this case is a bad solution. lets instead implement hooks which will make it easier for other modules to interact.

Because you count the number of selectable tags inside pm_tags_dropdown, drupal_get_form still displays an empty form if there are no tags.

This adds some unecessary markup (and an empty line) to the site so I think it's better to check the tags befor calling drupal_get_form()

This should work

/**
 * Count used tags from current user
 */
function pm_tags_count_tags() {
  global $user;

  $sql = 'SELECT COUNT(*) as count FROM {pm_tags_index} pmti LEFT JOIN {pm_tags} pmt ON pmti.tag_id = pmt.tag_id WHERE pmti.uid = %d GROUP BY pmt.tag_id';
  $result = db_fetch_array(db_query($sql, $user->uid));

  //$result is false if there are no entries because of the GROUP BY statement
  if ($result === false) {
    return 0;
  }

  return $result['count'];
}

/**
 * Implementation of hook_privatemsg_alter().
 */
function pm_tags_privatemsg_alter(&$content, $state, $account) {
  switch ($state) {
    case 'list_messages':
      if (pm_tags_count_tags() > 0) {
        $content['tags']['content'] = drupal_get_form('pm_tags_dropdown');
        $content['tags']['#weight'] = -5;
      }
      break;
  }
}

Apart from that, it does work for me but as you said some parts are not really pretty at the moment, especially the thread display. I'll have a look at it...

Update for my change in [#/324406] and a simple E_NOTICE-fix*

* Yes, I'm developing with E_NOTICE on ;)

subscribing

The only filter example that I know of[1] (atleast in core) is at admin/reports/dblog. I'm not sure how easily it can be adopted, but what you should take from it is multi-select and the reset button (yes of course, you can click messages again, but I have now idea how clear that is for an user).

[1] Update: Well, another examples are the (base) Advanced Search and the project advanced search pages. Speaking of search and filtering, why not add a genereric search field to search title and content, too...

Ok, here is my approach.... Handling of the filter form is quite similar to what dblog provides.

- PostgreSQL Support (Replaced " with ' for strings, removed backticks, changed the INNER JOIN so that it has a ON -> Depends on the PostgreSQL Support patch)
- UI provides now author and tag multi-selects and a generic search textfield (just for subject)
- Filters are stored in the session and there is a Reset-Button (As I said, similar to dblog)
- If atleast one filter option is active, the filter form is displayed by default, the title changed ("Filter Messages (Active)", better ideas ?) and the current filters are pre-selected
- GET-Parameters are still supported, they are checked and stored in the session at the beginning of the form building. Still sing " " (aka +) to delimit multiple authors/tags. And for authors and tags both the names and the Id's can be submitted, only the Id's are stored in the session. A GET-Parameter does reset (only) the respective filter type and is displayed in the filter form.
- Reworked the list_alter hook to use the SESSION values and IN "(x, y, ...)" instead of = x to support multiple tags and authors without multiple joins

Open/Unclear things...
- The current implementation of the subject search is not nice, % are hardcoded and have to be escaped twice to get past assembly_query and db_query (I don't like the current approach of assembly_query to insert the params with printf because that does not escape anything... )
- The usabilty needs to be improvided, It might be confusing for some users.
- Maybe reset a filter if no results are found (should be possible be checking it in the list_messages hook) ?

I'm attaching a diff too so that it is easier to see the changes.

Feedback?

1. Good catch, that's what I actually wanted, but forgot to change. Maybe we should display the form only if there is atleast one message ($content['messages'] non-empty). In this case, we would also have atleast one author.

The "problem" with GET is that the form does send the values as an Array, so you end up with: ?tags[1]=tagX&tags[2]=tagY, this does not look nice so we would need to change that if we want a nice GET-URL... and change it back in the list-alter hook. And you don't have control over the values, so a user could change the values so that we need to re-check them every time (including loading the Id's if we display the names in the URL).

But it is possible with one JOIN and special characters can be escaped with urlenocde. It would however need one query more to get the ID's.

And if we store the filters in the session it is by default permament. I wanted to show that with the open form. This is also how it is done with dblog. Maybe we could provide both ways, something like:

[ Filter once ] [ Save Filter ] [ Reset Filter]

i like the idea of URL parameters because it will enable us to save filters and make them reusable later on. maybe even allow users to define default filter ? that will also enable us to study most common filtering that users do and be able to optimize based on that.

None of that needs (nor should) happen now, just some forward thinking of what URL arguments will enable us. good stuff guys :)

> Is it not possible to modify the submit action to send out an imploded version? tags=x+y+z
Not directly, sending the form is the task of the browser. However, what would be possible is parse the sent values and rewrite them as a "easy to read" url and then do a drupal_goto, similar to what had you before.

>I just like hacing the ability to type in a url to get what I want...
It is currently possible to write/create Filter-URL's, but they are stored in the session after being accessed first. But I can see why it could be useful to display the url's.

I'll try to provide a improved version as soon as possible. I think the best way is to create a function like privatemsg_filter_get_filter() which checks Session, URL (and maybe at later point saved filters, see below) and returns the filter array so that the list_alter hook can work with it.

@litwol
Saving/storing filter sounds interesting, but is currently not that important imho. Some ideas for later..
- Save a filter with a name and display it somewhere (for exampe in the block)
- Add some special filter options as checkboxes, for example:
-- Display only unread messages
-- Display only unanswered messages
- Provide some default filters. This would for example allow us to (finally ;) ) remove the sent messages tab and instead provide a filter displaying the messages where the user is an author.

Ok, here is a new try.

Some changes:

- Button-Layout: [ Filter ] [ Save Filter ] [ Reset ] (Reset is only displayed when a Filter is active)
- GET-Values are no longer saved in the session
- Filters can now be used once as a GET-URL (Filter) or saved in the Session (Save Filter)
- Added a new method privatemsg_filter_get_filter, which returns GET or session parameters (If atleast one GET-Parameter is present, session is ignored)

Some things are not really nice, for example I am using the first query_args entry in list_alter as the user_id. And LIKE %...% is still very ugly, but I will open a new issue about query_args (Security related)

Forgot to add some of nbz's changes.

- Search -> Search Subjects
- Display the form if there are no tags used
- Display the form only if $content['list']['content'] is not empty or a filter is set

As clean as this solution may be i still cannot agree with the approach:

<?php
      $fragments['inner_join'][]  = "INNER JOIN {pm_tags_index}";
      $fragments['where'][]       = "pmti.thread_id = pmi.thread_id";
      $fragments['where'][]       = "pmti.uid = pmi.uid";
?>

These are join points and therefore should be inside inner_join:

<?php
      $fragments['where'][]       = "pmti.thread_id = pmi.thread_id";
      $fragments['where'][]       = "pmti.uid = pmi.uid";
?>

where as this is a conditional branch and should be inside where:

<?php
      $fragments['where'][]       = "pmti.tag_id IN (". implode(', ', $filter['tags']) .")";
?>

And PostgreSQL *needs* an ON for an INNER JOIN, so this needs to be there. Replacing ON with WHERE is not a good idea in general because i'm quite sure it is slower and may lead to different results.

privatemsg_filter_get_filter should take care of security issues for now. author's and tags are used if they are present in $tag_data or $author_data and the search value is escaped.

Your example with # is actually not related to security, but everything behind # is not even sent to the server but is instead used by the browser to "jump" to an < a name="xy" >-tag, if present. And if you use for exampe ?tags=some#tag, the server will send a GET ...?tags=some... but some is not found in $tag_data and therefor ignored.

Also i just realized something, code like this is a huge security risk:

<?php
      $fragments['where'][]       = "pmti.tag_id IN (". implode(', ', $filter['tags']) .")";
?>

we shouldnt place tags directly into the query. we must use proper %d and %s variable replacements to avoid sql injection attacks.

@litwol

Yes, I know. The downside of IN is that is not easily possible to use replacements because you don't know the number of elements there are. It is in this example however not a real security issue, because $filter comes from privatemsg_filter_get_filter which only returns GET-Parameters that are real tags used by this user ($tag_data) and transformed to the key. But there *is* a security problem, because the sql generating function does not use proper escaping either.. it simply inserts the arguments with vsprintf. I'll open a new issue about that as stated above.

However, nbz raised another question... do multiple authors/tags mean OR or AND ? If the latter, IN does not work anyway and I have to rewrite it so that there are multiple WHERE parts (Wich is not so complex, it's more a definition problem than a technical one)

And yes, with the ON we also need the array_unshift to be sure that the pm_index is always the first INNER JOIN.

The reason i went with sprintf is that it seems to cast variable types, %s and %d. so i'm not sure of the security complications that you are referring to. also we cant predict what kind of query rewriting some one may do so its best we stick to standards.

to find out how many %s or %d we need is rather easy, you can use http://us.php.net/array_fill with count($filter['tags']) number of times.

Yes, sprintf is fine for int values, but for strings, casting is not enough, we need to escape the strings...

To test it, simply remove the db_escape_string call in privatemsg_filter_get_filter and search for something like " string with ' ". (the search string *does* use %s).

This will produce the following sql error:

You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near '%') GROUP BY pmi.thread_id ORDER BY pmi.new desc, timestamp DESC LIMIT 0, 25' 
at line 1

query: pager_query /* Berdir : pager_query */ SELECT pmi.thread_id, pm.subject, MAX(pm.timestamp) as timestamp, MAX(pmi.new) as new FROM pm_message pm INNER JOIN pm_index pmi ON pm.mid = pmi.mid WHERE (pmi.uid = 1) AND (pmi.deleted = 0) AND (pm.author IN (29)) AND (pm.subject LIKE '%hackme'%') GROUP BY pmi.thread_id ORDER BY pmi.new desc, timestamp DESC LIMIT 0, 25

You are absolutely right, in that case we must replace using sprintf with :

<?php
  _db_query_callback($args, TRUE);
  $query = preg_replace_callback(DB_QUERY_REGEXP, '_db_query_callback', $query);
?>

Created a patch of your code, see #339565: Security issue with _privatemsg_assemple_query

Another question with that last stated where query... does it change the tagging from an AND to an OR (Where the tag id matches any one of that array, instead of ALL of them)? I am quite sure we want an AND.

IN means "is one of them" => OR

If we want AND I have to change that query anyway.

I'm not sure what to do... any similar drupal form i know (dblog -> type, issue search -> status) does use OR *but* an issue can have only one status, a log entry only one type and so on. But a thread can have multiple tags and authors, so it does probably make sense to use AND.

Yes, it seems so.

Here is a updated zip.

Changes:
- Re-added the multiple joins code, a bit modified however... it does use ON instead of WHERE's
- Also doing multiple joins with the users table to have a similiar functionalily for author filtering
- Added primary keys for pm_tags and pm_tags_index.
- Slightly changed the search parameter so that the % only have to be escaped once.

One thing I'm asking me is if we should allow private/personalized tags, so that each user could have his own set of tags. This would effectivly mean to move the uid attribute from the pm_tags_index table to pm_tags. Just an idea...

Thank you all for hard work. its been commited now: http://drupal.org/project/cvs/3279