🇦🇹Austria @drunken monkey

drunken monkey → created an issue.

drunken monkey → created an issue.

drunken monkey → created an issue.

The code in SearchApiFieldTrait is unfortunately very complex, but in theory there should already be code that ensures that fields will only be extracted if they are not yet set on the result row – see the code determining $required in \Drupal\search_api\Plugin\views\field\SearchApiFieldTrait::getValuesToExtract().
But it’s entirely possible that there are holes in that code. Still, as it’s probably rather specific to your setup, it would be great if you could determine where the code is going wrong and which additional checks might help.

I don’t think we need a new option for this, as this is already what we’re trying to do. It’s just fixing the existing functionality.

drunken monkey → created an issue.

Thanks for reporting this problem!
That’s really a doozy. Off the top of my head, I can’t venture a guess why this would happen.
Are you able to obtain a stack trace for the error? (It is thrown in \Drupal\search_api\Entity\Index::getDatasource().) That would help a lot.

(In the future, however, please don’t post config YAML as a comment, but instead use an attachment.)

@w01f: That seems like an entirely different problem, namely that you have Search API installed via Git and Drupal unfortunately does not handle that correctly. It therefore incorrectly reports a dependency version conflict.

It is known and unfortunate that Paragraphs and Search API don’t play very well together.
I usually suggest better using just a “Rendered HTML output” field for making the text inside Paragraph entities available for searches – however, in your case it seems you have already done that and it didn’t work.

The first thing I want to note, though, is that you usually shouldn’t add the “Paragraph” datasource to search indexes. This would only make sense if you want the Paragraphs themselves as search results – usually, however, users want the results to be Content items, and the Content’s Paragraphs included in the indexed fulltext for the content. In that case, you should just use the “Content“ datasource and add fields that point to the fields of the Content’s Paragraphs that you want indexed. (Or, as above, use the “Rendered HTML output” field.)

That being said, indexing Paragraphs directly should also work, so it’s still unclear why that search wouldn’t work.
To debug whether the problem is during indexing or during searching, please look into your database, in the search_api_db_default_index_text table. This contains all words indexed for the “Default content index”. By filtering for field_name = 'field_accordion_title' you should see all the words indexed for Accordion titles.

Anyways, indexing specific fields of Paragraphs is generally not recommended as it is very fragile wrt reorganizing of the Paragraph structure. (When you put that Paragraph into another one, the field will suddenly not be found anymore.) The “Rendered HTML output” field is generally the better solution, if correctly configured (to not include field labels, etc.). But debugging why that doesn’t work in your case is probably even harder, you’d probably need to add debugging code (or an XDebug breakpoint) to \Drupal\search_api\Plugin\search_api\processor\RenderedItem::addFieldValues() to see what’s going on/wrong there.

In conclusion, this is a pretty site-specific problem so I don’t think I can help you any more than this. You’ll have to debug this yourself, one way or another.

Thanks for suggesting this improvement!
You are right that I’d say adding explicit support for Paragraph entities would be a bit too specific a solution for my taste. So, making this easier for custom code to fix on its own seems like the better approach.

I wanted to comment and say that we’ve moved to using events instead of hooks and to please create one of those instead – however, I then realized that we haven’t actually made that move in this module. I created 📌 Convert all hooks to events Active for that – in case this gets merged, we’d just have to port one more hook to an event. (Though it seems less than ideal to introduce a new hook and then perhaps deprecate it a few weeks later.)

However, introducing a new hook for that also seems like a bit of an overkill, at least unless more people express interest in this functionality.
A simpler approach would be to extract the $datasource->getItemUrl($object) call into its own method on the LiveResults suggester – that way, you could simply change the plugin’s class to a custom subclass where you override that method.
Would that also work? It seems to me like that would be a less invasive solution to the problem, even though a bit more custom code would be needed as a downside.

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

drunken monkey → created an issue.

Merged.

drunken monkey → created an issue.

