Patch: Fixing fact that certain tax terms are nearly always selected by devel_generate

Works great. Before it would generate content mostly within one term in a vocabulary with a small (~10 terms) set. With the patch, content is generated more evenly over the available terms.

Works great for me too!

This is pretty inefficient. If we have 1000 terms in a vocabulary, then every call needs to randomize the 1000 terms. If we allow 10 terms and we generate 100 nodes, we'll randomize the 1000 terms 500 times.

I wonder how orderRandom() compares to counting the terms (this could be cached) and then using limit(mt_rand(0, $max - 1), 1) on the unsorted (!) terms...

I do think this should be fixed, but orderRandom() is probably not the right way...

the patch gives me "Hunk #1 FAILED at 15."
:(

JFTR
This issue might be a duplicate of #1433412: Taxonomy terms aren't randomised when generating nodes with devel_generate.

Patch applies and works as promised (with 7.x-1.3).

Will try the suggestions by salvis (#3) and report back as soon as I find the time.

Some resources for others who might want to give this a try:
* http://jan.kneschke.de/projects/mysql/order-by-rand/

Agreed that orderRandom() is not the best solution performance-wise, but the way it's done now causes the same terms to be selected.

Also, we shouldn't depend on MySQL tools.

Here's my take on this, we do a count query first, if there are any taxonomy terms, we use mt_rand to select a random row between 0 and count-1, giving us a much better random distribution.

This looks reasonable — should we cache the count (per vocabulary)?

Looks good, also a comment on performance, we might want to replace db_select by db_query.

Yes, but db_query() does not lend itself to rewriting anymore, and because of the missing translation layer it can cause problems for the non-MySQL databases. I'm eliminating all db_query()s from my modules.

Sounds good, here is a new version with static caching of the count by vid.

Looks good — let's see whether it applies to D8...

+++ devel/devel_generate/taxonomy.devel_generate.inc	(working copy)
@@ -15,22 +15,23 @@
+  // Count the available terms for this vocabulary, keeping a static cache to ¶

A trailing space crept in.

+++ devel/devel_generate/taxonomy.devel_generate.inc	(working copy)
@@ -15,22 +15,23 @@
+  if (empty($term_counts[$vocabulary->vid])) {
+    $term_counts[$vocabulary->vid] = (int) $query->countQuery()->execute()->fetchField();
+  }
+  if ($term_counts[$vocabulary->vid] > 0) {

Since you cater to the fact that the count could be 0, you should use !isset() rather than empty().

#13: devel_generate-betterrandomtaxonomyterms-1328380.patch queued for re-testing.

Good point, fixed to use !isset() and removed trailing space, here's a version that applies to 8.x-dev, but I have no drupal 8 environment to test in at the moment.

+++ devel/devel_generate/taxonomy.devel_generate.inc	(working copy)
@@ -15,22 +15,23 @@
+  $term_counts = &drupal_static(__FUNCTION__, array());
+  if (!isset($term_counts[$vocabulary->vid])) {

We're returning an array keyed by 'tid', I guess this is never going to hit cache this way, is it?

It should hit the cache as it's keyed by $vocabulary->vid.

Looks good. Anyone wants to test this?

Ok, tried the patch and the logic works great, had to change several things to make it work with D8 HEAD.

Thanks, pcambra! Are you OK with #21, richard.thomas?

Yep, looks good to me.

No longer applies to current 8.x-1.x-dev.

Tagging for backport.

My goal is fixing this in 7.x, but here is a patch for 8.x. Requires the patch in #2103393: field_behaviors_widget() is removed. Won't pass tests until that patch is committed.

Devel generation has changed to be pluggable in #2154141: DevelGenerate as a D8 plugin type so this needs a re roll.

I think I fixed the bug, but in an inefficient way. Lets just call this fixed and discuss efficiency when needed.