🇦🇹Austria @drunken monkey

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.

Thanks for reporting this issue!
However, as you see by the failing test, this problem is unfortunately not that easy to fix as we do want to have our excerpts be valid HTML, which is why we need to escape the plain text version before adding the highlighting tags. Conceivably, we could also do the highlighting on the plain-text version and then escape the text right before adding the highlighting tags, but that would require larger changes to the code.

For now, would you be able to add a regression test for this problem to HighlightTest? That way we can immediately see whether a proposed solution works as intended while also doesn’t break any existing functionality.

Is it possible that this is a duplicate of ✨ Integrate Address with Search API RTBC ?

The Search API issue is closed now and the decision was to stick with events there for now.
However, the main argument was to not change anything hastily, so it’s still arguable whether that means that this module should follow suit.
We’ll either keep everything as-is and be consistent with Drupal Core or drop that consistency to be consistent with Search API and the wider PHP ecosystem instead. Not sure that is worth the effort and hassle.

I guess I’m just gonna leave this up for discussion for now, in case anyone else has an opinion here, and see if I ever decide to actually implement this change.

Marking this as fixed with the decision of sticking with events for now.

Updated the IS and added myself as a supporter. I didn’t find any appropriate section in the PHP coding standards → so I propose adding a new one.

Btw, found this nugget in the class naming conventions which isn’t really up to date anymore (emphasis by me):

Classes should not use underscores in class names unless absolutely necessary to derive names inherited class names dynamically. That is quite rare, especially as Drupal does not mandate a class-file naming match.

I’m also doing this whenever I remember, and it really does help. And while the use of function strings is definitely getting less commons (as functions are in general), dynamic references to methods have the same problem. But not sure whether it’s worth it to have a standard on this. I guess it depends on the number of actual places this would affect in the code base.

OK, thanks, that’s good to know. Then I guess we do need those additional $langcode parameters, yes.
Anyways, still need those tests, so leaving at NW.

Good to hear, thanks a lot for testing and reporting back.
Merged.

Which search backend are you using and which processors do you have enabled on the index?

Good to hear, thanks a lot for testing!
Merged.

Thanks for creating this ticket!

However, my feeling is that this is much too niche of a feature for inclusion in this module. (I also don’t really understand how changing the order would make the page request go any faster?)
If this is just about “immediate” indexing on editing an entity, then you can work around this problem with custom code on your site by overriding the search_api.post_request_indexing service and just ordering $this->operations as desired at the top of the destruct() method (before calling the parent method).

Anyways, in case you’d still prefer a solution within this module, I’m gonna leave this ticket open to see if others express interest, too. If so, we can revisit.

Can you please provide more details?

