🇦🇹Austria @drunken monkey

Thanks a lot, looks great!
I just had some small style adjustments. Please review again and I’ll merge.

I also had the idea that it might be helpful to auto-expand properties like field_tags:entity, where there is just a single child property. I imagine any time you click on the “Expand” button of field_tags you will also want to expand the nested property (and no real harm even if you don’t). What do you say?
Could also easily be a follow-up, too, though.

Great job. Merged!
Thanks again!

Thanks for your work on this!
I made a few updates and created a change record → .

There’s one more thing before I can merge this, though: The new service should have an interface that defines all the methods it provides, the implementations should then just be documented with {@inheritdoc}. Then only this interface should be used for any type-hinting. (It seems we should also add the interface FQN as an alias for the service identifier, to be in line with how Core does it?)
Could you please make that final change?

Otherwise, this looks good now, thanks again!

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

I now tested the UI and it indeed looks great, thanks again!

I did find one bug, however:

As you can see, it is now not possible anymore to add nested fields of linked taxonomy terms (or other entity reference fields). I already wondered about the count($nested_properties) > 1 condition when going through the code changes, seems that’s not entirely correct. I assume this is used for cases where a complex data type just has a single, unexpandable main property which can just be indexed by adding the complex property itself to the index. However, you do still have to check whether that single property has a complex data type itself.

Seems it would also be good to have a test for that. \Drupal\Tests\search_api\Functional\IntegrationTest already vists the “Add fields” page but then does nothing on it – maybe we can just change that to add some nested property to the index to ensure this works.

I also see that you removed the explicit sorting of the fields, those are now displayed in the order in which they are returned by the entity type manager. Could you elaborate on your reasoning for that change?
Above, you write, “Now that the order is fixed above by adding rows in the correct nested order in the first place, a sort is no longer needed”, but I don’t think that was the reason that the sort was placed there. Rather, we wanted to make it easier for people to find the properties they were looking for. While I concede that this new ordering also has a certain logic to it, I’m not sure it’s an improvement for users, and the probably closest analog in Core, Views, does just sort the fields alphabetically.
So, maybe we should keep the fields sorted alphabetically for now and, if you want, create a new issue for changing the order to the built-in one?

@sokru: That’s also a good point. Probably, it would be good practice to do it wth deprecation and change record, even though it is pretty unlikely that this will actually affect anyone (and even in that case it would just result in a useless template file lying around, no actual bugs or anything).

Thanks a lot, this indeed looks great!

I’ve added some comments and changes to the MR, please test/review. (I didn’t get round to actually testing the functionality, will do that tomorrow. Sorry if I broke anything.)

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

Thanks for your work on this!
I went over the code and found a few places where a) the wording of UI text could be improved or b) the code wasn’t working 100% correctly in all cases anymore. All of this should now be fixed in the MR. Please test/review my changes and tell me what you think.

In any case, I agree with @borisson_, this definitely needs some test coverage. The number of potential code paths in \Drupal\search_api\Plugin\search_api\processor\RenderedItem::addFieldValues() borders on disconcerting.
I did, however, now already adapt all the views included in the module (mainly test views) that use the SearchApiRow plugin to utilize the new :default option, so we should at least get some very rough test coverage for the Views part that way. As our test coverage for the Views integration is generally rather shoddy, I’m not sure whether doing more is necessary, though it would of course be great.

@borisson_: OK, so what would you (or @webflo) say is the best way forward here?

How about instead decreasing the used UID value at each run?
Using a random value seems like it would lead to rare but all the more confusing bugs.

Still NW for the tests.

@alexl: Thanks, should be fixed!

Huh, I thought I’d replied to the latest comments already. Sorry.
Anyways, returning [] from search_api_views_data() seems like it would remove all the Search API tables from views, breaking all search views on the site. You can’t tell Views “this is not a real result, just a placeholder” in the hook implementation, what you return will get cached and used until the next cache rebuild.

Also, avoiding something as simple as a config entity load in hook_views_data() seems like it really shouldn’t be necessary. I’m sure there are lots of hook implementations that do this, if this really causes problems in some scenarios I’d say this would be a bug in Core.

