🇦🇹Austria @drunken monkey

greggles → credited drunken monkey → .

Looks good. Merged.

drunken monkey → created an issue.

Seems the fails unders Drupal 10 are in HEAD already, and it otherwise seems to work well, so merged.

drunken monkey → created an issue.

Didn’t work either, so let’s just fix this in the Search API Page module as we should have done from the beginning. See 🐛 Fix deprecation warnings in PHP 8.4 Active .

Success!

OK, that didn’t work either.
Seems like the test environment maybe just doesn’t understand the @dev version specification? Let’s try that …

Seems one potential fix here would be for the server backend plugin to include empty fields in their result items if they tried to retrieve the field, since this means that the item was indexed without a value for that field and explicitly returning the field as empty makes sense.

However, it also makes sense to have a similar setting for the excerpt as for highlighting individual fields, for not loading field values from the database if not already included in the results. So, if you want to provide an MR for that, I’d be open to including this functionality. (Just “respecting” the existing setting would break sites relying on the existing behavior so I don’t think we can do that.)

Thanks for reporting this problem!

As you already saw by the test failure, the Index class uses $this->datasourceInstances as the “source of truth” for its datasources. Keeping this consistent helps avoid a lot of thorny problems when changing index settings (see #2638116: Clean up caching of Index class method results (especially fields) → for background on this decision). For instance, a similar change as the one for removeDatasource() would have to be made to addDatasource() or getDatasourceIds() would return incorrect data when called after adding a datasource.

However, I guess this doesn’t really apply when the datasource plugins aren’t loaded yet, so maybe using either $datasourceInstances or $datasource_settings based on whether the former is initialized would work in all cases. The only “break” in functionality would now be that Index::getDatasourceIds() will never throw an exception, and since that exception wasn’t even documented I guess this is acceptable. As you say, it might even improve performance in rare cases.

Please give the new code in the MR a try!

drunken monkey → made their first commit to this issue’s fork.

Shouldn’t the server be set to null instead of ''? The former means “no server”, the latter looks to most code like a server with ID ''.
In your setup, there also seems to be something else going wrong as an index without server cannot be enabled.

If you try to import invalid configuration it is clear that errors will occur, I wouldn’t say that is a bug in the code.

What exactly was the schema error. The default is search_api.default_processor_configuration, so if you needed to add search_api.fields_processor_configuration then maybe your processor inherits from FieldsProcessorPluginBase? In that case, it is expected that you would need to specify that explicitly, we have no way of detecting in the config schema whether a processor inherits from that.

For those processors that just inherit from ProcessorPluginBase it seems to work fine. For instance, there is no explicit schema for the entity_status processor.

Seems changes to test_dependencies might only take effect after they are merged, not when just inside a fork, based on the docs → . So, in the spirit of experimentation, I just merged the MR. Let’s see what happens.

drunken monkey → created an issue.

Seems the deprecation warnings were actually already fixed in Facets, see 🐛 Deprecations PHP 8.4 Active . We are just not using the latest dev version of the module, and there hasn’t been a stable release that includes that fix yet.
Let’s see, maybe I can get the CI to use the dev version of the module after all.

Thanks a lot for writing the test, looks great!
I just made a few minor changes, now I’d say this is RTBC.

Great to hear, thanks for the feedback!
Merged.
Thanks again, everyone!

Merged.

Running this locally with XDebug enabled makes it immediately clear why PHPUnit fails to finish: The current code actually leads to an infinite loop, as the default method implementation \Drupal\search_api\Backend\BackendPluginBase::setConfiguration() actually calls $this->server->setBackendConfig(). Our current code actually already guarded against this, so that the config stays in sync no matter which method you call but you should still never get an infinite loop.
So, let’s just remove one condition from the if check and add the try/catch and that should fix all that.

Please test/review again!

Sorry if this is completely off-topic for this issue, but I’m unclear why setting a custom ignore file for deprecations via SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=…" doesn’t work to prevent those failures in PHP 8.4 anymore? E.g., in this MR I added such a file for ignoring deprecation warnings triggered by a different Drupal module (since it makes no sense to fail on those), but the “phpunit (max PHP version)” job still fails because of output caused by the deprecation warning.

If there is really currently no way to avoid failing on deprecated code in other contrib modules (except to disable larger swaths of checks such as tests printing output) then I’m very much in favor of adding some mechanism for that. Though I’m not sure whether the proposed changes would actually allow for that or just help you disable failing on output.

drunken monkey → created an issue.

drunken monkey → created an issue.

There are currently no PHPCS failures in this module, see the scheduled pipelines.

Agreed that it’s important to keep this from becoming common, but probably already covered by the linked documentation. Let’s just adapt our tools to spot this.

That sounds completely sensible, thanks a lot for the suggestion and already creating an MR.
Merged.
Thanks again!

Changed my mind, let’s not throw an exception after all. Doesn’t really add any value there, I’d say.

Sure, that seems sensible. Thanks a lot for the suggestion!
I created an MR with your exact code. Feel free to give it a try. I’d also appreciate feedback from anyone else that wants to weigh in, otherwise I’d just merge it in a week or two.

There is a slight API change here in that setBackendConfig() can now throw an exception, but I feel like that shouldn’t really matter in the real world. But I might still want to add a change record for this, just to be on the safe side – not sure yet.

drunken monkey → made their first commit to this issue’s fork.

Thanks for reporting this problem.
Does the patch in ✨ Allow excluding unpublished references from index Active help? If you have the “Entity status” processor enabled the code in that issue’s MR might already resolve this for you.

Added link to the CacheExclude → module.

Indexes on disabled servers should always be disabled themselves, so should not be returned by the loadByProperties() call. (See \Drupal\search_api\Entity\Server::preSave().)
Something seems to have gone wrong when disabling the server, or maybe you imported faulty config. Anyways, just disabling the affected indexed manually should resolve this problem for you. And if you do find out why the indexes weren’t correctly disabled, please file a separate bug report for that problem.

Thanks for reporting this issue. However, it sounds like this may already be covered by one of the existing issues in this queue:

• 🐛 WSOD when rendering a view on a disabled index with Search API cache plugin Needs work
• 🐛 View recalculated with wrong data from cache Needs review
• 🐛 Call to a member function preExecute() on null in SearchApiTimeCache::generateResultsKey() Needs work

The last of those has already be fixed, so please first make sure this bug still occurs with the latest dev version of the module. If so, then please try the patches/MRs from the other two issues and see if either of them resolves the problem for you.
Only if neither works and you are sure this is a separate issue then please change the status back to “Active”. Otherwise, please mark as “Closed (duplicate)”.

Thanks for your help!

Thanks for reporting this issue and sorry for the delay!
However, I’m afraid I cannot reproduce this problem:

$ drush sapi-s
 --------------------- ----------------------- ------------ --------- ------- 
  ID                    Name                    % Complete   Indexed   Total  
 --------------------- ----------------------- ------------ --------- ------- 
  default_index         Default content index   100%         233       233    
 --------------------- ----------------------- ------------ --------- -------
$ drush locale:update
…
$ drush sapi-s
 --------------------- ----------------------- ------------ --------- ------- 
  ID                    Name                    % Complete   Indexed   Total  
 --------------------- ----------------------- ------------ --------- ------- 
  default_index         Default content index   100%         233       233    
 --------------------- ----------------------- ------------ --------- -------

Am I doing something wrong for reproducing? Otherwise it seems some custom code or third-party module is to blame for this issue. You’d have to track calls to the appropriate method(s) on \Drupal\search_api\Entity\Index (probably reindex(), clear() or rebuildTracker()) or the tracker plugin and log the backtrace to figure out where the call is coming from.

Thanks for confirming!
I’m gonna leave this open another week in case anyone else wants to test and provide feedback, and will then merge the MR unless anyone objects.

Merged.

Should be fixed in this MR.

drunken monkey → created an issue.

Oops, overlooked this issue and fixed in 🐛 Fix failing pipelines Active instead.

Please test/review the MR so I can merge it.

@arousseau: Thatnks a lot for your feedback, good to hear this works for you! And also very important to know that this problem even exists anymore in the latest version of the module.

@prudloff: Thanks for that! So does the MR (still) work for you, did you try it?

@v.dovhaliuk: Can you also confirm that the MR works for you?
Just doing admin work on the issue without actually saying that the current patch/MR works for you doesn’t really help me move this along to finally merge it and fix the issue for everyone. As said, for issues that combine Views and caching I’m very reluctant to merge anything since a lot of changes in the past have just led to more issues.

Would anyone else here be able to test/review?

OK, thanks for the further input. Based on this it does seem like a change in the Metatag module is the root cause of this. It seems like field_metatags was previously defined as having a complex data type, with child properties like keywords and description, but now uses a custom data type that is scalar and thus cannot be properly indexed. However, I’m not using the Metatag module myself so I cannot speak to the underlying reasons for those changes and what we can do about them.

Moving this to the Metatag issue queue as it seems pretty likely that any fix for this would have to happen there, but not sure whether it’s clear to anyone that doesn’t have detailed knowledge of both Search API and Metatag internals what the proper fix here would be. Anyways, maybe some Metatag expert can still chime in here and we can figure this out.

Thanks a lot for your work on this, great job so far! However, there are still a couple of problems, as evidenced by the failing tests:

• I think SearchApiViewHooks is missing a services.yml entry.
• (I’d also call that class SearchApiViewsHooks, as the module name is “Views”.)
• I don’t think we can use autowire yet, but I just temporarily enabled testing against other Drupal versions in the fork to validate that assumption.
• Services in the two Hooks classes should be accessed via dependency injection (which is also why autowire would even be needed, I think), not via the global \Drupal class. As a rule, classes that are able to use dependency injection should not contain the string '\Drupal::'.

I already implemented the first two items, but please address the last one as well as the failing MR pipelines.

Thanks for creating an MR, but I don’t think this is actually a bug. The annotation itself is for any search display, while those properties are specific to Views displays. I don’t think it makes sense to add those properties to the general-purpose @SearchApiDisplay annotation.
Is there any actual problem caused by this?

Great to hear, thanks for confirming!
Merged.
Thanks again, everyone!

drunken monkey → created an issue.

As far as I can tell, adding those return type hints was abandoned in the final version of the MR, so nothing for us to do here.

Merged.

Both implemented in the MR.

There actually is already code that should be handling this nested structure:

      while (count($data) === 1) {
        $tmp = reset($data);
        if (!is_array($tmp)) {
          break;
        }
        $data = $tmp;
      }

However, it seems since I wrote this the response structure has slightly changed: an additional metadata property has been added at the top level so the count($data) === 1 check fails right away. The changes in this MR should fix the functionality again while still keeping the code as flexible as possible wrt different keys used in the response.
Please review!

drunken monkey → made their first commit to this issue’s fork.

Done. →

What do you mean with “before”? What exactly did you update (and from/to which version)?
This seems more like a change in the Metatag module, not this one. Did you update Metatag, too, and does reverting that update restore the functionality?

I agree, people should definitely get off those old Drupal versions. And yes, I won’t provide support for those anymore.
But, as said on Slack, just commiting patches as-is (or fixing one typo, admittedly) should still be fine.
Committed. Thanks again!

Seems these were added to the ignored deprecation warnings of Core as part of 🐛 twig/twig in drupal/recommended-settings is at it again Active .
Adapted our custom override of that file in this MR. Let’s see whether the test bot likes it.

drunken monkey → created an issue.

Good to hear, thanks for reporting back.
Not 100% sure how to handle this correctly, but probably we should just search all available fields if no valid ones are selected.
Well, the proper fix would probably be to listen to search index updates and adapt all associated search views accordingly, but that’s a lot of effort for very little actual benefit, I’d say. So, created an MR for the former approach.

@cemckinnie: There is no new processor, this just changes the behavior when using the (existing) “Entity status” processor.

Oh, thanks for pointing that out! Since we generally don’t really check whether the data included in the result row is actually used in the view, and it adds no extra cost to getRowId() (though it might of course lead to too much cache variation, decreasing overall performance), I think we can just include that anyways.

Note that we already had a similar proposal in #3026526: SearchApiTagCache::getRowId is very expensive → but then ended up just using the item ID after all (see comment #5 → ).

I also cleaned up the structure of the $row_data array a bit. Currently, we just added stuff to the top array which seems like it might lead to key collisions, potentially losing important information.

Finally: Is there a reason this is only in the SearchApiTagCache class and not on SearchApiCachePluginTrait to be used by all our cache plugins?
I don’t think this makes sense and corrected this in the MR, but please do correct me if I’m wrong. As I keep repeating I am not that familiar with the Cache API and especially Views’ interaction with it so I pretty much have to rely on others providing well-written code or correcting mine. (Which hasn’t worked great, historically, as we see here.)

Also, I noticed that (unrelatedly) the new SearchApiTagTimeCache plugin added in ✨ Add a third Views cache plugin (tag- and time-based) Needs work doesn’t even use the trait, for some reason. I’m pretty sure this will break lots of things and can only be an oversight, though it’s troubling that our tests don’t pick it up.
I’ve fixed that in the MR, too. As above, please correct me if you think I’m wrong.

Thanks for everyone’s work on this! I hope together we can end up with something that really works in as many cases as possible and doesn’t break any existing functionality.

In any case, we still need to figure out that test fail and add a regression test. (Though I guess the former is kinda optional since it was broken already – could just be a follow-up since it doesn’t seem to lead to problems in real environments.)

Right, that makes sense. Changing issue properties accordingly and moving to the correct queue.

Oh, yes, that is almost definitely related. It seems that the cached results data couldn’t be deserialized for some reason (maybe it got corrupted, potentially due to special characters?) and the Redis cache backend doesn’t correctly detect that case and still returns a cache hit. Then we get a cache hit back, but without any data, leading to this problem.
You should definitely look for an issue in the Redis module’s issue queue → , or create it if there isn’t one already. However, also can’t hurt to guard against this eventuality in our own code. So, glad to hear this helped.

Can someone else confirm that the latest code in the MR works for them? Then I could finally merge this.

OK, thanks, that makes sense. I’ve adapted the issue title and description accordingly.

Will it still be possible (or can we make it possible) for less experienced users to install this directly on their site via the UI? Otherwise it would still seem like a regression, even though it might be an improvement for a certain segment of medium-experienced users.

drunken monkey → created an issue. See original summary → .

Merged.

Should be fixed in this MR.

drunken monkey → created an issue.

Thanks for creating this feature request. As said in the other issue 💬 Looking for help displaying the suggestions in a view Active , I think this is a good idea. (Though I’m a bit confused why this was never proposed before …)
However, I currently don’t have the time to work on any feature requests, so this will have to wait until someone else creates a working MR for this (which I’ll happily review).

Seems like you might have created the view using different fulltext fields that aren’t there anymore?
Try re-saving the config form for the “Search: Fulltext search” filter in your search view and then saving the view.

If you are using Solr as a backend, you probably just need to pick a different type than “Fulltext”. E.g., “Fulltext "edge"” sounds promising. Switch the field type, reindex and then try again.
In general, it is expected that only complete words might be found, depending on your configuration. This is dependent on the backend used.
If you are not using Solr, please tell me which backend you are using.

Sorry, but this bug report is pretty unhelpful in its current form. Do you have any suggestions for how to improve this situation? For instance, do you suggest documenting that the module shouldn’t/can’t be used as part of an install from config, or is there some better way to prevent that?

search_api_entity_view() still had a wonky render array in there, with a superfluous '#markup' key, but should be fine now.
However, for some reason I cannot figure out the simple act of switching the search_api_test_search_excerpt view to use the search_api_tag cache plugin breaks it, resulting in an empty page (see here). I checked and I can reproduce the fail locally, even without any of the other changes in the MR – it’s really just about using the cache plugin, which shouldn’t even matter since it’s the first view of that page in the test. Not even adding an explicit cache clear to the test helps. Does anyone have any idea what’s going wrong there?

Also, this still needs a regression test for the actual bug reported.

Huh, that’s very strange, no idea how this could even happen. Do you have any custom or third party caching code for Views results that might interfere? Do you also see a warning for an “undefined array key” before the TypeError occurs?
Would be very interesting to catch the moment where the faulty data is cached via a conditional breakpoint in \Drupal\Core\Cache\DatabaseBackend::set() (or other used cache backend).

But I guess for the sake of getting this long-running issue finally fixed we can also just add an explicit check for whether that key is empty and avoid using the cached data otherwise. Added to the MR, please test/review!

It’s not just about Solr, there are other backends that support autocomplete as well. I think this way of doing it was an abstract choice, not one tied to a specific implementation. As detailed, I think it passes along all the necessary information for implementations to calculate the appropriate suggestions any ways they see fit. Changing this now might have a small benefit for one specific implementation, but might actually be detrimental to others, while definitely causing a break in behavior that all/most implementations would need to address.

So, yes, I think I’ll close this as “won’t fix”. However, if the Solr Suggester doesn’t work correctly with the data currently, it seems like the other issue still makes sense. You just have to implement the fix in a way that doesn’t require changes to this module.

Thanks for reporting this issue!
However, I’m not convinced a change to this module is needed. It seems this is a requirement very specific to one particular way of creating suggestions, and if you want a trailing space in that case then you should just add it in the place where it is needed, in the Solr Suggester plugin class. You can easily detect this case by checking for $incomplete_key === '', and could then either just use the unchanged user input (which is not affected by splitKeys() at all, as far as I can see) or use the keys set on $query and append a ' ' character.

The code is admittedly a bit complicated, so I’m not 100% sure I understand all of it, but it seems like you have everything you need in the getAutocompleteSuggestions() to ignore anything splitKeys() does, if necessary.

When updating any module (or Drupal itself) via the tarball you need to make sure that you remove any files that are missing in the new version, not just extract the new version over the old one. In this case, the search_api.views.inc file was renamed, which caused this error on your site.

In this case, simply delete search_api.views.inc when updating and the problem should be resolved.
However, in general, please change the way you update modules to avoid similar problems in the future. The correct way of updating with tarballs is to remove the module directory completely and then extract the tarball to get the new version of the folder. But just using Composer is much preferred anyways.

@remco hoeneveld: But your patch works for you? That’s at least good to know. I also updated the MR with the latest changes from HEAD.
Still needs test coverage.

@namisha jadhav: Thanks! I added that to the MR, let’s see what the test bot says.

@w01f: I did thank everyone for contributing their opinions and I am also thankful to xen for his contradictory one and the clear arguments he provided. However, they were obviously not enough to sway any of the others to change their vote so it’s a pretty clear 4-1 decision.
I explicitly announced in #7 what I would do:

It’s not a lot of voices so far, but seems there is a strong preference for sticking with events. I’m gonna let this open for a bit more and then mark as fixed.

It was of course xen’s right to try again to persuade people of his standpoint, but if no-one was engaging in the debate anymore it didn’t seem cause for keeping this issue open any longer. And as I had just posed the question, not voiced an opinion either way it wouldn’t have made much sense for me to argue with xen just for the sake of it.

I might have explained all that in #9, though, you’re probably right, so I’m sorry if that came of as rude.

@leksat: It sounds like you are talking about behavior at search time, is that correct? In that case, please create a new issue for this or take a look elsewhere (maybe debugging with git bisect) as I’m positive that this MR cannot have caused that problem.

I don’t think so, no, but does sound like a good idea.
Maybe look through the existing open issues first but then feel free to create a new feature request for it. Thanks!

OK, then I think we can close this. If there is a bug in what happened, then it doesn’t seem like it’s in this module.

drunken monkey → created an issue.

I think this decision makes sense and it should also be enforced. I guess this means that it should be a standard.
However, the only page we have about testing standards is called Drupal SimpleTest coding standards → and mainly lists things relevant for Drupal 6 and 7, so I think we might have more to do here than add a line about not using t().

Thanks for reporting this issue!

I can see where this would come from, yes – when your entity fields contain tokens, we don’t render those before compiling the excerpt, so the tokens would still be in there. We probably need to provide an option to just remove those “forcefully” from the excerpt, or maybe even render them as far as possible (which is mostly the site:* ones and maybe user:*).
However, since no-one else has complained so far this isn’t high priority right now. In case someone wants to implement this, I’m happy to review, but otherwise I’ll have to wait until more people follow/comment here to express their support.