Merged.

Merged.

*bump* Please test/review so I can merge this.

Please respond so I can merge.

Would be great to get a) feedback and b) help on that test. It has to be something stupid, probably because I’m unused to working with translations – maybe someone more experienced there can quickly spot my mistake?

I changed my mind, let’s keep compatibility and get rid of the AutowireTrait.
Changed the MR, please test!

Merged.

Merged.
Thanks for bringing this over the finish line, Scott!

Thanks a lot, @jonathan1055, great job!

Great to hear, thanks for reporting back!
Fixed the conflict and merged.
Thanks again!

@capysara: Excerpts are already created without any HTML (except for the highlighting tags, of course) by the Highlight processor, so there is nothing more to display for the view. By switching to “Do not allow HTML” you could just make the <strong> tags (or whatever you have configured) around matches visible.

If you want HTML preserved inside the excerpt you’ll need to implement your own code for creating the it and use that instead of the Highlight processor.

Merged this. I’d consider this experimental for now, I’ll create a new release once the feature has been tested thoroughly.

This is now implemented and tests are passing locally.
I based the MR on the MR for the Migration module to avoid later merge conflicts, so that one will have to be merged first before merging this one. Still, testers already welcome!

drunken monkey → created an issue.

To be on the safe side, I re-added the constructor with a deprecation warning in case it is called with any parameters. That way we will know to handle this properly in case we ever add a new parameter.

@recrit: Thanks a lot for your review, both your comments make a lot of sense. I now amended the MR accordingly, please review!

Luckily, we didn’t even publish a change record about the new required argument for AutocompleteHelper, so hopefully just removing it will be fine. (Anyways, removing a parameter in general seems pretty harmless – unless we add a new one in the future.)

Yes, let’s just go with that.

Could I please get a few more testers for the MR?
With 63 followers there should be a few willing to bring this over the finish line, right?

(And just to be on the safe side: The whole purpose of the MR is to retry indexing during the next cron run if this error occurs during the node/entity save operation. It won’t change anything about that initial fail, except for downgrading the log message to a warning, and also won’t help if indexing the rendered content still fails during the cron run.)

*bump*

Still didn’t work.
Well, those should be resolved by the other modules anyways, so we can live with them for a week or two.

On the other hand, I now tried to fix the reported deprecations against Drupal 11.2 for using the deprecated Entity::$original property. Let’s see how that goes.

This will now be mostly resolved in 🐛 Fix failing pipelines Active , but once we depend on Drupal 11.2 we can remove the DeprecationHelper::backwardsCompatibleCall() calls.

I have to admit that I don’t really understand this whole problem, or the whole mechanism at work here. Why did Core remove those words, and why does this not cause CSpell errors in the Core pipelines?

However, from a contrib perspective it seems clear that this is a regression and at least some of those words should be added back. (Though, by the above, I have no idea whether to do this in Core or in this project.) Looking through your list (Thanks!) I see lots of words that are actual nonsense, for which it makes sense to remove them and let contrib deal with any instances of them in their code.

However, a few words I’d say are common enough (some are even on Wiktionary, I linked them below) to warrant re-adding and I think I’d be confused if any of them were suddenly reported as spelling errors:

• autowire
• autowired
• bodyless
• divs
• doxygen
• iframes
• kernighan
• overridable
• permissionless
• phpcs
• postprocess
• rethrown
• varchar

That being said, it might be the case that some of those are now just part of the inbuilt CSpell dictionary, or something like that, so they don’t need to be included anymore? I quickly tested in the Search API module and this indeed seems to be the case: only “bodyless”, “kernighan”, “permissionless” and “varchar” are actually reported as errors.
I would suggest readding those four words in some way but, again, I don’t actually know where to best do that.

Sorry about crossposting, I really didn’t think anyone pick this up in the few minutes between me posting this and then creating the MR.