@scott_euser: Thanks for linking to that, seems like it’s worth a try. (Though it doesn’t seem like it’s completely the same problem.)

Great to hear, thanks for testing!
Merged.

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

Discussed privately and concluded that this is just a matter of documenting that the value key is not HTML, so shouldn’t be used as such by custom frontend code.

@bojan_dev: Where would you say is the best place to add this clarification, where do we have the highest chance of reaching frontend developers?

Merged.

Created an MR, let’s see how it goes.

Will be fixed in 📌 Test Coverage: Fix PHPStan warnings. Active .

drunken monkey → created an issue.

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

Luckily, I found this MR by jonathan1055 → which demonstrated how to add CI testing for EOL Drupal versions, so there is now even test coverage for Drupal 8.
Merged.

Sorry, but without further information about the stack trace it won’t be possible to fix this. We already try to catch exceptions wherever they may occur, so if we missed one place it will be very hard to find.

Created a change record → and an MR.

Thanks for reporting this issue!
I can immediately see the problem: While we always assume an item’s boost (or score) value is non-negative, there is nothing in the code actually ensuring this.
The fix is pretty simple for the “Number field-based boosting” – we even already have it there for date fields, we just didn’t think to also apply this for general numeric fields.
Created an MR for fixing this, please test/review!

I also created 📌 Deprecate negative item scores/boosts Active as a follow-up to officially declare and verify that boosts cannot be negative.

drunken monkey → created an issue.

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

Thanks for creating this issue!
It’s indeed not ideal UX that you have to configure the view mode for each bundle separately, especially if you add bundles after configuring the row style or field. (The field will at least post warnings to the log to point the user to this problem.)

For field-based processor (like Tokenizer, Ignore Case, etc.) we already have something similar, the “Enable on all supported fields” option added in #2867809: Add new fulltext fields to enabled fields processors → . That solved a very similar problem, namely the need to reconfigure all these processors when adding a new fulltext field. The solution here would be a bit more complicated, as it’s a select instead of a checkbox and we still need separate settings for different datasources (entity types), but seems something similar could work here, too.

Maybe like this: For each datasource, we have a select box with all the different view modes for that datasource, plus an option that says something like “Select per bundle”. When that latter option is selected, we display the additional select boxes for each bundle, otherwise the datasource setting juts applies to all bundles and the bundle-specific settings get hidden.
Alternatively, the datasource-level setting could just have the view modes as options, the bundle-specific select boxes could always be visible but we add a new default option to them, “Use datasource-level default”. (All labels mentioned here up for debate.)

If you want to work on an MR implementing something like this, that would be great!

drunken monkey → created an issue.

Hm, completely failing on a deprecation is a bit much, though, I’d have hoped to just get a warning …

Great initiative, thanks! I already merged your first MR, just making existing methods available seems like a low-hanging fruit.

My thoughts on the rest of your suggestions:

• setServerById(): I would just call that setServerId() maybe? Would seem like a natural extension from the existing getServerId(). It’s just a bit unfortunate that getServerInstance() and setServer() don’t follow this sensible pattern.
• addDatasourceByIdAndConfig(): We already added a separate config action plugin for that in ✨ Create a config action to add a data source to an index Fixed . However, if we add new methods for the other plugin types, doing it for datasources would also make sense for the sake of consistency – not sure how to handle that, we probably don’t want to offer two config actions for the exact same thing.
• setTrackerByIdAndConfig(), addProcessorByIdAndConfig(), addFieldByFieldConfig: I admit I first balked a bit at thes suggestions, as I dislike such verbose method names and the methods would pretty much duplicate existing ones. I therefore wanted to suggest instead adding dedicated config action plugins to implement this functionaliy, as we already have for datasources (see above).
  However, on the other hand it does seem a bit strange to have something available to site admins but not to developers. Also, I suspect the DX of those new methods would be better anyways than of the existing ones – I would now say we probably went the wrong way there when first writing the D8 version of the module.
  So, I guess I’m now on board with just adding those methods. I can also imagine our own code subsequently drifting towards using those new methods. They certainly seem handier in most situations. (Though the lack of dependency injection for entities continues to be vexing in this regard.)
  As mentioned above, though, we should think about what to do with datasources. We will probably want to add the addDatasourceByIdAndConfig() method in any case, but maybe just not make it available as a config action?
