taxonomy_select_nodes() hard codes limit

Agreed, I'm in the same situation... There should be a smart solution, but I haven't found it yet. :]

Here's a simple patch which compatbile with the 4.6.3's taxonomy.module. It works for me, compatbile with the older one.

Bests,
Aries
http://aries.mindworks.hu

The code style is terrible. Otherwise it looks good.

Another implementation. Goba suggested that I should use an extra parameter to handle the limit. Ok.

Sorry, the second patch is identical with the first one. Here comes the good one.

there are other listing pages like home page, tracker, feeds, etc. do they all need to use this limit scheme?

I don't know, but it was important for me when I implemented some custom node lister.

Marked http://drupal.org/node/31765 as a duplicate of this issue.

This is more a feature than a bug.

I'm moving this to 6.x-dev as (to my understanding) that's where the feature requests should live?

The reason I've come across this is because I need to use the functionality this patch offers. I have created a navigation system for an intranet that allows you to drill down into a vocabulary. Only seeing links to the next level of the hierarchy as well as nodes within that term. The reason I use this function is because I am appending the count i.e. '(2)' of all nodes within a given term and its child hierarchy. For example.

drill down: My Category (4) Another (22)

I was really perplexed as to why my count only ever said a maximum of '(10)' no matter how many nodes were contained within the sub-hierarchy. Turns out it was the limit on feed items! IMHO a completely irrelevant variable.

There needs to be some way to specify an 'all' count so that all nodes within a hierarchy are returned.

Thanks
Adam

P.S.

Add the nodeorder project to the list of projects that would make use of an additional parameter to taxonomy_select_nodes() that limits the number of returned nodes. For now we're either stuck with the 'feed_default_items' setting or copying the entire function.

A good patch should be able to apply 4.7 and 5.x shouldn't it? Does this really have to wait 'til 6.x?

