Currently, term references from nodes created using entityreference are not added to {taxonomy_index}, causing some core and default Views term-based displays not to display these nodes.

I suggests we add this ability. Much as the core mechanism, it will be incomplete and leave the taxonomy_index wrong when such a field is deleted, but it should be at feature parity with core.

This needs some entity_level hooks to be proxied by entityreference, not just field hooks. Some background appears in #1050466: The taxonomy index should be maintained in a node hook, not a field hook. Initial proof of concept as a standalone module: I'll turn it into a patch if this is considered useful.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fgm’s picture

FileSize
2.48 KB
fgm’s picture

Status: Active » Needs review
FileSize
10.8 KB

Rerolled as patch.

fgm’s picture

Grrrr spacing and git diff.... rerolled.

Damien Tournoud’s picture

Status: Needs review » Needs work

I'm fine with the principle of this patch. Not that I am a big fan of the one-off taxonomy index, but I understand it is useful for integration with some contributed modules.

I think we can replace entityreference_field_ui_field_edit_form_validate() with an implementation of EntityReference_BehaviorHandler::access($field, $instance). This method has been designed to answer the question: "which behaviors are valid for this particular field instance?".

Needs work for this.

samsouk’s picture

There is a problem with this patch, the EntityReferenceBehavior_TaxonomyIndex::entityDelete() method is not defined but called from entityreference_entity_delete() function.

fgm’s picture

Status: Needs work » Needs review
FileSize
9.71 KB

Rerolled to account for #4 and #5.

amitaibu’s picture

Oh, I wish I've seen this before rolling my own patch! :/

I took a little different route - one that doesn't use a behavior plugin. Patch checks that existing terms (e.g. referenced from a term_reference field) for little db optimization.
Also comes with tests :)

@fgm, what do you think?

fgm’s picture

I think optimizing can be useful, but I think (not sure, though) that since this index is quite brittle anyway, it is best never to assume that a previous situation is correct and regenerate on each change. I have not looked in detail at your optimization, though: maybe it brings the same resilience at a lower cost than mine, which is basically a copy of the one in taxonomy.module ?

Beyond that, I expect DamZ will prefer that this optional mechanism be kept out of the module itself, as the behavior plugin system provides.

Bonus for having provided a test, though :-)

amitaibu’s picture

Assigned: Unassigned » amitaibu

Assigning to myself.

amitaibu’s picture

  1. Patch adds entityPost[Insert|Update|Delete\ methods
  2. Add a behavior that is "force enabled" when referencing a term from a node. Core lets opt out of it using the variable_get('taxonomy_maintain_index_table', TRUE), so I think we should follow that.
  3. Tests are still passing :)

I have not looked in detail at your optimization

Nothing too fancy, I'm just iterating again over the values that taxonomy added and array_diff() so we hit the DB just if needed, and just with new values

fgm’s picture

Unifying the 3 entity hooks and declaring them in the interface is a nice touch indeed. The query optimization is smart, too.

I merged the patches with a few changes. Where not commented, I used your version:
- ::access() is, I think, more consistent and helpful in my version
- ::buildNodeIndex() is probably better protected than public, as we don't want to expose it
- ::buildNodeIndex() might (especially if it is public) be invoked from a non-ER entity hook, so better keep the test on field['module']
- ::settingsForm() was not present in your patch but can be helpful for the user interface, so I readded it
- I think we do not want this mechanism to be force_enabled, but if the admin sets the config variable to false, the plugin will still be enabled. Not sure how to get out of this one, so I left it in for now
- did some small naming changes to align with the rest of ER.

Note that one of the tests does not pass on my machine with D7 head, whether with you original patch or with my merged version. The test for two references to the same term returns (0 => 0, 0 => 1) instead of just (0 => 1).

Status: Needs review » Needs work

The last submitted patch, 0001-Issue-1586428-by-fgm-amitaibu-Integrate-entyreferenc.patch, failed testing.

amitaibu’s picture

@fgm, seems your patch is failing?

fgm’s picture

Ah yes, I noticed it in Munich and had no time to complete it yet. Actually, if you look at the test, I seem to remember that there is an ambiguity as to the expected result in that situation, which needs to be resolved: one could expect either a result or another, depending on the point of view, IIRC. I'll try to look at it tomorrow.

amitaibu’s picture

Issue tags: +Release blocker

Adding tags

amitaibu’s picture

I'll take another stab at this.

amitaibu’s picture

Status: Needs work » Needs review
FileSize
14.64 KB

1) Test should be passing, in previous patch we were missing the following code which created the duplicate results.

// Insert index entries for all the node's terms, that were not
// already inserted in taxonomy_build_node_index().
$tid_all = array_diff($tid_all, $original_tid_all);

2) I've removed the drupal_set_message() from EntityReferenceBehavior_TaxonomyIndex::access() as IMO it's an API function that shouldn't return messages, and anyway we have better info given in EntityReferenceBehavior_TaxonomyIndex::settingsForm()

Damien Tournoud’s picture

+ * TODO: hardcoded to SQL field storage. Remove that dependency.

The {taxonomy_index} table is specific to SQL field storage, so this comment is inappropriate. Moreover, we actually need to check that the field is using SQL storage in the access method.

amitaibu’s picture

Right, Added to Access:

    if ($field['storage']['type'] !== 'field_sql_storage') {
      // Field doesn't use SQL storage.
      return;
    }
amitaibu’s picture

