🇦🇹Austria @drunken monkey

Thanks for opening the issue and already providing an MR!
For now, I wouldn’t yet merge this as Drupal 10.1 is still supported, but I’ll already look at your code in the meantime. Then it’s hopefully RTBC by the time Drupal 10.3 is released (and 10.1 thus reaches EOL).

Thanks for reporting this issue and already providing a very nice patch!
Since we now switched to using MRs instead of patches, I created an MR with your patch. (I now see that we still had the old advice to not use MRs in the new issue notice, so my fault.)

Some remarks on the patch itself:

1. First off, I prefer to avoid negations in setting names wherever possible, so I changed the setting name to delete_on_fail, defaulting to TRUE.
2. Second, your description doesn’t square completely with what your patch does. Are you sure it works correctly on your site? From your description, it would seem like the problem is not failed indexing, but failed loading of items. In this case, yes, Search API thinks the item has been deleted without us noticing it, so also deletes it from the index on the search server. I changed the patch so that the setting now controls that behavior, not the other one during indexing. (As far as I can see, the code you changed only affects items deliberately removed from indexing, e.g., by the “Entity status” processor.)
   Please tell me if that was the solution you were looking for, or whether indeed your old patch did what you wanted it to. In the latter case, I think I’d need a more detailed description of the problem and what exactly happens.
3. Architecturally, the fact that a dysfunctional data store cannot be distinguished from missing entities is unfortunate. I’d really like the datasource to throw an exception in this case, instead of just returning an empty array. Unfortunately, this is currently not even allowed by the contract of \Drupal\search_api\Datasource\DatasourceInterface::loadMultiple(), so really just my own fault. Then I guess this new setting is probably really the best solution (even though it might mean that some actually deleted items stay in the index on your site).
4. This is a small enough change to not need much approval, I think, so I’ll just merge this after hearing back from you and waiting for a week or so for any additional feedback. However, I’d guess this will not be a massively popular option. Therefore, and because it’s really only something for advanced users, I think I’m removing it from the index config form. You, and any others in need of it, will have to manually enable it via a config export/import. I hope that is acceptable, but I really don’t want to push yet more options on already overwhelmed new users.
5. Finally, this will need automated test coverage before it can be merged. Would you be able to provide that? There is already a test for the affected functionality in \Drupal\Tests\search_api\Kernel\Index\IndexLoadItemsTest::testMissingItems(), we’d just need to add a copy of that method that verifies that the deletions do not happen when the new setting is disabled. (Doing it via a @dataProvider would also work, not sure what makes more sense.)

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

Thanks for the patch, @alfaguru, and thanks for reporting back, @VasiliyRepin!

Adding explicit options for all indexed fields indeed seems like a relatively easy solution that might help some people. I would however doubt that most people have those properties they want to aggregate indexed as individual fields, too. In a lot of cases, this wouldn’t make much sense.
Anyways, it’s a very small addition, so as long as it helps some people I don’t really see a reason against it. However, it’s important we don’t cause a regression here by removing the code dealing with already selected fields.

I created an MR with your patch and added some changes by me (in a separate commit), adding comments and reverting the removal of the existing code avoiding silently deleting config.

Thanks a lot for your patch!
In the future, though, please remember to set the issue status to “Needs review” when posting a patch or creating/updating an MR that you feel would be ready for being added to the project.
Also, in this particular project, we have now switched to using issue forks and MRs exclusively, patches cannot be tested by the test bot anymore. I now created an MR containing the changes from your patch.

Now, some notes on the patch itself:

1. The new config key would need to be reflected in \Drupal\search_api_autocomplete\Plugin\search_api_autocomplete\suggester\Server::defaultConfiguration() and config/schema/search_api_autocomplete.schema.yml (under plugin.plugin_configuration.search_api_autocomplete_suggester.server).
2. Your current code doesn’t really do what the config form description says: the description says the new option will “remove non letter characters”, but instead you remove any suggestions ending with such characters entirely. As per my comment #4 above (about both “foo” and “foo.” being suggested) this might make perfect sense, but then this should be clearly stated in the description.
   Maybe even better, there might be different settings for this option, about what to do with non-alphanumeric characters: ignore, strip, strip when at the end of the suggestion, remove whole suggestion when ending with one. However, when stripping characters we have to be careful not to run into this issue with duplicates, so if enough people are interested then starting with just your implementation might be fine. The description just needs to clearly state what this does. (Also, we might want to already define the new config key in a way that would later make it easier to expand?)