Great job, thanks a lot!
Unfortunately it’s not that easy anymore to test against other databases (after the switch to GitLab CI) but tests against MySQL pass and any problems with Postgres or SQLite should be reported on the weekend when the scheduled pipelines run.
I just did a bit of code cleanup and will now merge.
Thanks again!

Great to hear, thanks for reviewing!
Merged.

Thanks, very helpful! Then yes, it seems clear that this is the right thing to do.
Merged.
Thanks a lot again!

Merged. Thanks again!

Huh. No idea why that didn’t work.
Especially: Why did the PHP 8.4 tests not report the newly added implicitly nullable type?
Trying again with a different regex.

Seems these are just deprecation warnings caused by other contrib modules. We can just ignore them here, they should be fixed in the respective modules (see 📌 Fix implicitly nullable parameter type hints Active and 🐛 Fix deprecated implicitly nullable types Active ).
I created an MR, let’s see if pipelines pass now (resp. report the one new deprecation I added for testing purposes).

drunken monkey → created an issue.

drunken monkey → created an issue.

See this MR.

I tested, but it seems like <script> tags are already stripped even in the overview, so this shouldn’t be a security issue.

drunken monkey → created an issue.

After several problems just getting the tests to run, and with consistent results, in Drupal 8, pipelines are now passing.

Since it will hardly be possible to wait until the two bugs are fixed and we depend on the versions that contain those fixes, I now instead refactored the code to use a key-value store instead of third-party settings to store the migration history. This also makes for simpler code, so it’s a win-win.
Now just trying to get the pipelines green …

The module is now working well and has test coverage. We also broadened its scope, it can now be used to migrate from any other Solr hosting provider, not just from Acquia Search.

Apart from the failing CI pipelines, there are two remaining problems that are unfortunately caused by bugs in other modules (unless I’m mistaken):

• Saving a copied index will make the Migration module “forget” that it was copied, displaying wrong information on the Migration Overview page and making switching of the index’s search views impossible. This is a bug in the Search API Solr module, see 🐛 Saving an index removes other modules’ third-party settings Active .
• When uninstall the Migration module (which generally makes sense to do after the migration is finished) all views touched by it are deleted. I think this is a bug in the Views module in Core – I reported it at 🐛 Uninstalling a module deletes all views that have third-party settings by that module Active .

drunken monkey → created an issue.

My trivial suggested solution is implemented in this MR.
Let’s see whether the pipelines succeed, otherwise we might already know the reason why this is currently not done.

drunken monkey → created an issue.

drunken monkey → created an issue.

Thanks a lot for reporting this problem and already proposing a fix!
It indeed seems like we misunderstood the purpose of the version key when initially creating the search_api.libraries.yml file. Your explanation makes sense and the code in JsCollectionRenderer::render() seems to corroborate it as well. Still, as I have very little expertise in this area of Drupal development I’ll leave this issue open another week or so to give others a chance to weigh in before merging.

drunken monkey → created an issue.

Thanks for reporting this, @heyyo, and thanks a lot for creating an MR, @prem suthar!
However, there were quite a few spots missing. Fixed those as well in the MR, please test/review and I can merge.

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

Indeed, this is just a duplicate of 📌 Nullable types must be explicit Active .

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

Wow, thanks for the regression test, that’s very helpful! With this, it is easy to see the problem.

However, I’m not convinced by your proposed solution. In fact, the more I think about it, the more it seems like our previous solution wasn’t very thought-through, either, and the comment explaining the problem somewhere between misleading and plain wrong.

As far as I can see, all the second part of the code in AutocompleteHelper::alterElement() is trying to do is make sure our processSearchApiAutocomplete() callback will be invoked. And if we just look in general at the point in the code where AutocompleteHelper::alterElement() is invoked (completely ignoring the specifics of what BEF might do) there are two possibilities:

