restore forum link functionality via working tracker URLs

alternatively, you could have a little filter form at the top of the page, just like admin/content/node. you'd have a dropdown (or even a multi-select) for what types you want to filter by, and potentially the taxonomy/category filter, too. that'd obviously require a different patch, but might make for a more useful (and consistent) UI. just a thought. if there's interest, maybe i'll roll a patch for this.

that said, i'm definitely in favor of adding something, even if it's the existing patch here, not my alternate UI proposal. haven't had time to review/test this carefully, but +1 in principle.

"My recent posts" doesn't work any longer with this patch.

+  $type = $arg;
+  $search_nodes = node_get_types();
+  if (array_key_exists($type, $search_nodes)) {
+    $add_on_sql = " AND n.type ='$type' ";
+  }
+  

1. There's no need for the "$type" variable; simply use $arg.
2. There's no need to do any of this logic unless $arg is not an int.

Oh, and 3. some comments on what that chunk is for would be nice. // Restrict tracker by node type, if found. or something

And though this is a small API change, I support its inclusion. That "Active forum discussions" thing has been confusing forever. Even better would be if I could do tracker/1/forum to find a list of "My" forum discussions.

Here's an alternate approach. It places a &type=X querystring argument (inspiration from Moshe's "node_type_filter" module).

1. No API change! :D
2. Can now restrict for both tracker and tracker/uid
3. Also supports restricting by multiple types (&type=forum,story)

In addition, I rolled in the fixes for the forum module links.

Marking http://drupal.org/node/55251 a duplicate of this bug, since the goal of this patch is to fix that bug.

Title change.

chx was kind enough to point me to the array_intersect function, which greatly simplifies the code. :)

Oops! &type=8237489w7e89fsd789f7s9df should not trigger the extra SQL.

Going to add a UI to this patch, per dww's review on IRC. Should be able to cook this up later today.

FYI: patch from #13 works as advertised. i think adding a UI to the tracker page to make use of this functionality is worth doing to take advantage of it. it's really going to be a great usability enhancement.

the only thing i'm on the fence about is what happens when you do type=uninstalled_type. currently, you see all posts, as if you didn't put the type restriction on at all. i'd prefer that it showed you no posts, since there are no posts that match that type. but, it's not *that* important, especially once we have the UI (since the UI itself will only present valid types).

otherwise, this is great. i hope it makes it in.
thanks!
-derek

Actually, no. I'm going to save the UI for a separate patch, because I can't atm think of a way to add this in without adding in another callback function and thus changing the API, and I don't want to see this held up over that. Those links in the forum module confuse the living crap out of people and that really needs to get fixed this release.

I like the behaviour of #13 better, because it shows a definite difference between "hey this didn't actually do anything" vs. "hey there are no nodes of type X." But #12 does the behaviour dww describes. Whichever the core committers fancy will work.

- I'd like to see the usual %s used instead of putting the node types right in the query.
- As long as we are changing the link text, why not remove the period, those aren't full sentences.

drumm: I struggled with this for bit, but can't get it to work.

$sql  = "SELECT .... AND n.type IN ('%s')", ... implode("', '", $node_types);

This makes the SQL:

SELECT ... AND n.type IN ('forum\', \'story\', \'page')

I searched throughout core for instances of this type of behaviour, and came across the following two approaches that existing modules are doing (aggregator and taxonomy, respectively):

db_query('DELETE FROM {aggregator_category_item} WHERE iid IN ('. implode(', ', $items) .')');

$sql = 'SELECT DISTINCT(n.nid), n.sticky, n.title, n.created FROM {node} n INNER JOIN {term_node} tn ON n.nid = tn.nid WHERE tn.tid IN ('. $str_tids .') AND n.status = 1 ORDER BY '. $order;

Which one do you prefer? placing the implode() directly in the SQL, or storing the imploded value in a string and placing that directly in the SQL?

My mistake. Both of these are numeric, so don't apply here.

This was the cleanest I could make it. Punctuation on those forum links was taken care of, too.

http://drupal.org/node/83751 was marked as a duplicate of this bug.

Still cleanly applies to HEAD.

still applied to HEAD. it looked fine, and worked perfectly, so i was about to set to RTBC.

however, i decided to re-roll with a slightly different approach to the SQL. in this version, we create a $where variable with the additional stuff only if type is in the REQUEST. we include that single $where in all the relevent queries. this way, the additional cost of the WHERE clause to filter by specific types (the AND n.type IN ($node_types_list)) is only added to the query if we need it (instead of always adding the AND n.type IN ... but giving it the full list of all possible types).

so, there's no way this query could slow down if you're not adding the additional stuff to your URL.

sadly, there's still no UI for this, but at least providing this support will a) fix the evil forum links and b) allow contribs to use this functionality.

RTBC?

here's an even cleaner, prettier version of the code.

also, in IRC zen pointed out http://drupal.org/node/17119 which is an ancient issue this is really duplicate of (though it uses a different approach with URL args, not ?type=, so, i'm not marking this or that as duplicate just yet). ;)

Oh, awesome! I'm so happy someone found this issue again! :D I can't count the number of times people have reported that these links on the Drupal.org forums are broken.

The lack of UI is kind of a bummer, but I imagine a very simple contrib module could add this in. The important thing is that it fixes this long-standing bug.

Tested both "my" tracker and normal tracker with:

• type=forum
• type=forum,book
• type=lkjsdhjkshdjhskjdf
• (no type)

Works great. RTBC.

And since there is no UI, this is merely a bug fix, so marking it as such. I would mark it critical due to the sheer volume of people reporting this as a bug, but it's been broken for so long that it can probably stay normal.

This would allow someone with 'administer content types' to execute an SQL injection attack. Though I wouldn't know a useful attack in only 32 chars here, I don't like it.