• Which backend are you using?
• Do you have the “HTML filter” processor enabled for your search index?
• Can you please include the JSON response received from the autocomplete endpoint? (Open the Developer Tools of your browser, go to the “Network” tab, then trigger the error and copy the response of the network request that appeared (to /search_api_autocomplete/{search}.)
• Is this still a problem with the latest dev version of the module?

I’m pretty sure creating a custom datasource is the correct and most promising way to implement this. The datasource plugin is what defines what constitutes an “item” in the index, and if your items aren’t entities but just parts of them then it seems you’ll need a custom datasource plugin to do that. Otherwise you’re almost guaranteed to run into conflicts with the item IDs which some parts assume to refer to whole entities (or, rather, translations) while other parts are changed to actually handle just entity chunks.

I think adapting the configuration UI in a way that makes it as clear as possible is a relatively simple task in this context. You could, e.g., instead of providing a new datasource just override the plugin class used by the entity:* datasources and then maybe (if you need both the normal and this special “chunked” functionality) provide an option switching between the handling of whole entities and chunking them.

One potential pitfall is that it sounds like editing a node to make the body text longer/shorter might change the number of chunks this node will be split into? That would mean issuing trackItemsInserted()/trackItemsDeleted() calls for those chunks in the update hook, which could be a bit confusing. But nothing that can’t be handled, I’d say, as long as you’re aware of it.

There is now a release candidate → for the Drupal 7 port.

Thanks for reporting this issue!
You are right, this is currently not handled correctly. Luckily, there is an easy way to fix this right in the database backend, namely at the point where we convert the search keywords to an SQL query.

Please try out the MR to see if this fixes the problem for you. (I even spotted an unrelated second bug, fixed that, too.)

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

Thanks for posting this suggestion.

However, it seems using Views for this purpose would be an incredible overkill. We’d either have to create a temporary table with the suggestions for every autocomplete request or write a completely new Views query plugin to support this. In the latter case, getting filters or sorts to work would require even more effort.
Currently, we also just render individual rows and pass them back to the client in a JSON array, so it would be tricky to make this work with a view and probably lose the view’s surrounding markup.

All in all, I think you’ll have to create your own module for this. You can use hook_search_api_autocomplete_suggestions_alter() to implement this functionality, I think. Just use setRender() to set the HTML you want to display for each suggestion.
If you publish the module and it has enough users, we can consider integrating it as an option into this module after all.

What kind of notice are you suggesting exactly?

In the Search API, entity translations are indexed as individual items (i.e., “node 1, fr” and “node 1, en” are indexed as completely separate, independent items), so the Views option you mention wouldn’t make sense for us: we always use the row language for rendering.
This is also why I can’t really explain how this could be happening. This is definitely working in general, there are countless sites out there relying on this functionality.

I assume you are using the “Rendered entity” row style? Then you could try debugging \Drupal\search_api\Plugin\search_api\datasource\ContentEntity::viewMultipleItems() to see whether a wrong $langcode somehow ends up somewhere in there.
In some way, some custom code or (not too popular) contrib module must be interfering here to produce this wrong result.
Or is it possible that the issue is related to caching? Have you tried disabling caching to see whether that fixes it? (This should of course also work with caching, but something messing up the cache metadata could still be an explanation.)

Downgrading priority to “Normal” since I’m positive this error is specific to your site.

I already did something like this a few years ago for the Apachesolr module → , see #254565-4: Integrate with Views front-end → . It is definitely possible: you have to define a new filter plugin for the view, execute your search in there and then use the result item IDs to filter the view.
The main problem with this approach is that it performs absolutely horribly so isn’t suited for anything but pretty small sites. Also, things like facets won’t work correctly as the search doesn’t contain the other filters you put on the view.
Architecturally, it makes zero sense to do it this way and wherever possible you should just create a Search API view instead.

That being said, in reality there are of course lots of specialized plugins provided for Views by other modules that aren‘t compatible with Search API views so there are views that cannot easily be rebuilt as a Search API view. If that is the case, and the site is small enough, then something like this could indeed make sense.
I’m leaving this open for now. If enough people express their support and there’s someone willing to implement it we might actually add something like this.

Great catch, thanks!
Merged.

It seems the bug you mention is just in a patch/MR for a different issue, not in the actual code of the module?
In this case, please provide the updated patch in the relevant issue and close this as a duplicate.
Also, please note that patches are no longer supported by the d.o CI infrastrucutre. Please use MRs instead.

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

Basically, yes, for each time autocomplete suggestions are shown to the user they will have to be retrieved from the Drupal site. There are potentially a few methods for alleviating the load, though:

• For one, you can increase the delay after typing before autocompletion gets triggered to reduce the number of requests.
• Secondly, client-side caching is already built into Drupal’s autocomplete (unless I’m mistaken) – however, it’s only for that page, so this will not help much.
• If you are using a reverse proxy like Varnish it should also be possible to configure that to cache autocomplete responses. Note, though, that the cache hit rate for search requests is often rather poor, as there are of course a lot of different keywords people could enter (at least on most sites).
• Finally, there is the “Custom script” suggester which you can use to point the autocomplete functionality to a different (non-Drupal) endpoint, which can help to decrease response times (and server load per request) significantly. Since this is advanced functionality, which will also require some custom code, it is usually not available. See the README.md file and CustomScript.php for details.

With custom code, you could of course also try other ways of circumventing the REST request to the autocomplete endpoint, like sending some frequent user inputs and their suggestions as JSON right with the initial page request, or storing received autocomplete responses in local storage to get caching across pages.

I’m leaving this open for now in case others have more suggestions or personal experience, but feel free to mark as fixed if this already answers your question.

@vensires: That doesn’t sound like the same problem. Or did you get the same The datasource with ID 'X' could not be retrieved for index 'Y'. error as the others?

A stacktrace would still be very useful, but I have a first suspect: LiveResults::calculateDependencies(), which calls $index->getDatasource() without catching the potentially occurring exception.
Please try whether this MR resolves the problem for you.

A new release is already planned, just trying to coordinate with other modules (mostly Solr) so we don’t break people’s sites. (Lesson learned from previous releases.)

Added a comma in the proposed text, but looks good otherwise.

@riyas_nr: This looks great to me, thanks a lot for your work on this! Setting to RTBC.

I amended the “Proposed resolution” section of the IS.

@eduardo morales alberti: I’m not sure this is actually needed, did you run into actual problems with the previous code? I think it should be the case that $reference_item already contains the right translation at this point, otherwise I think we’d see other problems as well.
But this is precisely why it is important to have test coverage for this – if you can provide a test that fails with the previous code then it would be clear that your changes are in fact needed.

I don’t think it’s necessary to generally enable testing against PHP 8.4 for all MRs and commits, that seems like a waste of money. I do already have a weekly pipeline that tests for PHP 8.4, I now also added one for Drupal 10 and PHP 8.1, as suggested – thanks.

Merged.

This should already work as you describe, see Index::reactToProcessorChanges() and ProcessorPluginBase::requiresReindexing(). Can you give me more detailed steps to reproduce?

Thanks for reporting this issue! It would indeed be nice to not load those Javascript files unless they are needed.
However, Javascript isn’t really my area of expertise, I wouldn’t know quite how to implement this suggested solution. Any MRs would be welcome, though!

Thanks a lot for your feedback, sounds good!
Merged.

I also created and published a change record → .

Thanks again, everyone!

Merged.
Thanks again!

Merged.

Since last week’s scheduled pipeline runs there was also one more change that affected us, the deprecation of entity_test_create_bundle() → .

Should all be fixed in this MR.

drunken monkey → created an issue.

Great to hear, thanks a lot for testing!
Merged.

Thanks a lot for weighing in on this, everyone! Good to get feedback on this decision.

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. We are free to revisit this in case there are further developments.

@xen: I don’t think that OOP hooks using events internally is really an argument, since that’s immaterial to anyone using or defining them. It’s more an implementation detail and doesn’t make them any more in line with general PHP best practices.

Merged.

Created an MR.
Note that this would only add the latency key for Solr searches, though, not when using other search backends.

Thanks for the feedback, good to hear this works.
Added my tiny change from #6 to the MR and merged.
Thanks again, everyone!

I think I would rather disallow type-hinting with \stdClass at all to resolve this, then it not being allowed in the PhpDoc would not be a problem. Type-hinting on \stdClass seems like bad practice to me and it should be discouraged if it isn’t yet in our standards.

Anyone know what 'stdClass' was excluded?

Seems like this was written in a time when Drupal mostly used anonymous objects and we just wanted to document those as object in the PhpDoc, not as \stdClass. Actual type hints were not allowed in PHP at that time, so the question of a discrepancy between PhpDoc and in-code type hint never arose.
And we probably couldn’t think of a good reason to actually require a \stdClass object instead of any object with the required (public) properties – same as I do now.

@stephenplatz: Does the patch in #25 work for you?
Otherwise, what error and stack trace do you get?

Thanks for the suggestion! However, I’m not sure I’m convinced.
In theory, it seems like a good idea to separate the rules for the two, but then the extra “Acronyms in a Class or Method name” section makes it awkward. You now have to read two sections for the naming conventions of a specific entity instead of one, which might make it easier to miss. It also looks a bit silly to have a section with just a single sentence.
“Stand-alone name examples” being a sub-section of that also doesn’t make much sense, I guess it would make more sense to bump it up to an <h3>? Or maybe all four should be <h4> under a new <h3>Object-oriented code</h3> heading (or similar)? The first half of the example could even be nested under “Classes and Interfaces”, as it only lists those.

All in all, to me it seems like this creates more problems than it solves, tbh, if only because of the acronyms rule and the example code that is pertinent to both classes and methods. (Also, I guess the acronyms rule even applies to properties? Would be strange if it were XmlSource and getXml() but $extractedXML.)

Oh, darn, it actually seems like this is a bug in Drupal Core: I created 🐛 TypeError when having exposed form in block and setting "role" attribute on it Active to address it.
As this not only affects the tests, but can actually be reproduced on an actual site if you have an exposed form in a block, I fear we cannot actually merge this until the Core bug has been fixed and we depend on a version of Drupal Core that contains that fix.
I unfortunately don’t even think we can really work around this problem in this module without an unacceptable amount of code.

drunken monkey → created an issue.

That’s a very good point, thanks. I opened 🌱 Decide between events and OOP hooks Active to discuss this for the Search API module, this module should then probably just follow suite to remain consistent at least within the Search API ecosystem.

Now that Core supports OOP hooks → the question is whether we want to keep moving towards the use of events, or reverse course to embrace hooks once again.
Please help with your input in 🌱 Decide between events and OOP hooks Active .

drunken monkey → created an issue.