There are cases when we want to index/reindex a given content entity but that entity is not in an update process, hence is not indexed by search_api_entity_update() hook implementation. Also we don't want to re-save the entity just to get it reindexed. We workaround this by running this code:
$entity = ...
$entity->original = $entity;
search_api_entity_update($entity);
This is bad for several reasons:
search_api_entity_update(), being a hook implementation, is not meant to be called directly, it's a kind of magic function that is called only by the entity save process.$entity->original = $entity; trick is there to avoid an error in search_api_entity_update(), line $old_translations = $entity->original->getTranslationLanguages();. The code assumes that the entity is indexed ONLY as part of entity save process, where the original public property is set.search_api_entity_update() and search_api_entity_insert() are duplicating almost the same code.ContentEntity::indexEntity() that replicates most of the code from search_api_entity_update() but handles also the case when $entity->original is not set.search_api_entity_insert() and search_api_entity_update(), just call the new method.None.
None.
New static method: ContentEntity::indexEntity().
None.
New helper method to index a content entity: ContentEntity::indexEntity($entity).
| Comment | File | Size | Author |
|---|---|---|---|
| #24 | 3106342-24--indexing_helper_method.patch | 5.9 KB | drunken monkey |
Comments
Comment #2
claudiu.cristeaComment #3
claudiu.cristeaPatch.
Comment #4
claudiu.cristeaAn improved version that is enforcing a content entity by properly typing the function/method parameter. Also
static::should be used to call other static methods inside the new method.Comment #5
claudiu.cristeaOoops, the hooks are called for all kind of entities.
Comment #6
claudiu.cristeaOh, the skip check should be reverted.
Comment #7
claudiu.cristeaComment #8
claudiu.cristeaComment #9
claudiu.cristeaComment #10
dimilias commentedI can also vote for this as I have already ran across this. Does this needs tests?
Comment #11
claudiu.cristeaWell, we are only moving the code around. The current implementation has already test coverage.
Comment #12
dimilias commentedIt is a good addition nonetheless. RTBC by me.
Comment #13
claudiu.cristeaThe variable names are confusing. Simplified the code by using
array_diff()instead of iterating.Comment #15
claudiu.cristeaFixes the case when the entity is new.
Comment #17
claudiu.cristeaOops!
Comment #19
claudiu.cristeaComment #20
borisson_This makes the code simpler to reason about, it has more deletions than additions and I don't think we can apply this anywhere else already.
Big fan!
Comment #21
drunken monkeyThanks a lot for this patch, really looks like a great idea. And sorry I didn’t reply any sooner – for some reason, it seems the notification for this issue slipped through the cracks in my Inbox. I only noticed this now, when searching for RTBC issues I might have missed.
I remember that you already pinged me about this issue a few months ago (which I always recommend doing when you don’t get a reaction from me), but seems I didn’t realize at that time that it wasn’t in my stack of unread issues. I’ll take more care in the future – sorry again!
Anyways, unfortunately the patch became outdated in the meantime, the code in question was already moved to
ContentEntityTrackingManager. However, anindexEntity()method still sounds like a good idea there, so why not?Re-rolled the patch – please test/review and I can commit it.
And thanks again!
Comment #22
jeroen_vreuls commentedThe last patch initializes
$original_translationsas follows:Shouldn't this be:
Because when I index an existing entity it now calls
trackItemsInserted()instead oftrackItemsUpdated(). (although my change might break indexing new entities)Comment #23
claudiu.cristea@drunken monkey, thank you for reviving this. Unfortunately I cannot test this on a real project because on our project we're using an old pinned version of Search API, where only #19 applies. And that works as expected. So, I only did a formal review and I see nothing wrong. Still some nits:
As this is new code, we can strict type also the method's return,
voidin this case.Let's strict type the closure parameter and return type.
Comment #24
drunken monkey@ jeroen_vreuls: No, it shouldn’t, checking
$entity->originalis in general correct here.However, you do have a point: when calling
indexEntity()directly, which is what we want to do here,$entity->originalwill always be empty, so the code will always think it’s dealing with a new entity. So, I guess we probably need a public wrapper method that retrieves the original as appropriate, and then a protected method with the actual (current) code.Hm. But how do we “retrieve the original”? In the general case, we’d have to pass the version of the entity that was last tracked – something which we can’t achieve. Off the top of my head, I can’t even think of a way to reliably determine the known translations of the entity, which is really the only thing we need the original for. So if you make changes to the entities translations and want them indexed via this mechanism, you’d have to set
$entity->originalyourself before calling this.However, I think for the more traditional use case of having other changes (not new or removed translations) you want indexed, I’ve now got this fixed. Please see the attached patch revision.
Also, is it only me or is “current” and “original” a bit confusing here? The change is just happening, so “current” doesn’t seem very clear to me.
I now renamed the variables to
$new_translationsand$old_translations. Would you agree that’s better (@everyone)?Finally, I think
indexEntity()is also a misnomer for that method. It won’t index an entity, it will mark it as needing to be indexed (either because it’s new or changed). Only if the search index option “Index items immediately” is enabled will the entity also be indexed right away (or at least in the same page request).So, I also renamed the method, though I’m not sure what would actually be a good name for it. In any case, I also corrected/expanded its documentation.
If you really want a method for indexing an entity directly, you should at this point probably create a new issue. For that, no interaction with the tracker, etc., would be necessary – just create an item for each translation of the entity and send them to the server for indexing.
Unfortunately, no – we still want to support Drupal 8 while it is maintained, and that only requires PHP 7.0 – so
voidcan’t be used as a return type hint.You’re right, yes. Also included in the attached patch.
Comment #25
drunken monkeyAny more input on this? This has grown quite complex by now, so would like some feedback before committing.
Comment #26
claudiu.cristeaAs I told in #23, I cannot test on real use case. However....
... this concerns me. The whole idea of this feature is to make it simple to index an entity. Without this feature we already did:
So, we set
$entity->original. Then what's the point to have this feature, except code reusability?Comment #27
drunken monkeyWell, it still works correctly when not doing that, as long as the set of translations didn’t change (none added/removed). From the IS, it seems that’s all you wanted?
Anyways, feel free to propose any solution to this if you can think of one. I can’t, so it would be either this or nothing.
Would you prefer passing
$originalas a separate parameter?Comment #28
drunken monkeyAny more feedback?
Comment #29
claudiu.cristeaUnfortunately, I didn't had time to test and, as I told before, we're using a pinned SAPI version which only works with the initial patch. But it seems that #27 has already clarified my concerns.
Comment #31
drunken monkeyOK, thanks for your feedback. Then let’s just commit this, I guess. It’s definitely an improvement, even if it can’t provide all of the functionality we’d like to see.
Committed. Thanks again, everyone!