Simply create a content type of type something' (note quote) to reproduce.

Sorry, about that.

I meant "administer content types" permission.

After creating the content type something', visit tracker?type=something'

I couldn't get type=something' to do anything differently, but you're right that that's not good.

What's *especially* not good is it seems we're not validating content names/types at all! I'll go file a critical bug about that.

Will this fix it? I loop through the array of node types about to be sent to the SQL query and check_plain on them.

check_plain is about making strings safe to use in html. Variables should be passed to db_query as parameters. The example on Writing secure code falls behind the block so I'm re-posting it here all broken up:

$args = $content_types;
$placeholders = array_fill(0, count($args), "'%s'");
$sql  = 'SELECT n.nid, n.title FROM {node} n INNER JOIN {term_node} tn ON n.nid = tn.nid WHERE n.type in (' ;
$sql .=  implode(',', $placeholders); 
$sql .= ') AND n.status = 1 ORDER BY n.created DESC';
pager_query(db_rewrite_sql($sql), 10, 0, NULL, $args);

Proper link Writing secure code and status, sorry.

Posted other issue here: http://drupal.org/node/99180

And thanks for that!! I hadn't seen that example before. Rolling a patch now.

Ok my brain hurts trying to write this in such a way that it's not completely horrid to read.

I think I'll wait until dww gets back on and then talk w/ him about the best way to do this.

good catch, heine! ;)

how's this? instead of messing with contortions for proper cramming of %s into the query, why not just do what %s accomplishes: call db_escape_string(). new patch adds exactly 1 line:

    $node_types = array_map('db_escape_string', $node_types);

i don't see anything wrong with doing this, instead of messing with %s. if we ever change our minds about what it takes to make a string safe for use in an SQL query, we should change db_escape_string(), not how we handle %s in db_query(). there are already other places in core that call db_escape_string() directly, when it makes code easier to understand.

That's SO much better than what I was trying to do. ;) However, it doesn't seem to work anymore. :(

Also, we have a notice generated if $where is not defined. Probably should be initialized to '' at the beginning.

Never mind.. something exploded with my install. :P Patch works fine.

Patch is the same, but cures the notice.

And this one fixes some very minor spacing issues and, as an extra added bonus, removes my settings.php from the diff. :P

This is getting there. The initial view of All recents posts just shows 'forum topics', what a novel idea! :D But...when clicking on My recent posts, where you'd expect to see all forum posts done by you, you actually see all node types by you.

So we need to at least persist the node type between the two links there. This seems a little dirty since there should be some UI to let the user UNfilter by type and see all recent posts or all MY recent posts. But this gets into messing around with the hook_menu() items and we should probably think of a good solution to get tracker a bit beefier to support different combos of node type, uid filters in a user-friendly way.

But this is such an ugly bug that has been around for soooo long so this fix is better than nothing. I'd be alright with this going in.

This patch just fixes two spaces and two double quotes because hey, I can help too, right?

Yeah, the not persisting thing is a bit awkward, but there doesn't seem to be a way to implement that without breaking APIs, and "in theory" this page would be better generated by Views anyway, when we get that into core. ;)

But at least now, both the "Active forum discussions" and "My active forum discussions" links do the right thing, so people can either bookmark them, right-click > open in new tab, or click on the one they want.

Tested again with RobRoy's newest patch (thanks for fixing those things up), and marking RTBC again. Patch attached is just a re-rolled version of RobRoy's that applies without offsets.

ahem. RTBC. :P

Cool, yeah agreed on the Views stuff. This is a good solution for the moment. Thanks guys.

minor cleanup of a few extra spaces that snuck in. ;) still RTBC as far as i'm concerned...

This is not a bug report but a feature request. Will have to wait for Drupal 6 or later.

This is a bug report IMO. Having a link to recent forum topics that goes to a list of ALL content is a bug. Please correct me if I'm wrong. If this is too much to get into D5, then let's just kill that link as it is HIGHLY confusing for users.

100% agreed with RobRoy. I can't begin to count the number of people who report in the forum, the issue tracker and in #drupal-support that "The links at the top of the forum are broken."

If this is too "feature-y" to get in now (would be a shame, since "My recent" and "Active discussions" are common functionality in other forums, and the fact that these links are there leads me to believe that we want them too), then here's an alternate patch that strips the links out altogether, which is 300% better than leaving them there when they don't do what they say they are going to do.

Since this is themeable then it isn't too big of a deal to put it back in for some sites.

Committed to HEAD.

it isn't too big of a deal to put it back in for some sites.

except that there's nothing to link to, since the tracker doesn't provide any urls that work. :( sites that want this functionality (which we've claimed to have supported since before i was around) have to install views or xtracker or some other contrib to provide a tracker where you can filter by node type. *shrug*

-derek

I'm still all for getting the tracker patch in for the time being, so my +1 on that, but having no links is better than having broken links. :)

agreed, i don't think removing the broken links was a bad idea, i just would have prefered to see them fixed, instead. hence my new patch. of course, it's ultimately up to drumm/dries/unconed to decide, but i just wanted to make it easy for them if they choose to restore/fix the functionality, instead of just removing it. ;) plus, it would be great if contrib modules could more easily extend the existing tracker, instead of re-implementing their own, and this patch helps make that possible.

cheers,
-derek

"tracker/$user->uid" should be 'tracker/'. $user->uid

I'd like to see the where clause building use the database API and keep the variables as %[sd]. '... ('. implode(',', array_fill(0, count($excluded_sectors), "'%s'")) .') ...' is how I did this on another project. Then $excluded_sectors was array_merge()ed into the final argument to db_query().

Looks like that is inactive. Status needed.

Considering that no new tasks will be completed against D5 and that no one has shown any interest in this issue for a long time, I am closing this issue ticket.