Category filter not working on admin/node

Patch attached. Please review.

Thanks
-K

I just realised that I hadn't tested nested hierarchies.. and it broke :/

Patch recalled.
-K

False alarm. The original patch still applies.

However, the "category is [category name]" is still b0rked. This is due to the same nested array issue as with the actual filter issue (fixed with the above patch). Reasons why fixing this is icky are:

-The options will again have to be flattened. However, form_options_flatten only returns the keys and not the values [returns 1 for values].. The values are what we want here..

-Even if a new flattening function is added which preserves both keys and values and their association, the actual values for the category options are prefixed with "----"s for indicating hierarchy..

-Last option, is to use form_options_flatten and get the keys/tids and load the taxonomy term using that.. But this would imply a check for $type=='category', which is again going to make things ugly..

This filter also breaks when the filter is refined to check for multiple categories.. The JOINs are broken..

I will look at this later, but for now please review the above patch..

Thanks
-K

Patch attached:

-fixes the JOIN issues for the category node filter. Nodes with multiple parents can now be filtered successfully.
-displays the category name by loading the taxonomy term. I couldn't think of a cleaner solution for this..

Reviews appreciated :)

Thanks
-K

taxonomy_get_term -> module_invoke('taxonomy', 'get_term');

Cheers,
-K

Thanks,

how soon this will get commitedt to thead HEAD .. ?

T.

as soon as someone tests it throughly. for example, do combined filters work? various taxonomy options?

I read the code and found it good (I suggested calling the taxonomy through module invoke).

Ok,

Thanks for this one, patched and now testing so far everything works as it should.. will live with this one for a while :) as far as I understood it doesn't affect any of existing content it only works as a filter for getting faster to the existing content and there wont be any possible damage...

T.

Ok,
Thank you for the good work. I tested the patch with combined filters and various taxonomy options; it works properly so far.

While you are changing node_filter() :

  if ($taxonomy = module_invoke('taxonomy', 'form_all')) {

... omitted code...
    $filters['category'] = array('title' => t('category'),
      'where' => 'tn%d.tid = %%d',
      'options' => $terms,
      'join' => 'INNER JOIN {term_node} tn%d ON n.nid = tn%d.nid ',
  }
  if (isset($filters['category'])) {
    $filters['category']['options'] = $taxonomy;
  }

..what about cleaning it a little more? I see no side-effects to this :

  if ($taxonomy = module_invoke('taxonomy', 'form_all')) {

... omitted code...
    $filters['category'] = array('title' => t('category'),
      'where' => 'tn%d.tid = %%d',
      'options' => $terms,
      'join' => 'INNER JOIN {term_node} tn%d ON n.nid = tn%d.nid ',
    );
    $filters['category']['options'] = $taxonomy;
  }

Shanti,
rR

@Riccardo: Done - thanks :)

Please re-review.
-K

@zen: I have found another problem.
Category selection doesn't show up free tagging vocabularies. Same problem on node advanced search.

The fix is easy (on node.module):

On line 748 :   // Fixes node advanced search 
      if ($taxonomy = module_invoke('taxonomy', 'form_all', $free_tags = 1)) {


Line 980 : // Fixes node filtering
  if ($taxonomy = module_invoke('taxonomy', 'form_all', $free_tags = 1)) {

rR

+    //The %%d is to accomodate multiple INNER JOINs whilst preserving the %d for later use.
+    'where' => 'tn%d.tid = %%d',

is a hack IMO! :/

@Riccardo: Another good catch :) I'll add a patch shortly.. btw you don't have to wait for me to do this.. Feel free to attach your own, or even open a new issue for it :)

@Dries: Well, this was the _neatest_ solution I could come up with. It was either this or appending a %d to the string after the first sprintf. Any ideas?

Thanks for the reviews :)
-K

A new patch with all modification made by Zen, plus :

- the changes proposed on #11 to fix free tagging

- a different approach to table aliases using a token ('tn#') and str_replace() to append a counter to tn alias. Is this acceptable?

Please review.
rR

OK guys, I fix this and then go to sleep.

riccardoR, putting $foo =1 into a function call parameter is not really elegant, and mostly totally unnecessary. If you need to assign $foo 1 , do it before the call, and use $foo as parameter. If you do not need to do that then simply use 1 as parameter. drupaldocs shows the definition headers not the call.

folks, thanks for the hard work. All I needed to do was some minor code cleanup. node_filters stored SQL -- but it was used only in node_build_filter_query so I simply moved the queries to their place. Then I grabbed the $index which was available from foreach and created the category queries with it. Similar technique can be found in taxonomy_select_nodes. No need for $i and increasing it.

I also found that in node_filters $filters['category']['options'] was given a value twice. This let me delete some code.

Another minor removal, this time from an earlier attempt of mine :)

And finally, some comments.

i teste and code reviewed the most recent patch. code looks real nice and works as promised. nice work everyone.

+1 Excellent work chx :)

Cheers
-K

I removed two blank lines and $i = 0 which is unused. No need to retest.

And now, the correct patch :(

Drupal gurus,

Any chance this patch will work on a 4.6 site or is the code more for 4.7? I am using hundreds of terms on a site, so the ability to filter by category is crucial for me as admin.

Also, any chance this will be tagged for 4.6.5? That would be a real bonus.

Is 'exploit hole' proper English? I sounds so ... nevermind.

Patch attached:

-Exploit -> Exploitable
-Simplified node_filter array declaration
-[OT] Removed unused variable ($i) from node_revision_overview

@Ramdak - I have no idea, sorry.

-K

+1
@chx: Your revision is very clean and elegant. I have learned a lot here.

Thanks everyone.
rR

Thanks,

Checked this .patch and it seams to do all the tricks with even with many categories and many AND ... AND .. levels plus free tagging is now working as well.. this is realy good one.

T.

Hello Folks,

Now, when filtering is working fine and flawless.. I think that there is still a small improvement needed.. when building a category dropdown list then probably it would be a good idea to exclude from there non used categories... I mean if there is no single node in that category, then it's wise not to show that category at all in category filter dropdown menu ..

This is just to add as comment here before this .patch will be rolle out into the HEAD..

T.

Fixed //.

-K

I leave this for Steven to review and commit; he wrote the original code and is best placed to review this.
The $table/$index code looks a bit weird to me.

Fixes missed //. Patch directly edited.

-K

I committed the patch with improved comments, but the free tagging issue IMO needs to be addressed.

Free tagging vocabularies tend to be very large and would make the page very long to load. The advanced search has a similar problem though. The only thing that springs to mind is to use an autocomplete-based mechanism for large/free tagged vocabularies.