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.

Files: 
CommentFileSizeAuthor
#19 1586428-er-term-index-19.patch14.69 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 114 pass(es).
[ View ]
#17 1586428-er-term-index-17.patch14.64 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 114 pass(es).
[ View ]
#11 0001-Issue-1586428-by-fgm-amitaibu-Integrate-entyreferenc.patch15.59 KBfgm
FAILED: [[SimpleTest]]: [MySQL] 72 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#10 1586428-er-taxonomy-index-10.patch13.13 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
#7 1586428-er-taxonomy-index-7.patch8.52 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
#6 0001-Issue-1586428-by-fgm-Integrate-entyreference-from-no.patch9.71 KBfgm
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]
#3 0001-Issue-1586428-add-taxonomy_index-integration.patch10.15 KBfgm
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]
#2 0001-Issue-1586428-add-taxonomy_index-integration.patch10.8 KBfgm
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]
#1 ertaxonomy.tar_.gz2.48 KBfgm

Comments

StatusFileSize
new2.48 KB

Status:Active» Needs review
StatusFileSize
new10.8 KB
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]

Rerolled as patch.

StatusFileSize
new10.15 KB
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]

Grrrr spacing and git diff.... rerolled.

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.

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

Status:Needs work» Needs review
StatusFileSize
new9.71 KB
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]

Rerolled to account for #4 and #5.

StatusFileSize
new8.52 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]

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?

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 :-)

Assigned:Unassigned» Amitaibu

Assigning to myself.

StatusFileSize
new13.13 KB
PASSED: [[SimpleTest]]: [MySQL] 73 pass(es).
[ View ]
  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

StatusFileSize
new15.59 KB
FAILED: [[SimpleTest]]: [MySQL] 72 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

@fgm, seems your patch is failing?

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.

Issue tags:+Release blocker

Adding tags

I'll take another stab at this.

Status:Needs work» Needs review
StatusFileSize
new14.64 KB
PASSED: [[SimpleTest]]: [MySQL] 114 pass(es).
[ View ]

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

<?php
// 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()

<?php
+ * 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.

StatusFileSize
new14.69 KB
PASSED: [[SimpleTest]]: [MySQL] 114 pass(es).
[ View ]

Right, Added to Access:

<?php
   
if ($field['storage']['type'] !== 'field_sql_storage') {
     
// Field doesn't use SQL storage.
     
return;
    }
?>

Status:Needs review» Fixed

Committed, thanks!

Great, thanks for finishing it.

Status:Fixed» Closed (fixed)

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

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

Specific to my installation or?

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

Status:Closed (fixed)» Needs work

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

debugging....

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.

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.

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