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.
Comments
Comment #1
fgmComment #2
fgmRerolled as patch.
Comment #3
fgmGrrrr spacing and git diff.... rerolled.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'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 ofEntityReference_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.
Comment #5
samsouk CreditAttribution: samsouk commentedThere is a problem with this patch, the EntityReferenceBehavior_TaxonomyIndex::entityDelete() method is not defined but called from entityreference_entity_delete() function.
Comment #6
fgmRerolled to account for #4 and #5.
Comment #7
amitaibuOh, 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?
Comment #8
fgmI 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 :-)
Comment #9
amitaibuAssigning to myself.
Comment #10
amitaibuvariable_get('taxonomy_maintain_index_table', TRUE)
, so I think we should follow that.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
Comment #11
fgmUnifying 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).
Comment #13
amitaibu@fgm, seems your patch is failing?
Comment #14
fgmAh 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.
Comment #15
amitaibuAdding tags
Comment #16
amitaibuI'll take another stab at this.
Comment #17
amitaibu1) Test should be passing, in previous patch we were missing the following code which created the duplicate results.
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()
Comment #18
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe {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.
Comment #19
amitaibuRight, Added to Access:
Comment #20
amitaibuCommitted, thanks!
Comment #21
fgmGreat, thanks for finishing it.
Comment #23
rp7 CreditAttribution: rp7 commentedThis still doesn't work on my installation (installed EntityReference 7.x-1.0).
Specific to my installation or?
Comment #24
nbchip CreditAttribution: nbchip commentedIt should work for newly created fields. For me , after update to 1.0, still didnt work on already existing fields.
Comment #25
skyredwangFor newly created fields, it's still not working (even after upgrading to dev-Feb-01).
debugging....
Comment #26
skyredwangAfter a complete uninstall and manual clean up, then re-install, it finally worked. I guess this is no upgrade path.
Comment #28
robmalon CreditAttribution: robmalon commentedI 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:
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)
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.
Comment #29
fgm@robmalon: think you could turn this into a hook_update_N patch ?
Comment #30
MixologicI 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..
Comment #33
Erik.Johansson CreditAttribution: Erik.Johansson commentedA update hook implementation of @robmalon solution would be great.
Comment #34
sinasalek CreditAttribution: sinasalek commentedI have the same problem, updating entity reference field does not update taxonomy index table
Comment #35
daxelrod CreditAttribution: daxelrod commentedPatch 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.
Comment #36
daxelrod CreditAttribution: daxelrod commentedComment #38
daxelrod CreditAttribution: daxelrod commentedRe-roll patch to correct directory.
Comment #39
daxelrod CreditAttribution: daxelrod commentedComment #40
LittleRedHen CreditAttribution: LittleRedHen commentedNot 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
Comment #41
daxelrod CreditAttribution: daxelrod commentedI believe this is resolved in a now closed issue, #1924444: Duplicate entries with taxonomy_index index..
Comment #42
moonray CreditAttribution: moonray at Chapter Three commentedThis was not fixed by the ticket referenced in comment #41.
Comment #43
Chris Matthews CreditAttribution: Chris Matthews as a volunteer commentedUnassigned @amitaibu