Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Since upgrading to 2.7, our log is now full of this.
Happens everytime you use search. I have tried looking for the cause but am not getting anywhere so let me know what/where to look.
Cheers.
Comments
Comment #1
jsacksick CreditAttribution: jsacksick commentedIs your search index overriden ? You should go there admin/config/search/search_api/index/product_display/status and check if there's a revert tab.
Comment #2
alexp999 CreditAttribution: alexp999 commentedIt is, as in its default form it struggles to find any of our products.
I tried checking and re-saving all the options but it made no difference.
It will be a real pain if we have to revert...
Comment #3
jsacksick CreditAttribution: jsacksick commentedIt seems to be a Search API bug, something happened between the 1.4 and the 1.5, I'm trying to find the exact cause.
Comment #4
jsacksick CreditAttribution: jsacksick commentedI did a git bisect and it was working until this commit http://drupalcode.org/project/search_api.git/commitdiff/99d20f73957d5803...
Comment #5
jsacksick CreditAttribution: jsacksick commentedI know understand why, the cache of the fields has been set but it doesn't contain all the fields ... The cache contains 33 fields while I have 90 fields indexed...
Comment #6
jsacksick CreditAttribution: jsacksick commentedI think the cache has to be cleared more often, reacting on index_update is not enough IMO.
Comment #7
drunken monkeyWhen should it react, then, in your opinion? What is/could be happening that changes the fields but doesn't trigger that hook?
Comment #8
jsacksick CreditAttribution: jsacksick commentedWhat we could do instead is check if the cache is complete by testing it against the
$index->options['fields']
array, so if the cache doesn't contain the same amount of fields than in this array, then set it again.Comment #10
jsacksick CreditAttribution: jsacksick commentedComment #11
jsacksick CreditAttribution: jsacksick commentedI tested the patch from #10 and there was a last remaining bug in Kickstart, right after the installation, it's now fixed with the attached patch.
Comment #12
jsacksick CreditAttribution: jsacksick commentedHm... It works but the patch is incomplete because it doesn't handle all the usecases, this will only work for the getFields() only indexed call, before I continue working on this, what's your opinion on that Thomas ?
Comment #13
jsacksick CreditAttribution: jsacksick commentedThe attached patch partially reverts the commit mentionned, it removes the cache and it now works again as it used to at least inside Kickstart.
Comment #14
jsacksick CreditAttribution: jsacksick commentedBetter with the correct patch :)
Comment #15
drunken monkeyDefinitely not, sorry. For sites with many content types / fields and maybe some nested structures, each individual
getFields()
call was measured to take up to 5% of total page execution time. For static information, this is unacceptable, even if it will be faster for sites with a less complex data structure.I'd guess the problem during installation would be that some of the fields that are set in the index don't exist yet in the entity data structure. Thus, when the first
getFields()
is called, some of the index's fields are sorted out, and this reduced result is then what gets cached.Possible fixes:
- Don't enable/create the index until the underlying data structure is complete.
- Check whether some fields were left out and don't cache the result if that was the case. (Probably also log a watchdog warning, since this would be bad if happening on a normally running site.)
While the second option seems smarter at first, I think the first one would actually be preferable, or maybe using both. Having an index with fields that don't exist (yet) is generally not a good idea. E.g., #1694832-7: React properly to Field API field changes might then re-save the index with the reduced set of fields.
So, if it's possible to re-arrange things so that this situation doesn't arise, I think it would be a good idea to do that.
Comment #16
jsacksick CreditAttribution: jsacksick commentedOk, I used the debugger this time :p and the issue seems to be caused by the Search API ranges data alteration callback which is calling the
getFields()
method of the index inpropertyInfo()
, the wrong data were cached because of this. I'm going to send a patch in the Search API ranges issue queue for that.See #2001846: Rewrite the data alteration callback.
Comment #17
drunken monkeyAnd, does that patch there solve the problem? If so, then this issue here might be obsolete?
Or would you still like to get a patch in to help prevent such complications also in other cases?
Comment #18
jsacksick CreditAttribution: jsacksick commentedIt solves the issue, it seems that calling
getFields()
inside propertyInfo is not a good idea.I also noticed that the
propertyInfo
method of a data alteration callback is only called when the status the data alteration changes (When you enable/disable it).The patch attached here (http://drupal.org/node/2001846#comment-7444278) has a workaround for that in
configurationFormValidate()
Comment #19
drunken monkeyNot when you change the settings? This would be bad, and definitely a bug we need to fix.
Otherwise, we should perhaps still note this in the doc comments.
Comment #20
jsacksick CreditAttribution: jsacksick commentedThe offending line is in
search_api_admin_index_workflow_submit()
,Comment #21
drunken monkeyI'm pretty sure the methods will also always be called (on the enabled data alterations) when re-creating the
getFields()
cache. The line in your comment is only for automatically indexing all fields that were added by a data alteration that was just enabled.Comment #22
jsacksick CreditAttribution: jsacksick commentedI know but don't you think it makes sense to automatically index fields you specifically added?
Comment #23
drunken monkeyI think I've lost you. Of course I think that makes sense, that's why that code is there. What do you mean? What is the issue here now?
(Except that we should perhaps document that you shouldn't call
$index->getFields()
ingetPropertyInfo()
.)Comment #24
jsacksick CreditAttribution: jsacksick commentedI'm just saying that if you have a data alteration already enabled (Search API Aggregation or Search API ranges for instance) and you add new fields, these are not going to be indexed right away.
Comment #25
drunken monkeyOK, yes, that's right. It's somewhat harder to implement that, and also less common than just for enabling data alterations.
Also, this is clearly a different issue. If you want it added, you are welcome to create a feature request and supply a patch.
Regarding the issue at hand, I'd suggest updating the documentation for data alterations – see the attached patch. I took the chance to also adapt the documentation in that file to the Drupal documentation standards, but the important change is the addition of the following paragraph to the
propertyInfo()
documentation:Do you think this is clear and helpful? (And do the comments maybe still violate some Drupal standard?)
Comment #27
drunken monkey#25: 1999858-25--data_alteration_doc_comments.patch queued for re-testing.
Comment #28
drunken monkeyCommitted.