• setBackend(): This would be a major architectural change as a server’s backend plugin is currently immutable. I would therefore need a very good argument why this would be helpful/needed before considering it.
• setReadOnly(): Yes, good idea. No clue why this is currently missing.

Note that these should all also be added to the interface, in my opinion, which means you’ll have to add them to the UnsavedIndexConfiguration class, too. (Just as a tip, as I regularly forget that as well.)

Methods that are not in Index class yet, but might be useful:

• setServer
• setReadOnly

setServer seems like a typo, as that method already exists (and is listed by you a few lines above). Do you know what you meant?

I agree with @borisson_, this looks great. Thanks!
Merged. Thanks again!

@michaelbrownjr: Good point, thanks! Better clarify.

Found it!

Thanks a lot, great job!

I just had some minor adjustments, the most relevant being that the #disabled setting on the “Enabled” checkbox makes even less sense now than before, so I removed it.

If you are fine with these changes then I can merge. In any case, thanks a lot again!
Reviews from others of course also welcome.

Suggested solution:
I think we should only allow the execution of search_api_views_data() if triggered by drush cr or /admin/flush and stop its execution if triggered by a regular page request.

If we do that, won’t Views just cache that empty array as the new Views data for the Search API, breaking all search views?

Also, I’m pretty sure other code is loading config entities during their hook_views_data() implementation as well, so if Views or Drupal cannot deal with that (reliably) I don’t think us working around that problem makes much sense – it should be fixed directly in Core.

I think it would also be helpful to include guidelines and examples/suggestions regarding acronyms in class and variable names. Sometimes you see stuff like SMTPManager, which is harder to read and looks bad, versus SmtpManager, which is preferable. Same with variables like $mySmtpManager. The acronym should be treated and (title/lower)cased like a word.

This is already in the current standard, see item 3 of Naming → .

drunken monkey → created an issue.

Created an MR.

Would be great to let the CI test for deprecations, but unsure how to do that.
Locally, I just used the .deprecation-ignore.txt from Core but added a line for ignoring all deprecated Search API hooks (since reporting those is pointless).

drunken monkey → created an issue.

Huh, for some reason switching caching for the view to “Search API (tag-based)” breaks the existing test for the “Create excerpt even if no search keys are available” option, no idea why:

• Page with existing code
• Page in the MR

Postponing this to work in 🐛 Excerpt fields are not rendered for anonymous users Needs work instead, please follow that issue, too, if you are still interested. Seems the two problems are too closely related to fix either one on its own. This issue might just be closed. (I’ve added issue credits in the other issue already.)

Thanks a lot for your feedback, this is very valuable information!

I now combined these two patches into a new MR. Would be good to get some more confirmation from people that this either helps or at least doesn’t make things worse for them.

My big problem is that I can still not reproduce this, so can’t confirm this is properly working.
And, in any case, we definitely still need a regression test for this before it can be merged. I don’t think it should be too hard to add this as a new method to our existing \Drupal\Tests\search_api\Kernel\Views\ViewsDisplayCachingTest or \Drupal\Tests\search_api\Functional\ExcerptFieldTest? (I changed the latter to use caching for its view in the hope that this would already let the test fail, but no dice.)
Would someone be able to work on this?

Could you please supply a few more details regarding your setup?

1. Which search backend are you using – Database, Solr, or something else?
2. Which suggester plugin(s) are you using for Autocomplete?
3. Can you use your browser’s Web Developer Tools to retrieve the exact JSON response you receive from the autocomplete endpoint?
4. In both your screenshots the offending suggestion is the highlighted one. Can you confirm that the suggestion still looks wrong if you move the highlight to a different row?