3. Finally, before it can be merged this would also need automated test coverage.

In any case, thanks again for making another jab at this!

Please provide more information about your setup. For instance, what backend plugin do you use?

Very strange that the Solr plugin would try to send a POST request to /solr/index.html when deleting items. What does your Search API server configuration look like, esp. the “Solr path” setting?

PS: It seems you (like many others – it's really easy to misinterpret) are confused by the "Issue tags" field. As the guidelines → state, they aren't meant for free text tags related to the issue, but only for specific categorization purposes, usually by module maintainers.
So, if you aren't sure your current usage is correct, please just leave the field empty.

I’m open to any suggestions and MRs. I’m not that good with Javascript myself and have very limited time, so replacing all of this module’s Javascript in order to work with some replacement won’t be possible for me.

Yes, something like this seems like it would be the way to go. Instead of a processor, it could also be a Views query option (in the SearchApiQuery class). In any case, though, this will need to be declared as a feature → so backends can declare whether they support it or not.

For backends supporting the feature, the processor (or the query plugin) could add an option to the search query containing both the used date field and the “half-life” value. The backend plugin would then need to check that option at search time and modify the result scores accordingly. (One additional hiccup being that, for the DB backend, we’d actually want to support all three DBMSs which I’d guess would all need a slightly different syntax for this.)

As the keywords are passed as-is to Solr (yes, they are in the q parameter) it seems this is a Solr configuration problem. I’m therefore moving your issue to that queue.

It seems you are searching for English text, so probably you’re looking for the ts_X3b_en_* and tm_X3b_en_* dynamic fields. They most likely both have type text_en, so you’re looking for <fieldType name="text_en" … inside schema_extra_types.xml.

Looks good now. Merged.
Thanks again, both of you!

At a glance, this looks like it should have worked.

After reindexing, can you look into the indexed contents of your Solr server and check whether the field_locations_union field (probably locm_field_locations_union in Solr) is present, and the contents look as expected?
Then you could try to look at the Solr request generated for your search query (using, e.g., the Solr Devel module) and see whether that contains the appropriate filter parameters.

Thanks a lot for that! In the meantime, it seems that MR was deployed to default-ref anyways, so we can even go without this temporary modifications. I added one last cspell:disable-next-line directive (I actually had already added it, just forgot to push 🤦‍♂️), the MR should pass now. If it does, I’ll merge.

If you target 8.1+ perhaps https://www.php.net/manual/en/class.returntypewillchange.php is something you could look at? Not sure.

I had already tried that, but no dice. As the docs say, this can only be used to silence deprecation notices, not avoid fatal errors. (Not sure how it actually can be used, to be honest. It didn’t seem to do anything no matter what I tried.)

Thanks for yoour feedback.
Merged.

Wow, seems there’s still a lot to do …

Fortunately for us, it seems like Drupal 11 won’t be coming out in July after all, so we still have quite a bit of time left.

My question now would be whether we should keep the core_version_requirement: ^10.1 || ^11 line in our .info.yml file? Reverting that to just ^10.1 after we had already declared ^11 in several stable releases seems risky, but on the other hand declaring compatibility while there are still fatal errors when doing almost anything with the module is also highly misleading. And since that is the case, it doesn’t seem likely anyone would have been using this module with Drupal 11 anyways, so we wouldn’t be breaking anything by reverting the Drupal core version requirement.

In any case, I now added a note about this to the 1.34 release notes → .

Thanks for the feedback. Merged.

No, I don’t think these are covered by our FunctionalJavascript tests. I just tried it out manually and it works fine.
Thanks for finding the docs! However, the “correct code” examples they give do not work for me, and also aren’t consistent with the error message that says matches() should be used instead. So, they don’t seem very helpful.

Anyways, I think we can just merge this, should be fine.

The problem is also present in version 2.x of the module.
This adapted patch fixes the problem for me, by just not adding Javascript if the block is empty.

Patch attached, please let me tell what you think.

drunken monkey → created an issue.

Turns out the Search API module itself is not ready (currently just throws fatal errors, see #3425235-12: Make the Search API module compatible with Drupal 11 → ), so still need to wait for that, too.
I did, however, just commit the Search API Pages patch, so that’s something.

Turns out the Search API module itself is not ready (currently just throws fatal errors, see #3425235-12: Make the Search API module compatible with Drupal 11 → ), so still need to wait for that, too.
I did, however, just commit the Search API Pages patch, so that’s something.

Huh, seems the Action module was removed in D11 ( 📌 Remove Action UI module Postponed ) and some of our tests depend on that. So, another thing to fix …

drunken monkey → created an issue.

drunken monkey → created an issue.

Search API will unfortunately need to add an array return type hint to BackendPluginBase::__sleep() (see #3425235-12: Make the Search API module compatible with Drupal 11 → ), so the Solr backend will need to do the same in order to avoid a fatal error.

I’m not quite sure yet when I’ll merge my Search API issue, but it would be good to get this fix in for the Solr module as soon as possible.

I now just committed that issue myself and triggered a pipeline with Drupal 11 testing. Unfortunately, it showed that there is currently a fatal error, as Core added return type hints to the __sleep() and __wakeup() methods of

I created an MR that will hopefully fix this. Unfortunately, it will break all classes that override those methods (i.e., some of the modules providing backend plugins) even with Drupal 10, so this will probably not go in right away. First, as many of the sub classes should be fixed as possible.

I’ve deactivated weekly testing against D11 again for now.

Committed this. We’re gonna see whether the module actually works with Drupal 11 via the Search API Autocomplete pipelines.

Still waiting on Facets ( 📌 Make module compatible with Drupal 11 Needs work ) and Search API Pages ( 📌 Make module compatible with Drupal 11 Needs review ) for that.

Keeping this open so I remember to turn on testing against Drupal 11 once it’s available. However, still waiting on Facets ( 📌 Make module compatible with Drupal 11 Needs review ) for that.

Since no-one objected I’m now just committing this.

Still waiting on 📌 Make language_fallback_fix compatible with Drupal 11 Needs review . Adding that as a related issue.

Created an MR.

drunken monkey → created an issue.

This indeed worked as desired. Created an MR.

drunken monkey → created an issue.

Merged.

Should be fixed in this MR.

drunken monkey → created an issue.

Done as part of the 1.0 release.

Created this MR which should fix this.

However, not trivial to find out what Prefer matches to $.is is supposed to mean, for a JS non-expert.

I think your problem might be that you have also enabled the “Taxonomy term” datasource. This is only necessary if you want taxonomy terms as result items alongside content, which I don’t think is the case. If you just want taxonomy terms indexed as part of content items (i.e., you want to be able to filter content by them) then you don’t need them as a datasource, just enable the corresponding fields on the “Fields” tab.

What is probably happening (to check, you could add the “Index Property Index: Datasource” field to the view) is that the first results shown are taxonomy terms, and since they don’t have any of the fields you configured (since they are all from the Content datasource) nothing is shown.
Simply removing that datasource from the index should fix your problem. Please re-open if that is not the case, I’m tentatively marking this “Fixed” already.

GitLab CI seems happy, so I merged and created 🐛 Fix ESLint errors Active as a follow-up for the ESLint errors.

Thanks again for all your help on this, @jonathan1055!

drunken monkey → created an issue.

Thanks for reporting that issue!

We already have ✨ Support for entity reference filtering like TaxonomyIndexTid Postponed for adapting to that issue once it gets committed. Adapting existing code to be compatible with the changes of course has higher priority – though I guess this would only concern custom code using our filter trait as long as we don’t provide our own subclass of the new filter plugin. So, not sure if we need to concern ourselves with that.

On the other hand, of course, we needn’t even wait for that issue, but could add the return type right away – narrowing the return type in a sub class is legal, after all.
However, this in turn might affect custom code using our filter trait (though it might be that this would only be the case in pretty complex setups), so just adding a new return type isn’t a completely harmless step, either.

All in all, I’m not sure how to proceed here. I’m open for input from anyone else!

Oh, seems the Lorem Ipsum changes are now also deployed. Created an MR to remove our custom workaround.

Sure, here you go: #3442293: Make it easier for contrib modules to specify additional CSpell flag words → .

drunken monkey → created an issue.

Sorry, still not enough information for me to do anything. If I can’t reproduce it, there is no failing test, no stack trace or other detailed technical analysis, then I can only guess at the root cause. Since the error occurs when Core is writing to the config cache, it is still even unclear if this problem is really inside this module, or if it’s a bug in Core.
Is the exception also thrown in Drupal\Core\Cache\DatabaseBackend->doSetMultiple() in your case?

That a dump of the search results (I guess?) is displayed on the search results page seems like a completely separate issue, as none of the others mentioned that. Please create a new issue for that. However, I don’t think I heard about anything like that before, so it seems pretty specific to your setup. Maybe you just need to disable/restrict error reporting? In any case, without more detailed technical information I won’t be able to help with this problem, either, I’m afraid.

Seems like the “Aggregated field” provided by Search API would already mostly cover that use case, except that the UI currently doesn’t allow you to specify nested properties as the source of the aggregation (as you say, you can only aggregate the whole entities) – see ✨ Allow adding indirect fields/properties in aggregate_fields processor Active . However, you can already get this to work correctly by directly editing the search index’s config (see, e.g., comment #20 in the linked issue for an example). Adding support in the UI for this is, unfortunately, not that simple.

I will go ahead with your recommendation of trying to see the request that is sent to Solr (I'm not sure offhand how to do that on Acquia hosting but I will research).

If you’re using Devel, there is the Search API Solr Devel module (already included when downloading Search API Solr) that lets you log Solr requests. Otherwise, you can add custom code mimicking \Drupal\search_api_solr_devel\Logging\SolariumRequestLogger.

In the meantime, if you have any prior experience with Acquia's configsets and know of any gotchas, please let me know. I can download the config set so if there is anything in there I should paste here, I can.

The configuration of the field type used for your fulltext fields would be interesting, but for that we’d first need to know the Solr field names of those fields. Then you can look up the used type by comparing the <dynamicField> elements in the Solr schema (usually contained in the schema_extra_fields.xml file). Finally, find the <fieldType> element where the name attribute matches the field type and post that here.

Pointing to the new parent issue.

@lucasgrecco: Thanks!

Great to hear, thanks for reporting back!
Merged. Thanks again!

I can see that if a maintainer wants to add "please" into the flagwords, like you do, they have to take a copy of .cspell.json and modify it, which has a down-side as you say. I think this is a very good candidate for another _CSPELL... variable, _CSPELL_FLAGWORDS to make it simple for extra words to be flagged, without having to create the config file. If that sounds good to you I can work on it.

That would be great, yes! Thanks for the suggestion!

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

The attached patch should fix both of these problems.

drunken monkey → created an issue.

drunken monkey → created an issue.

Looks mostly good. A few notes, all addressed in the attached patch revision.

1. +++ b/src/EventSubscriber/SearchApiSubscriber.php
   @@ -20,24 +21,29 @@ class SearchApiSubscriber implements EventSubscriberInterface {
   +    $mapping['solr_text_custom'] = $mapping['text'];
   

   Should solr_text_custom_omit_norms be included here, too?

2. +++ b/src/Plugin/search_api/backend/SearchApiSolrBackend.php
   @@ -776,6 +776,7 @@ class SearchApiSolrBackend extends BackendPluginBase implements SolrBackendInter
   +      'solr_text_custom',
   

   I assume this should have the solr_text_custom_omit_norms type, too?

3. +++ b/src/Plugin/search_api/data_type/CustomTextDataType.php
   @@ -11,7 +11,7 @@ use Drupal\search_api\Plugin\search_api\data_type\TextDataType;
   - *   fallback_type = "text",
   + *   fallback_type = "search_api_text",
   

   For all of those: Fallback types should be Search API data types, so those are already correct as-is.

Thanks a lot for creating the issue and MR.
Your code already looks pretty good, just some code style issues. However, if we have that method, we should aim to cover all cases, so it would be great to also return FALSE if a condition group contains nothing but other empty condition groups. In that case, no condition would be added to the query, but $this->conditions is non-empty. I amended that, explained it in the doc comment and also added a bit more of testing.

Please review and I’ll merge. Thanks again, in any case!

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

Thanks a lot for creating the ticket and merge request. Looks all good to me.
Merging. Thanks again!

It seems like you either have Solr set up incorrectly (if you customized the configuration compared to the one the Search API Solr module generates) or the keywords do not correctly reach Solr. Normally, if something is a stopword, it should be properly ignored both during indexing and during searching. If it is only ignored during indexing then you’d see the issue you describe. Properly configured, though, the search terms walk the dog should be treated exactly like walk dog, completely ignoring the “the”. (Ignoring a bit of magic to make phrase searches still work properly.)

Anyways, search views can also configured to ignore keywords below a certain length – see the “Minimum keyword length” of the “Search: Fulltext search” filter. So maybe that’s the problem? Though this should also just discard those short keywords completely, not cause 0 results, so that could not be the entire explanation.

My first step would be to check the request that is sent to Solr. If the keywords appear there, then you are likely looking at a Solr configuration issue. If not, then the problem is somewhere in the Drupal configuration – or, probably, in you custom code.

In any case, there has to be something wrong with your setup specifically – in general, this should all work fine out-of-the-box.
As a last ressort, if you can identify why those short words are treated specially, you could also remove that configuration to just have them treated normally. Having them cause empty result sets is of course the worst possible outcome for common words.

Thanks for creating the patch!
This mostly already looked good, I just spotted a couple of faulty changes, and sometimes in doc comments the line wrapping was now off.
Made a few adjustments and created an MR so this can be tested.

Great to hear, thanks for testing!
Merged. Thanks again!

Thanks for looking into this. In the meantime, I have resolved the issue in my modules (see 📌 (Try to) fix the GitLab CI RTBC ) at least for GitLab CI (and disabling Drupal CI, as needs to be done until June anyways). So, I can’t really help you here much anymore. I think the Search API Solr module → was previously affected, too, but this also seems like it might be resolved? (At least their pipelines now fail with different errors.)
So, if no-one is affected anymore, we might also just close this issue? Seems this problem was only present for very particular module relationships.

Yes I will, but you can also check yourself. When #3436206: Add lorem-ipsum spelling dictionary → is merged you should see the commit listed in this link:
https://git.drupalcode.org/project/gitlab_templates/-/compare/default-re...

Thanks, that’s very simple to follow. Then I’l just do that.

Do you think that 'please' should be added back into the flag-words for contrib?

I’m not sure, honestly, otherwise I would have created an issue. I personally want to follow that rule in this module, and I do think it makes sense that, if Core thinks the word should not be used, then contrib modules should also follow that. Especially those that care enough about standards to actually have cspell run on their code.
However, I’m still hesitant to proscribe this to all modules. I think I’d rather just enable it for this one – unfortunately, that doesn’t seem possible without forking the .cspell.json file (and thus not getting further updates for it).
I think the ideal solution would be if it would be easy to enable/disable specific flag words (or specify your own list). Then the default value wouldn’t matter as much, and I’d probably argue that the same list that Core uses should also be default for contribt. However, as it currently is, making this the default would make it rather difficult for modules that do want to use the word (especially if it’s in lots of places in their code) but also want to pass cspell checks.

Is there any link where I can read your discussion on the topic, why you chose to remove “please” from the list? (In any case, it’s clear why it’s just “please”, all the others are just wrong/undesired spellings of words that are still allowed. “Please” is the only one Core just doesn’t want used at all.)

As discussed on Slack this seems to not be a bug in this module after all, but one in the Solr module that was simply uncovered by the recent change in this one. I created a follow-up issue 🐛 Invalid type "text" specified in SolrFieldDefinition data defintions Active in the Solr module but will still create a new release of this module with the workaround in the MR to keep more people from being affected.

drunken monkey → created an issue.

Created an MR with your patch and some minor style changes. (Testing of patches is not working anymore, we now need to use MRs – a bit funny, I know.) Would be great to get some quick feedback on whether this still worked for you, then I’ll merge and create release 1.34 – hopefully without any critical bugs this time.

And then we’ll either keep this open or create a follow-up to develop a proper fix.

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

Here → is the release.

As noted in the release notes, however, this won’t restore the caching strategy you had set on your views before the update to 1.32. You will need to manually restore the proper caching setting for them, in some way.

Fixed. Will now create a new release.

Thanks a lot for reporting this issue, and thanks @Taran2L for notifying me via Slack! This is indeed a critical bug which needs to be addressed in a new release ASAP.
Created an MR so this can be tested and will merge and create a new release right away once the tests come back green.

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

quietone → credited drunken monkey → .

Thanks for the feedback. Then I’ll merge this right away. Would be great if you could notify me once I can revert the changes to the used template file project/branch.
In any case, thanks a lot again!

I am interested to see how you get on with the downloaded .cspell.json file, as I do not actually have cspell locally!

Well, I just used the one already included in the Drupal installation. I just ran yarn install in core/ and then ran ../../core/node_modules/.bin/cspell . inside the module directory. After downloading and adapting the .cspell.json file from the CI artifacts this worked like a charm – so, once again, thank you for your help! Then I can just run this locally as a pre-commit hook (as I already do with PHPStan and others) to spot any new typos right away. Plus, I can make sure that “please” stays out of this module after we fix 📌 Remove uses of "please" from this module Active . (Though I guess this still wouldn’t guard against MR commits by others …)

Keeping this issue open so I can still revert the changes to the used template file project/branch.

drunken monkey → created an issue.

I think I have it fixed now in the MR. Please test/review!

Incidentally, I now saw that I was actually wondering about the two lines I now deleted back when adding partial matching ten (!) years ago: #1299238-74: Add option for partial matching → . The one who wrote them couldn’t explain why they were there but since tests passed either way I decided to keep them in there.

Thanks a lot for reporting this problem!
After a bit of experimenting I was indeed able to reproduce it and write a test for it – I put it into this MR. Didn’t have time to develop a fix yet – the code is unfortunately pretty complex (as are the generated SQL queries), so it’s hard to fix this without breaking other functionality.

Just one note: When reporting SQL errors it’s always very helpful if you note right away which database you are using. But it’s MySQL, correct?

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

Committed those changes, but leaving open in case there is more.
Glad to hear the bot thinks the module is compatible now, though.

drunken monkey → created an issue.

Disappointingly, this ran against an outdated module version, consequently the MR just attempts to fix watchdog_exception() calls which we eliminated a month ago.
Leaving this open, though, in case the bot still produces something more useful.

In case others stumble upon this issue, please also see 📌 Make module compatible with Drupal 11 Postponed .

Nvm, that was for the wrong module. Seems Update Bot can’t find any faults in this one, which is encouraging.

Marking this “Fixed”, further work will be done in 📌 Automated Drupal 11 compatibility fixes for search_api_saved_searches Needs review .

Would be great if some of you could test this to make sure this would now finally work reliably.

Thanks again for all your work and help on this!
The pipeline passes now, so this would be mergeable. However, I’m not sure whether it’s better to do that right away or better to wait until #3436206: Add lorem-ipsum spelling dictionary → is fixed? Probably the latter, but that makes it more likely that merge conflicts will be introduced.

Also, one more question: Is there an easy way to run CSpell locally with the same settings as in GitLab CI? I tried to just copy .cspell.json from core/ to this module (excluding it from Git so I can just use it locally) and adapting the paths, but still there are lots of errors – even ignoring the Lorem Ipsum ones. E.g., “cacheable“ and “cacheability” are reported as typos.

Furthermore, it seems GitLab CI doesn’t check for the forbidden words that Core uses – especially “please”, which this module unfortunately uses a lot. Do you know whether that is on purpose, should contrib modules not avoid using “please”, too? If so, is there a way to get the forbidden words checks back for this module’s CI? (There would be a lot more to fix in that case, but not too much to make it infeasible.)

Great to hear, thanks for reporting back!
Merged. Thanks again!

Great to hear, thanks for testing and reporting back.
Merged. Thanks again!

Test bot was happy, too, so I merged the MR with @klausi’s fix.
No test coverage for D7, unfortunately, but seems innocent enough so I just merged that, too.

Thanks again, both of you!

Thanks a lot for proposing this, good catch!
It seems almost impossible to exploit, but I’m the last one to argue against sticking to best practices, so of course such a check should use the correct function.

I created an MR with @klausi’s patch so it can be tested. I’m also posting a patch for Drupal 7 with the same fix.