1. $element['#process'] is not set. Then everything is fine as FormBuilder will just set the default process callbacks later, at which point our processSearchApiAutocomplete() callback will be included.
2. $element['#process'] is set. Then whatever code set that key is actually responsible for including the defaults as appropriate, otherwise it will already be causing bugs in lots of other places. We have to assume that any default callbacks that are not included are excluded on purpose.
   The problem then is only that whatever code set the #process key could not know about our processSearchApiAutocomplete() callback as the element type was different back then. So, we just have to hope that adding it won’t go against what that code is trying to do. However, there is really no point in adding our parent element’s default callbacks here.

In light of this, and assuming my above reasoning is correct, it seems to me like we should just check whether $element['#process'] is set and, if so, prepend our own processSearchApiAutocomplete() callback.
I changed the MR to this fix, keeping the tests, and it seemed to work fine. Please try it out, and also review! (I made some code style changes to the tests as well.)

Come to think of it (a little late), we might also have made both methods throw a \RuntimeException since, logically, you shouldn’t ever pass or receive an UnsavedIndexConfiguration object in an entity update hook, so making this cause a fatal error might have actually been a better idea.
But probably not worth the additional effort now (or at all), should almost definitely be fine as-is.

Ah, of course, sorry! I had just created 📌 Make use of the new EntityInterface::getOriginal() method Postponed for actually using the new methods (which of course needs the 11.2 dependency) so I was thinking about that when seeing this issue. But yes, we can and should of course fix that fatal error right away, thanks!

Now that I merged the latest HEAD into the fork the pipelines are also passing. So, merged.
Thanks again, everyone!

Nevermind, that other issue’s just about adding the methods to our own UnsavedIndex class to avoid fatal errors, not about using them in the rest of our code. So, re-opening.

Thanks for suggesting this improvement!
As the test fails show, though, your implementation is a bit rough. Instead of forcing a new property on the database query object we can just use its addMetaData() method. I updated the MR accordingly, please test/review.
I also added support for Postgres. SQLite, however, doesn’t seem to support random seeds (yet), see here.

Before I can merge this it still needs tests, though. Would you be able to add some to \Drupal\Tests\search_api_db\Kernel\BackendTest::searchWithRandom()? (We’ll just have to make sure to skip them for SQLite.)

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

You can just add the “Search result excerpt” field to the view mode of your choice and then use that for displaying the search results.
You will also need to enable the “Highlight” processor for the search index and configure it to create an excerpt.

Duplicate of 🐛 [D7 PHP 8.3] Deprecated function: Creation of dynamic property Fixed – which has been fixed more than three months ago.
In the future, please check the latest dev version of the module before creating an issue.

Just click “Translate view” in the Views UI and you should be able to translate all UI text for the view.

Thanks for reporting this issue!

However, it seems very specific to your setup, with modules I’m not using, so I’m not available for debugging this. I’d be open to reviewing and merging any MRs, though, if you find a solution that works (and doesn’t negatively impact the module in other ways). We might also need test coverage at that point, but probably pointless to start with that unless we have an idea about a solution.

Are you using Solr? Then I can at least suggest a workaround that should work pretty well. When enabling the “Retrieve highlighted snippets” setting, Solr will already give you highlighted field values for all searched fields, so you could just build the excerpt using those. (This is actually what we did in Drupal 7 – see SearchApiSolrService::getExcerpt(). We abandoned this approach for security reasons, but if there is no field-level access restrictions for any of the fulltext fields on your site then this wouldn’t be a concern for you.)
You can get the Solr response (with the highlighting values) via $result_set->getExtraData('search_api_solr_response').

Great catch, thanks a lot for reporting this and already providing an MR.
Based on a quick test, I think all of those should actually be checked with … is not empty, it seems like '0' (or 0) would be a valid value for all of those (though more likely for some than for others).
I amended the MR. Please test/review!

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

Something like this maybe?

drunken monkey → created an issue.

Thanks for posting this! You’re right, this might still help others who come across this problem.
However, I’m still marking it as “works as designed”. Alpha versions come with the explicit warning that data compatibility is not guaranteed, so problems like this are expected.