Good to hear, thanks for the feedback!
Since we’re adding a new API here, which is then pretty much set in stone, I’d still like to leave this open for a few weeks in case anyone wants to comment on the proposed architecture for this (even though the change is pretty small). I’ve also pointed people on Slack to this issue.

Thanks for creating this MR!
However, I see that the code already specifies everywhere (except for removeDatasource(), admittedly – funnily, that is the one method not mentioned in the issue title) that $datasource_id is a datasource plugin ID, in exactly those words. Therefore, I struggle to see what value these additional sentences provide. I’d very much like input from others before merging this.

One real problem I see in that area, though, which is not addressed by this issue is that setDatasources() apparently requires the $datasources array to be keyed by the datasource plugin IDs without specifiying this in the documentation.
Therefore we should either fix the documentation, or the code. (I think this would have blown up if anyone would have used this incorrectly so far, so I think changing the documentation would be fine here.)
Created 🐛 Fix contract of IndexInterface::setDatasources() Active as a follow-up for that. I’d just fix the minimal problem with the removeDatasource() docs in there unless enough people comment here to support expanding those doc comments.

drunken monkey → created an issue.

Fixed in this MR, please test/review.

While most of the places in the code concerned entity labels (search indexes in most cases, to be precise) which can be NULL theoretically, the most important changes were probably the ones concerning fields, in the AddHierarchy and RenderedItem processors, as field labels are probably more often NULL than entity labels.

drunken monkey → created an issue.

I agreee, please feel free to keep working in these old issues.

@drunken-monkey, can you try setting _TARGET_PHP_IMAGE_VARIANT to "ubuntu-apache" in your ".gitlab-ci.yml" file? As you are only running one variant, that variable can be set in the top level variables, like the other ones you already have.

I added it directly to the schedule, ran it again and it worked fine. Thanks!
(I don’t think it would make sense for me to add it to the .gitlab-ci.yml file since I normally don’t run tests using SQLite.)

I now updated the IS and added a suggested text (and my support):

Variables should be named using lowercase, and words should be separated either with uppercase characters (example: $lowerCamelCase) or with an underscore (example: $snake_case). Be consistent; do not mix camelCase and snake_case variable naming inside a file. The only exception is when adding property promotion to a constructor: properties should always use camelCase, even if the rest of the file uses snake_case for variables.

(The last sentence is the addition, everything else was left as-is.)

Great, thanks!
Merged.

Thanks, looks good!
I just moved the known words into the .cspell-project-words.txt file and lowercased them.
Merged. Thanks again!

Also, no need to add the CHANGELOG.txt entry yourself, I do that as a part of my merging process anyways.

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

Thanks, merged!

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

Looks good, thanks!
Merged.

Looks good now, thanks!
Merged.

Great to hear, thanks for testing! Merged.

@rajeshreeputra: Sure, sounds good. Please feel free to create an issue for that.

Sorry, it seems you were still working on this and didn’t want me to review yet? I apologize, I didn’t realize this until now. Lots of people forget to set the “Needs review” status, especially with the new MR system, so I’m used to reviewing MRs without that status unless the comments in the issue clearly state this is a work-in-progress.

Anyways, I have to say that it would also have made my job a lot easier if you’d used just a single issue and MR for all those issues. We now seem to be doing the same work in five different branches and I can already see us heading toward a number of avoidable merge conflicts.
Could you maybe consolidate all your work on the CI pipeline and composer.json in this issue’s MR? Then we can also merge the MR once it’s completely green, instead of some of the jobs remaining yellow for now.

It was added to make sure we test the searchstax settings form with multiple roles, with permission "administer searchstax", what is your thought?

There is no administer searchstax permission, and both of the methods used the exact same code for generating the test user. So I don’t quite understand what you mean by that.

Sure thing. It was added as D11 support started from 1.32 version.

I still don’t see the point, as users might still be on Drupal 10.2 or 10.3 at the moment. If a user wants to use this module with Drupal 11, then they’d be forced to use a version of Search API that’s compatible with that anyways, without us explicitly requiring it.

