Provide administration form to execute reindexing batch

Bump. I would appreciate that too

I created a basic UI to make a batch call to taxonomy_entity_index_reindex_entity_type. Since I'm new to Drupal, I don't know much about the Entity API and I don't have much time I left the part where it comes to retrieving a list of all entity types aside and just implemented it for node.
However, the basic structure is designed in a way that shouldn't make it too hard to add this additional functionality.

I just recognized that there was a dpm() call left in the submitted patch + a form validation issue.
So here's a patch for the patch.

You'll want to upload a single combined patch so it's easier to review.

Looks like a good, well-factored solution.

Your code seems to works for me. Thank you!

Push.
I put everything into a patch. Hoping for a commit soon...

patch works for me.

Attached patch removes a stray print_r() and cleans up the code a bit according to Drupal coding standards. Please mark "reviewed and tested by community" if the patch works properly.

Oops, missed a couple @see.

#11 works for me! Thanks!

+++ b/taxonomy_entity_index.moduleundefined
@@ -1,6 +1,30 @@
 /**
+ * Implements hook_menu().
+ */
+function taxonomy_entity_index_menu() {
+  $items = array();
+  $items['admin/config/system/taxonomy_entity_index'] = array(
+    'title' => 'Taxonomy entity index',
+    'description' => 'Manage the Taxonomy entity index.',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('taxonomy_entity_index_simple_form'),
+    'access callback' => TRUE,
+  );
+  return $items;

Please put the menu callbacks in a taxonomy_entity_index.admin.inc file so they are not included in the main .module file.

+++ b/taxonomy_entity_index.moduleundefined
@@ -1,6 +1,30 @@
+/**
+ * Implements hook_admin_paths().
+ */
+function taxonomy_entity_index_admin_paths() {
+  $paths = array(
+    'admin/config/system/taxonomy_entity_index' => TRUE,
+  );

Anything under admin/* should already be defined as an admin path - this is redundant.

+++ b/taxonomy_entity_index.moduleundefined
@@ -203,3 +227,92 @@ function taxonomy_entity_index_reindex_entity_type($entity_type, &$context) {
+/**
+ * Validation callback for the batch reindexing form.
+ *
+ * @see taxonomy_entity_index_simple_form()
+ */
+function taxonomy_entity_index_simple_form_validate($form, &$form_state) {
+  if (!array_filter($form_state['values']['types'])) {
+    form_set_error('', t('You must select at least one entity type.'));
+  }

We can just add #required => TRUE to $form['types'] rather than a validation function right?

Also, let's name the form 'taxonomy_entity_index_reindex_form' or 'taxonomy_entity_index_batch_form' - the world 'simple' doesn't have any context for me here.

I think there are some bugs with the actual reindexing batch operation. I could remove, and repost these in a separate issue if you want. Possibly they could even be core bugs with EFQ, but more likely something is wrong with the logic of how these things are getting set.

Here's what I got for terms:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /d7-sfentity2/batch?id=23&op=do StatusText: OK ResponseText: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'vocabulary_machine_name' in 'where clause': SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM {taxonomy_term_data} taxonomy_term_data WHERE (tid > :db_condition_placeholder_0) AND (vocabulary_machine_name IN (:db_condition_placeholder_1, :db_condition_placeholder_2)) ) subquery; Array ( [:db_condition_placeholder_0] => 0 [:db_condition_placeholder_1] => accounts [:db_condition_placeholder_2] => service_group ) in EntityFieldQuery->execute() (line 1011 of /Applications/MAMP/htdocs/d7-sfentity2/includes/entity.inc)

From this debug output it looks like EFQ is presuming that vocabulary_machine_name is a field directly on {taxonomy_term_data}. However, the actual field is just called vid, and machine_name is a field on {taxonomy_vocabulary}.

And here's what I got for users:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /d7-sfentity2/batch?id=24&op=do StatusText: OK ResponseText: PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'bundle' in 'having clause': SELECT COUNT(*) AS expression FROM (SELECT 1 AS expression FROM {users} users WHERE (uid > :db_condition_placeholder_0) HAVING (bundle IN (:db_condition_placeholder_1)) ) subquery; Array ( [:db_condition_placeholder_0] => 0 [:db_condition_placeholder_1] => user ) in EntityFieldQuery->execute() (line 1011 of /Applications/MAMP/htdocs/d7-sfentity2/includes/entity.inc).

I think this second one is because users only have one bundle, so the $query->entityCondition of 'bundle' should be left out for them.

Nodes worked fine.

The following code, based on #1054162: Taxonomy bundles not supported by EntityFieldQuery (followup), with the addition of unsetting the bundle for users, when added to the module, solved this issue:

/**
 * Implements hook_entity_query_alter().
 * Adds bundle support for taxonomy terms, and removes bundles from a user query.
 */
function taxonomy_entity_index_entity_query_alter($query) {
  $conditions = &$query->entityConditions;

  // Alter taxonomy term queries to use vid, not vocabulary_machine_name.
  if (isset($conditions['entity_type']) && $conditions['entity_type']['value'] == 'taxonomy_term' && isset($conditions['bundle'])) {
    if (in_array($conditions['bundle']['operator'], array(NULL, '=', '!=', 'IN', 'NOT IN'))) {
      $vids = array();
      if (is_array($conditions['bundle']['value'])) {
        foreach ($conditions['bundle']['value'] as $vocabulary_machine_name) {
          $vocabulary = taxonomy_vocabulary_machine_name_load($vocabulary_machine_name);
          $vids[] = $vocabulary->vid;
        }
      }
      else {
        $vocabulary = taxonomy_vocabulary_machine_name_load($conditions['bundle']['value']);
        $vids = $vocabulary->vid;
      }

      $query->propertyCondition('vid', $vids, $conditions['bundle']['operator']);
      unset($conditions['bundle']);
    }
  }
  // Alter user queries to not have a bundle.
  if (isset($conditions['entity_type']) && $conditions['entity_type']['value'] == 'user' && isset($conditions['bundle'])) {
    unset($conditions['bundle']);
  }
}

The fact that this was necessary, however, suggests that Drupal core has some deficiencies (bugs?) in the way that it handles bundles on entities other than nodes.

Do you think something like this should be added to the patch, or is it a separate issue? If the latter, how should it be handled?

@Dave Reid:

I am currently using a hacked version of this module with the patch from #15 + the logic from #18.

In light of #1054162: Taxonomy bundles not supported by EntityFieldQuery (followup) likely not getting committed in the next D7 release, do you think the hook_entity_query_alter() implementation should be in this module, or should I move it to my custom module which depends on this module?

I want to have the most maintainable solution moving forward.

Sounds also like the changes in #15 are necessary for this patch to go in. I don't know if I'll have time to get to that soon, but sounds like they're fairly simple.

@EvanDonovan: Sounds to me that maybe a separate bug report should be opened for #18? Since this issue is a feature request.

So, in addition to the recommendations in #15 i did another (little) changes.
@Dave Reid The patch is against the (git) dev 7.x-1.x, ...dont't know why it doesn't appear among the options.

Update status

I notice a couple of minor things with the patch, just some trailing whitespace and a missing newline at the end. I'll reroll #22.

Okay, reviewing the patch:

+++ b/taxonomy_entity_index.admin.incundefined
@@ -0,0 +1,121 @@
+/**
+ * Batch callback; re-index all the terms for a given entity type.
+ */
+function taxonomy_entity_index_reindex_entity_type($entity_type, &$context) {
...
+/**
+ * Batch 'finished' callback.
+ *
+ * @see taxonomy_entity_index_admin_form()
+ * @see taxonomy_entity_index_reindex_entity_type()
+ */

I am not sure we want to move these two functions out of the main .module file. The batch job might be triggered from other than the admin pages. So probably just the admin form and submit handler should live in the .admin.inc file.

Outside of that, it looks like all the feedback from #15 and #16 has been addressed.

I'll fix that as well I guess.

'access callback' => TRUE,

Why the access always granted??
This looks like a major security issue!
I must say i also overlooked this in #22...

We should implement hook_permission or use one already defined in core (e.g. 'administer site configuration' or something).
Any opinion...?

@neoglez: Since it is under admin I believe it's already restricted. Might want to test to make sure. :)

@xjm: Don't need to, just look in _menu_check_access ;-)

// Check for a TRUE or FALSE value.
  if (is_numeric($callback)) {
    $item['access'] = (bool) $callback;
  }

Right, but since the item is a child of the admin path, it inherits that access restriction. Edit: sorry, to clarify, I am saying that the access callback key should just be removed. There is no need to add one.

Not when the access callback is explicit defined.
edit: Ok, now we're right :)

Here; code speaks louder than words. :) I tested and confirmed that with this patch, non-privileged users do not receive access.

Edit: Hah, I am not used to real-time conversations on issue nodes. ;)

Oops, except the patch itself dropped a hunk. Fixed here.

Wow, that was almost real time! Thanks a lot!
The patch looks fine to me (i also tested).
I also like this approach.

Here http://drupal.org/node/109157 says:

"From Drupal 6.2 on, access callback and access arguments are no longer inherited from parent items"

When applying this patch, no menu appears in the admin interface (Configuration > System) as it thinks is access callback false by default (from _menu_check_access)

 $callback = empty($item['access_callback']) ? 0 : trim($item['access_callback']);
  // Check for a TRUE or FALSE value.
  if (is_numeric($callback)) {
    $item['access'] = (bool) $callback;
(..)

I think we should choose a permission or create one for this case. As it is under system (it could be also under search & metadata imho) we could reuse "Administer site configuration"

Patch attached with this change.

Ah, thanks re: #39. Good catch. Clearly I'm out of touch. ;)

Looks good to me.

From Drupal 6.2 on, access callback and access arguments are no longer inherited from parent items

well, it's not actually like that.
Note, for example:

"access callback": A function returning TRUE if the user has access rights to this menu item, and FALSE if not. It can also be a boolean constant instead of a function, and you can also use numeric values (will be cast to boolean). Defaults to user_access() unless a value is inherited from the parent menu item; only MENU_DEFAULT_LOCAL_TASK items can inherit access callbacks. To use the user_access() default callback, you must specify the permission to check as 'access arguments' (see below).

"access arguments": An array of arguments to pass to the access callback function, with path component substitution as described above. If the access callback is inherited (see above), the access arguments will be inherited with it, unless overridden in the child menu item.

from hook_menu

Yes, but this is a MENU_NORMAL_ITEM (default) and this kind of menus, afaik, no longer inherit access properties from their parents, that's what patch in #39 fixes, if you omit the access properties, the menu is never displayed in the admin page (Configuration > System) in this case.

Hmm, I thought it was displayed when I tested #37. Perhaps I failed to properly clear my menu cache or something. In either case, though, #39 is RTBC.

I installed the patch from #39 but do not see any kind of administrative menu. The patch installed successfully with no warnings and I flushed my caches. Also, I am using the Administrator account and have double-checked that the "Administer site configuration" permission is enabled. Is there something I am missing?

I've just tested it in a fresh install and patch in #39 works fine, but maybe it the url should use dashes instead of underscores.

NR again based on #44 and #45.

Thank you! That worked perfectly.

Hi,

i wrote drush integration during the views code sprint in hamburg. I removed the bundle condition from the rebuild query, because it will fail for taxonomy terms and comments. I think we can solve this issue in a follow up.

Cheers,
Florian

webflo: can you submit that patch in a separate issue, since the patch here is RTBC and just waiting on commit? Thanks!

created an issue for @webflow's drush commands from #48 #1504654: Add Drush command for re-indexing

@xjm please consider this issue here about indexing Views integration Entity indexing

#45 tested, considered RTBC

Committed to 7.x-1.x: 9b0abb3