Status: Needs review » Fixed

Committed, thanks!

fgm’s picture

Great, thanks for finishing it.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

rp7’s picture

This still doesn't work on my installation (installed EntityReference 7.x-1.0).

Specific to my installation or?

nbchip’s picture

It should work for newly created fields. For me , after update to 1.0, still didnt work on already existing fields.

skyredwang’s picture

Status: Closed (fixed) » Needs work

For newly created fields, it's still not working (even after upgrading to dev-Feb-01).

debugging....

skyredwang’s picture

Status: Needs work » Fixed

After a complete uninstall and manual clean up, then re-install, it finally worked. I guess this is no upgrade path.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

robmalon’s picture

I was using an older version of this module when I started developing the project I'm just wrapping up today and discovered this issue. I too tried updating to the latest release of dev (which as mentioned above works for any new fields). Well, I already had data entered, so recreating the fields would have been a pain. I decided to manually update the field_config_instance (table in the DB) for the fields that this affected. In case anyone else is interested, here is what I used to do this:

1. Find your fields machine name/bundle in field_config_instance.

If you have more than a few, you can run a query like this:

SELECT * FROM field_config c
LEFT JOIN field_config_instance ci ON c.id = ci.field_id
WHERE c.type = 'entityreference'
AND ci.data NOT LIKE '%taxonomy_index%'

If this query comes up with nothing then you don't have this problem on any of your fields...

2. Take the serialized data one at a time from the results above and paste it into the simple PHP script below (replacing my example $old_config_instance variable)

$old_config_instance = 'a:8:{s:5:"label";s:4:"My Label";s:6:"widget";a:5:{s:6:"weight";s:1:"2";s:4:"type";s:14:"options_select";s:6:"module";s:7:"options";s:6:"active";i:1;s:8:"settings";a:2:{s:8:"editable";i:0;s:12:"apply_chosen";i:0;}}s:8:"settings";a:1:{s:18:"user_register_form";b:0;}s:7:"display";a:1:{s:7:"default";a:4:{s:5:"label";s:6:"hidden";s:4:"type";s:6:"hidden";s:6:"weight";s:1:"4";s:8:"settings";a:0:{}}}s:8:"required";i:1;s:11:"description";s:168:"Description text";s:24:"ds_extras_field_template";s:0:"";s:13:"default_value";N;}';

echo '<strong>Before</strong><pre>';
print_r(unserialize($old_config_instance));
$new_array = unserialize($old_config_instance);
echo '</pre>';

$new_array['settings']['behaviors']['taxonomy-index']['status'] = 1;
$new_config_instance = serialize($new_array);

echo '<strong>After</strong><pre>';
print_r(unserialize($new_config_instance));
echo '</pre>';

echo '<strong>Result</strong><pre>';
print_r($new_config_instance);
echo '</pre>';

3. Replace the current data for that field with the result.

So, now all that is left is resaving your node's to make sure it's working. But, I bet you have a whole bunch of nodes you don't want to individually save. To get around that you can setup VBO: https://drupal.org/project/views_bulk_operations. I also have https://drupal.org/project/admin_views setup with a custom "All Taxonomy Terms" field added to the new /admin/content view. This way I can easily see which nodes still need to be updated. To bulk update execute -anything- on the nodes that need a refresh. For example, if you're not using the "promote to front" feature you can choose "demote from front page" with no consequences... The only down side is this will change your "last updated" timestamp, though so would going to each node individually.

You might have to adjust the code above for your use case. Make sure you experiment in dev/staging first before putting the change into a production site.

fgm’s picture

@robmalon: think you could turn this into a hook_update_N patch ?

Mixologic’s picture

Issue summary: View changes
Status: Closed (fixed) » Needs work

I hate to needs work an old issue, but I was looking at my taxonomy_index table, and if you have more than one taxonomy entity reference on a piece of content, it will run the db_insert N times. So right now my index table is 5 times the size it should be because I have five taxonomy entity references on my content types..

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: 1586428-er-term-index-19.patch, failed testing.

Erik.Johansson’s picture

A update hook implementation of @robmalon solution would be great.

sinasalek’s picture

I have the same problem, updating entity reference field does not update taxonomy index table

daxelrod’s picture

Patch which fixes duplicate entries in {taxonomy_index}. If multiple entityreferences exist on a node, the buildNodeIndex() function is called for each one. This patch modifies buildNodeIndex() to insert references for each field individually.

daxelrod’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 35: entityreference-taxonomy-index-update-1586428-35.patch, failed testing.

daxelrod’s picture

Re-roll patch to correct directory.

daxelrod’s picture

Status: Needs work » Needs review
LittleRedHen’s picture

Not a proper solution, I know, but in case anyone needs a quick-and-dirty workaround for this issue, the attached patch for the core taxonomy module makes it grab taxonomy_terms in entityreference fields at the same time it checks for plain taxonomy fields. This was enough to make the 'taxonomy terms with depth' views filters work for me on D7.53

daxelrod’s picture

Status: Needs review » Closed (duplicate)
Parent issue: » #1924444: Duplicate entries with taxonomy_index index.

I believe this is resolved in a now closed issue, #1924444: Duplicate entries with taxonomy_index index..

moonray’s picture

Status: Closed (duplicate) » Needs review

This was not fixed by the ticket referenced in comment #41.

Chris Matthews’s picture

Assigned: amitaibu » Unassigned

Unassigned @amitaibu