composer.json file added to make sure the dependencies get downloaded, currently the search_api is dependency but won't get download and module throws error see 🐛 Unable to install modules: module 'searchstax' is missing its dependency module search_api. Active .

Ah, yes, I already realized that the CI pipeline chokes on this for some reason. It works fine when I try it locally – as said, the Drupal Composer repository does already compute the composer.json information from the MODULE.info.yml file as far as I can see.
Still, if this is required for CI, then let’s of course add it. However, my point about not having conflicting information in composer.json and MODULE.info.yml still stands.

Oh, one more thing: Do you suggest continuously testing against all those Drupal and PHP versions, or would you comment out these variables in .gitlab‎-ci.yml‎ before merging?
In my other projects, I just use these variables for testing for MRs which might impact multi-version compatibility, plus have a weekly schedule to test against all supported versions. In general, drupal.org asks to keep resource usage for CI restricted to what makes sense, so I try not to go overboard.

Thanks for creating this issue and helping add test coverage and proper CI to this module.

Your MR looks like a good start on the whole, thanks! However, I do have a few remarks:

• Instead of explicitly ignoring config schema problems I think it makes more sense to fix them.
• The testSettingsFormAccessible() test method seems superfluous and like it would just put unnecessary load on the CI servers. I think we should just add the single additional assertion to the other test method.
• Why did you add a version restriction to the search_api dependency? Is there actually something in that version that is needed by this module? In that case, though, we should still set 1.33 as the minimum version, as 1.32 is explicitly deprecated → .
• As mentioned in the other issue, I don’t think there’s a point to adding a composer.json file if it just replicates the information in the MODULE.info.yml file. (Also, the two should not contradict each other, like they do if we add version restrictions to just one of them.)

I added all of this to your MR. Please review!

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

Thanks for suggesting these changes!
However, I don’t think either of these modules, Search API Solr → or SearchStax Search API Module → , are actual dependencies of this module.

While the module does offer some functionality that is specific to Solr, the tracking/analytics support is completely independent from that and will work for any Search API searches.
The documentation you link to is specifically for setting up a new search using a SearchStax server. (Maybe the documentation should reflect that better, though.) Also, that documentation already clearly states that the SearchStax Search API Module → is optional, depending on how your search server is configured.

Furthermore, from what I can tell it is not necessary to create a composer.json file if it merely mirrors the information in the MODULE.info.yml file, as the Drupal Composer repository will already fill in all of that information.
What we could do, though, is add these two modules as suggested packages in composer.json.

drunken monkey → created an issue.

Thanks for reporting this problem. However, it seems there are actually two unrelated problems:

1. The DivisionByZeroError you mention just seems to be triggered by a view having a page size of 0 (i.e., showing no results at all). While we of course want to prevent this fatal error, it would occur no matter whether or not you’ve configured the Searchstax module.
2. Unrelatedly, it would of course make sense to spot early on in _searchstax_add_tracking() that no analytics key is present and return without any further processing. (The check is currently unnecessarily far down in the function.)

Should both be fixed in this MR. Please test/review.

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

No idea what those fails for “phpunit (next minor)” are about (which doesn’t even seem to use the next minor version?), and it’s definitely an improvement, so: merged.

drunken monkey → created an issue.

Thanks for reporting this issue!

However, I think the “minimum word length” option already works exactly as you describe: For minimum length 3 and input “st john” it will discard the “st” but keep “john” and not display any error message. The message will only be displayed if all words were removed that way (which makes sense since then the user will just see completely unrelated items from the index).

I think what might be happening is that you’re using Search API Autocomplete and, while you type, Javascript sent a request with just “st” as the keywords. The way Autocomplete works internally it seems like this would also trigger the form validation and even, confusingly, display the resulting error message on the next page load.
To check this, you can use XDebug to place a breakpoint on the relevant (i.e., second) $form_state->setErrorByName() call in SearchApiFulltext::validateExposed() and then see when that gets triggered.

If this is indeed the cause of the problem for you, this would be a bug report for the Search API Autocomplete module. Feel free to move it accordingly. it does seem like a bad idea to display Views exposed form validation errors triggered during autocompletion. We might just want to call \Drupal::messenger()->deleteAll() after $view->build() in \Drupal\search_api_autocomplete\Plugin\search_api_autocomplete\search\Views::createQuery().

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