Attached is yet another patch that does just about the same thing. This one is backwards compatible, and it is for 4.7.4. If you don't pass in the $count parameter, everything runs just the way it does now. Here's how it works (from the parameter list in the function's comment block):

 * @param $count
 *   If $pager is TRUE, the number of nodes per page, or -1 to use the
 *   backward-compatible 'default_nodes_main' variable setting.  If $pager
 *   is FALSE, the total number of nodes to select; or -1 to use the
 *   backward-compatible 'feed_default_items' variable setting; or 0 to
 *   select all nodes.

Any chance we can get this committed to the 4.7 branch? If so, I'd be glad to work on the 5.x patch as well...

Attached is the patch re-rolled for HEAD (Drupal 6). Probably doesn't make sense at this point to put the patch in any version prior to 6, but I switched the category to "bug report" from "feature request" because I suspect there are quite a few contributed modules out there that will get bitten by http://drupal.org/node/114774#taxonomy-revisions and whose jobs will be made more difficult because they have cut-and-pasted their own copies of taxonomy_select_nodes(). I apologize in advance for setting this to "bug report"...

Marc

This still applies with a big offset. But probably for drupal 7 now. Moving back to feature as well, the upgrade docs are there for just that purpose.

Please re-roll.

Robin

subscribing as I have to look for this patch every time I do an upgrade or setup a new site.

+1 for finally getting this bug fixed -- this has been asked for over the last three versions of Drupal.

#296632: taxonomy_select_nodes() uses 'feed_default_items' system variable when pager is FALSE
#389598: taxonomy_select_nodes() limits results based on "Number of posts on main page" variable.

Were both duplicates.

Another duplicate #21882: Add $pager_alt variable to taxonomy_select_nodes

Another attempt at a patch for this issue. Against a fresh checkout of D7 HEAD.

Very definitely needs review -- this is my first encounter with the Drupal 7 DB abstraction, and I don't have a D7 site set up to test with yet.

The $limit argument defaults to NULL, which retains the existing behaviour of using the 'default_nodes_main' variable value.
Negative integer values indicate no limit. I thought about using FALSE for this, but -1 is often used as a substitute for 'unlimited', and it meant one less check to do.

Trying again using cvs diff.

Right, I've tested this on paged and unpaged queries in D7. I made a mistake in the comments, though, so am attaching a fixed version.

And again, for yet another stupid comments error...

Sure, some other defaults would be better, but new default behaviour would necessitate changes elsewhere in Drupal where this function is called, which is what I explicitly want to avoid.

This issue has been around since Drupal 4.6.1 (at least), with many duplicates raised over the ensuing years, and it has yet to be fixed.

Because of this, I want this patch to have absolutely no effect on any existing code, yet make it possible for people to use this function as an API-based way of obtaining nodes with a given term assigned, which at present they can't.

Hence: $limit defaults to NULL which means the function uses 'feed_default_items' as normal. But module authors can set $limit if they wish to. And despite it being unpleasant to have to pass that $order array in order to pass a $limit, I've left them in that order to ensure the current behaviour is maintained when there is no $limit specified.

I'm changing this back to 'needs review'. Feel free to revert that if you still disagree.

At a minimum, taxonomy_term_page() needs to be changed to explicitly pass the default nodes main variable to this - otherwise we'll be paging based on the feed default, rather than the nodes per page default.

Ah hell. Thank you. I completely messed that patch up by not noticing that paged and unpaged used two completely different variable names for the limit! That explains how I ended up with that mis-match in the header comments, though.

Ahh, much better. We might want to look at this a bit more later, but this suddenly becomes a much more useful function than before and it's a very small change. Please wait for test bot before commit.

I don't like how we have multiple defaults hard-coded in the function. These should be passed in by the caller, IMO.

I guess that's approval to do this properly, then. Sounds good to me.In core, taxonomy_select_nodes() is only called in two places, both within taxonomy.pages.inc: taxonomy_term_feed() and taxonomy_term_page().

There's a bug (maybe two) in the existing taxonomy_term_feed() -- it thinks that the first argument is still an array (and I also suspect that the second array item is supposed to be the pager argument). I think taxonomy_select_nodes() should support multi-term arguments for it to be a good API function. Should we try to get that functionality back at the same time as dealing with limits?

The multi-term issue aside, for the original problem I would propose that we change the current D7 prototype to:
taxonomy_select_nodes($term, $pager = TRUE, $limit = -1, $order = array('t.sticky' => 'DESC', 't.created' => 'DESC'))

(or would people prefer $limit = NULL to indicate no limit?)

Actually, I'm not sure why the change was made from passing term ids to passing a term object, from which only the tid is used. Just passing the tid (or tids, if we move back to multiple terms) seems to make more sense. Patch updated accordingly.

Now that there's no default, it looks like $limit could just default to FALSE?

I like this patch a lot better. I think FALSE makes more sense than -1.

Changed to FALSE.

I've taken out the $limit = (int) $limit; check, and haven't replaced it with anything to ensure that a non-FALSE $limit argument is a positive integer (or a string representation thereof). You do now end up with a SQL syntax error if the $limit argument is invalid. Should invalid inputs be handled, or is it appropriate to let it fail? I'm not sure what the preference is.

Validation when setting the 'default_nodes_main' and 'feed_default_items' variables probably factors into this, and system.admin.inc uses '#options' => drupal_map_assoc(array(1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 15, 20, 25, 30)) for both, so things will generally be fine for the core uses of this function without extra error checking required. I'm happy to add it in, though.

I'm not seeing why that failed testing, when the previous patch was fine. It says one hook menu test failure, but doesn't give more detail than that, so I'm not sure where the supposed failure was. False positive?

Sending for re-test.

Patch looks much better. We don't validate for completely invalid arguments, just let things fail.

Only thing I'd change is ($limit !== FALSE) which should just be if ($limit)

The query is valid when $limit = 0, so we definitely don't want that to return unlimited results in that instance (even if no one should sensibly be requesting that).

That shouldn't hold this up and makes enough sense. I've got a bad feeling we've missed the deadline for API changes. But since the function signature here was changed only 2-3 days ago, this is arguably critical cleanup after the field API conversion - especially since no-one can directly query term_node tables any more, so this function become more important when upgrading. Dries/webchick - please move to D8 if this is considered too late.

Committed to CVS HEAD still. Thanks!

Now that this is fixed in D7, would a patch for Drupal 6 be considered if it does not change the behaviour of any existing calls to the function? (i.e. the new limit argument is last, optional, and the default value maintains the existing behaviour).

subscribing, that would be great.

@jweowu, the best way to find out is to provide such a patch. Gabor is sometimes open to changes like that.

#38: taxonomy_select_nodes_limit_35.patch queued for re-testing.

Please ignore, re-testing. it was mistake

subscribing

"Ensure the patch applies to the lastest checkotu of the code-base."
?

Subscribing

//subscribing.

Subscribing

#38: taxonomy_select_nodes_limit_35.patch queued for re-testing.

#38: taxonomy_select_nodes_limit_35.patch queued for re-testing.

verta: That's a patch for Drupal 7 (which was committed). There's no point in re-testing it now, because it doesn't apply to Drupal 6.

Thanks for clarifying. Unfortunately, we are still using D6. The reason I was following this issue was that we wanted to be able to use the Priorities module. The limit of 15 taxonomy items was a show stopper there.

I come back from time to time to see if this is ever going to be fixed in D6. I don't know how to "roll" a patch, but I can test whether or not a dev release fixes a given issue or not.

subscribing

#24: taxonomy_select_nodes_limit.patch queued for re-testing.

There is nothing to review for D6 here.

My gut feeling is that since this fix requires an API change, and that to make the same change to D6 that happened in D7 would require breaking anything using this function that passes in $order, and that with D8 around the corner D6 is definitely not accepting new functionality at this point, this is probably a "won't fix".