taxonomy_select_nodes() limits results based on "Number of posts on main page" variable.

I agree this needs to be fixed - it's taken me the last 30 minutes to figure out this is why my script wasn't working as I've just gone above 10 nodes in the list...

Ian

Patch to add a new argument, $number_results, to taxonomy_select_nodes(). If it is specified, it will override the default_nodes_main and feed_default_items variables.

This shouldn't affect any existing calls to the variable, since the new argument is at the end of the call, and is NULL by default.

I am copying the documentation for the new argument and the new function declaration below:

/**
 * @param $number_results
 *   The number of results that should be returned per page if $pager is TRUE.
 *   If $pager is FALSE, this is the number of nodes returned.
 *   Defaults to 'default_nodes_main' for $pager being TRUE. Defaults to
 *   feed_default_items if $pager is FALSE.
 */

function taxonomy_select_nodes($tids = array(), $operator = 'or', $depth = 0, $pager = TRUE, $order = array('n.sticky' => 'DESC', 'n.created' =>  'DESC'), $number_results = NULL)

tagging for Developer Experience.

... and forgot to set to D7 and 'needs review'.

Hopefully this is the last I clutter the issue...

This change looks good but since both the internal variable and the SQL are going to use LIMIT I think we should use $limit as the parameter too.

Good call.

That would simplify the code somewhat as well. I'll reroll with the suggested changes tomorrow.

New version with catch's suggestion implemented. Variable is now called $limit.

This also allowed me to eliminate several lines of code from the patch (yay for readability!)

Seems like you're checking and setting $limit twice? Wouldn't it already be set by the second time?

Unfortunately, this is to match the current behaviour of this function:

• When $pager is true, the number of results is equal to default_nodes_main per page (set in Administer->Site Configuration->Site Information->Number of posts on front page)
• When $pager is false, then it is equal to the number of items that are supposed to be shown by feed_default_items (which is set in Administer->Content Management->RSS Publishing->Number of items in each feed)

To that end, when $pager is TRUE, I check and set it to default_nodes_main if $limit is not specified. If $pager is FALSE, I set it to feed_default_items, if $limit is not specified.

An alternative solution is to alter the function so that no default is provided for $limit, and leave it up to the calling function to decide what $limit should be. That would allow us to clear this special casing out of the code, and make it more general purpose. Since this function is only called once in the code, this would be a very unobtrusive fix.

Attached patch now makes it so $limit must be specified when called.

Furthermore, $limit was inserted before $order in the function declaration as I assume people will want to set the number of nodes returned far more often then they will want to change the default sort order.

Finally, the single call to the function in core has been updated to use the new argument.

Setting to needs review - testbot had a problem.

Just had an idea..

What if we remove the limit handling completely from that function and return the $query object instead of the results? I did the same in search.module conversion. Imho, this would have the following advantages:

- We can remove these arguments from that huge function definition
- It allows to further customize that query, for example adding more fields, if you just want to display a list of nodes, you don't need to use node_load on all of them.
- Sort is a bit more complex, but maybe we can remove that too...

If we do that, we may want to change the function name to indicate the change.

It could look like this:

$query = taxonomy_select_nodes($tids, $terms['operator'], $depth)->extend('PagerDefault');
$nids = $query
  ->fields('n', array('sticky', 'created')
  ->orderBy('n.sticky', 'DESC')
  ->orderBy('n.created', 'DESC')
  ->limit(variable_get('default_nodes_main', 10))
  ->execute();

The only implementation in core would still use two queries, once to load the nid's and second, loading those nodes in node_load_multiple(). It would be rather cool if we could do node_load_multiple($query), but that is for sure something for another issue ;)

Oh, and another thing, I noticed that I somehow messed up the apidoc, see http://api.drupal.org/api/function/taxonomy_select_nodes/7. The $sort param example is displayed at the top. But if we remove it, that gets fixed anyway.

Hello, I am experiencing a similar problem where I would like to retrieve ALL nodes associated with a taxonomy term, but am only getting 10.

I am wondering which patch (comment #7 or #10) to use (#10 failed testing), and if there are any plans to commit this to the 6.x release? Just concerned about the long term viability in case the patch gets over-written with the next update.

Thanks!

- Tyler

> and if there are any plans to commit this to the 6.x release?
I can answer you that... No. No Features/API changes are commited to a stable branch.

I agree completely with #14. This function is nothing more then an helper function to build a query. It should build a structured query using db_select().

Attached is a first try of my idea in #14.

Edit: Something is really weird with both (old) function calls, one does have the arguments inside the tid array and the other is using $depth = TRUE. erm :) I "fixed" it, not sure if it working as intended now.

I like this approach better.

On a quick inspection, the code looks good. However, you will need to update the documentation at the top of taxonomy_select_nodes() to reflect the now-missing arguments and changed return value.

Other than that, this should go in!

I knew I forgot something :)

Updated the apidocs, maybe we should even rename the function to indicate the change? I have no idea to what, though...

perhaps taxonomy_get_term_nodes_query()? Or simpler taxonomy_term_nodes_query()?

Test class	                          Pass	Fail	Exception
-----------------------------------------------------------
Taxonomy term functions and forms.	  139	  2	    1

Looks like some tests need to be updated....

I can't reproduce these errors locally, I'm trying with a new patch. Would be great if someone can run the tests locally and tell me which test assertions exactly fail...

If this isn't a candidate for backport, then it might not be necessary at all if #412518: Convert taxonomy_node_* related code to use field API + upgrade path gets in, or at least need a lot of refactoring.

See you in #25801: taxonomy_select_nodes() hard codes limit.

If this isn't a candidate for backport, then it might not be necessary at all if #412518: Convert taxonomy_node_* related code to use field API + upgrade path gets in

The committed code for that issue appears to be:
http://drupal.org/node/412518#comment-2126866

which still contains the same problem in its version of taxonomy_select_nodes()

So regardless of backporting (which would be nice), this seems to be an ongoing problem for Drupal 7.

Come on, this issue if been reported for ages !! (also in http://drupal.org/node/296632). Please fix it!
Now I'm having the same problem which costed me 30 mins before I saw this :(

I justed banged my head on the desk for a few hours trying to figure this one out without hacking the core, and failed to do so. So i just reimplemented this function in my module with the new $per_page parameter :(

...glad to see this one has finally been fixed ;)

$pager variable needs to be set to FALSE (otherwise the function will default to $pager = TRUE which has a default limit of 10)

You may use taxonomy_select_list($tid, $pager = FALSE)