drunken monkey → created an issue.

@jonathan_hunt: Thanks for the feedback, did not know that this could be a direct EntityAdapter instance. But should be no harm in just checking for that, too – thanks.

Still needs test coverage, please.

@damienmckenna: I think you don’t completely understand the way Search API works. In the Search API, you can only filter on (and sort by, search through, etc.) fields that you have explicitly added to the index. When you index just field_tags, then that integer entity ID is what’s indexed on the server, and only this can be used for filtering/sorting/…. If you want to be able to add filters for any of the taxonomy term fields, you will have to add them to the search index first, as @cboyden explained in #12.
The relationships in Views are purely used for the “Fields” of the view, they don’t influence the available filters or sorts at all.

Do I understand correctly that all content entities are now considered unreliable items?

No, this is only for any item IDs queued for immediate indexing when it is unclear whether the object represented by that ID actually exists on the site. It would specifically be used for this scenario of marking items as “changed” when a referenced entity changes, the warning would still apply in any other circumstances, when an item is marked as “changed” (and “Index items immediately” is enabled) without the item ID also being marked as “unreliable”.

Would someone be willing/able to write a failing test for this? That would make it much easier to find the correct solution, I think.

The fulltext search is actually not a “condition” in the Search API, but a separate part of the search query, the “keys”.
You can retrieve them via $query->getKeys() – they’ll usually be represented as an array structure, see the doc block of \Drupal\search_api\ParseMode\ParseModeInterface::parseInput() for details.

Fulltext search basically just has two operators, AND (“contains all of these words”) and OR (“contains any of these words”). To switch to OR, you can either do

    $query->getParseMode()->setConjunction('OR');

before the keywords are set on the query or

    $keys = &$query->getKeys();
    if (is_array($keys)) {
      $keys['#conjunction'] = 'OR';
    }

afterwards.

If $keys is a string, not an array, than the way to express an OR conjunction depends on the backend. (For the database backend, there is no way to do that with unparsed keywords, i.e., if $keys is a string. Use the “Terms” parse mode instead.)

Good to hear, thanks for reporting back so quickly!
Merged. Thanks again!

It seems like the search_api.settings:default_tracker config key is not properly set? Normally, it should have been set to default on installation.
Maybe try reinstalling the module and see if that helps? Otherwise, please tell me the output of \Drupal::config('search_api.settings')->get('default_tracker'); (or look up that config key some other way).

Since the Search API module in general doesn’t provide any search pages, blocks or other user-facing UI, I don’t think adding something like this to this module would make much sense.
Since all your search pages are views there might be some Views-related contrib module that already lets you do something like this – enter something in a text field and than submit to one of several views at the user’s choice. However, I guess this feature mostly makes sense for searches, so not sure if it exists yet.

I, in any case, don’t know of something like that, so if you cannot find anything you’d have to write some code for this yourself. The code would be pretty simple – a block with a form, containing a textfield and a dropdown, that submits the entered keyword to the selected page. But only trivial if you are a developer, of course.

On the new server though, this causes a site failure:
Drupal\Component\Plugin\Exception\PluginNotFoundException: The "media_type" entity type does not exist.

If that’s the error message you get, then why is there a different one in the issue title? Those are two completely different things.

Seems sensible, but I think @mkalkbrenner → originally added the logo (plus he’s the one maintaing the Drupal 8+ version), so waiting for him to confirm.

+1

Seems the test fails later without the changes in #14, so keeping the MR like that.
Still far from a complete solution, though.

The Search API is not equipped to retrieve search results that way. You will either need something like the Search API Grouping module → or execute several searches, one for each datasource/group. In any case, you’ll probably need to provide your own custom suggester plugin for this (or override the “Live results” one).
If the code is neat enough, and other people also express interested, I’d be willing to consider something like that to the module, so I’m leaving this open in case someone wants to suggest code for this.

NW for the tests.

Any update on this?