Thanks, great suggestion!
The code also looks pretty good already. Since you already helpfully added the label to the test view I just went and also added assertions for that.

I do have two questions, though:

1. In the documentation you linked it is suggested that the role and aria-label attributes should actually be on the <form> (or other container) element, not on the input field. Is there a particular reason why you placed it on the input field instead?
   I do agree that it makes sense to only activate this behavior if there is an exposed “Search: Fulltext search” filter (people use the Search API to build lots of things that aren’t searches), but I think just manipulating $form['#attributes'] in SearchApiFulltext::buildExposedForm() should work fine to still put the attributes on the form itself in this case. It’s then just a bit unclear whether that filter is really the correct place to have the configuration for the ARIA label. (Otherwise, what should happen when there are multiple such filters?)
2. Speaking of the configuration: Is there a reason you put the aria_label option into the “root” options of the filter instead of nesting it within expose? As this will only take effect when the filter is exposed, having it as part of the “expose” option seems to make more sense.

Anyways, thanks a lot again for your work on this, really a very good MR already!

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

Great job, thanks!
I’d merely suggest renaming the “Naming » Classes” sub-heading → to “Classes, methods and properties”. I was a bit confused by properties missing from the “Naming” sub-menu.

Thanks a lot for creating this issue and already providing a great merge request!

I don’t think we’ll have any problems with backwards compatibility here since making the nullability explicit makes no difference to PHP (except for the deprecation warning in PHP 8.4+). To test, just run the following locally (or run it on 3v4l.org):

class A {
  function foo(array $a = NULL) {}
}
class B extends A {
  function foo(?array $a = NULL) {}
}
class C extends B {
  function foo(array $a = NULL) {}
}

No warnings at all for PHP 8.3 and lower, just the expected deprecation warnings for 8.4.

No idea what went wrong with your pipeline there, but just re-running it worked fine. So, merged.
Thanks again!

Huh, that is indeed very strange behavior and I don’t have any quick suggestions here for you.
(Unless you maybe have the “Stopwords” processor enabled and “angstel“ and “baambrugge” are in there?)

It’s unfortunately not very convenient to inspect the contents of a Database Search index, but here is how to do it:

1. Go into your database.
2. There should be a table search_api_db_[INDEX_NAME]_text.
3. Filter that table by the item in question, something like WHERE item_id = 'entity:node/[NID]:nl'.
4. The result will list all indexed words for that item in the word column. Look for “angstel” or something similar there. If it isn’t there, then something removes that word from indexing for some reason. If it is there but maybe looks a bit off that’s another thing to worry about, but in this case the problem is probably at search time.

Thanks for reporting this problem!
This sounds a lot like 🐛 RuntimeException while trying to render item Active , which currently even has an MR to test. Do you have “Index items immediately” enabled, and are there errors logged after you save the node? Then please try out that MR and report back in the other issue. (And close this one as a duplicate.)

Thanks for reporting this problem!
I could indeed reproduce it right away: If there are translations in the view (maybe even just if the site is multilingual) there is a fatal error.
Looking at the code, I actually think we don’t even need getEntityTranslationByRelationship(). In our case, the result entities should all already have the correct language.

I created an MR with that approach, please try it out and tell me if it resolves the problem for you!

I also tried amending SearchApiBulkFormTest to catch this problem in the future, just by adding one translation to the test entities. However, for some reason which I couldn’t figure out for the life of me the translation I added just didn’t save properly. See the attached patch – maybe you (or someone else?) are able to make it work?
(For me, this fails at the last line tagged with # Temporary debug code. – the translation is just not there anymore after reloading the entity.)
In any case I think we should have a regression test for this before merging.

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

Regarding the CSpell error, I commented in #3494834-13: Add common words not in core dictionaries → and hope this will just be resolved on a global basis. I suspect this is currently biting a lot of modules.