🇦🇹Austria @drunken monkey

Sorry, but the Drupal 7 version is not actively supported anymore except for security problems.

Thanks a lot for reporting this problem, and sorry it took me a bit to respond!

I could indeed reproduce the problem, and it’s really not trivial. While it wouldn’t be too hard for the backend plugin to work around this problem (instead of $index->clear(), just call $index->reindex() and $this->deleteAllIndexItems() separately), we don’t want to force backend plugin developers to figure that out and do that – as far as possible, this should “just work”.

Unfortunately, we cannot just delete the task before executing it, otherwise we cannot guarantee any more that the task will be executed in case of something like a fatal error. We could implement a more elaborate system where workers “claim” tasks and we have those claims expire after some time (something like done here for the indexing queue) but that seems like a lot of work for this relatively small problem, plus potentially more error-prone. Also, this might mean that operations stop completely for some time if a task execution does fail.
(Though I now realize that it is currently possible that the same task will be executed multiple times concurrently, so that is also not ideal. But a different problem, one step at a time.)

It seems to me like the simpler and more robust solution would be to just prevent any nested task executions: When a task is currently being executed (i.e., during the event dispatch) we set some flag to prevent any parallel execution of tasks (both this same one and others). The problem would be then we’d then have to act as if all tasks had still been executed properly to allow the operation to go forward, which does seem a bit risky. On the other hand, it seems pretty sensible that pending tasks would not prevent us from doing any operations needed by a task currently being executed. So, logically, this should be fine, though this by no means guarantees that there wouldn’t be any problems in practice.

In any case, this approach would be implemented in this MR, please test/review!
Pipelines are passing so at least we shouldn’t be breaking something in an obvious way.

Making the restriction a bit more specific (i.e., only prevent execution of the same task, or only of tasks for the same server and/or index, etc.) would also seem like an option to prevent the specific problem you report, but I think logically we actually do want to ignore all pending tasks while executing one.

There is now a separate MR for that bug in 🐛 Fix hidden bug when entity properties are updated via ConfigEntityBase::set() Needs review , I added that as a dependency of this issue’s MR. (Before that one is merged, this one won’t pass anyways.)

OK, the fix for this unrelated bug is a bit more involved, so moving that to its own issue, 🐛 Fix hidden bug when entity properties are updated via ConfigEntityBase::set() Needs review . (Apparently, set() is also called during entity save, which causes this new bug and corresponding test failure.)

drunken monkey → created an issue.

Merged.

Merged.

Merged.

Merged.

Merged.

Merged.

Any more feedback on this?

Got private positive feedback, so: merged.
Thanks again, everyone!

Thanks a lot for weighing in on this, Joris! Always good to have some input when changing the API.
In my mind, the main use case is actually the one from the change record: You’re already checking for, e.g., $index->hasValidTracker() but then the IDE still marks your call to $index->getTrackerInstance() as potentially throwing an exception.
I agree, just replacing exception handling with needing to explicitly check for NULL would be just a huge step back by several decades, so definitely don’t want to encourage that.

With the null-safe operator, I personally feel like there is a very handy shortcut for when you just need the entity/plugin for a single call anyways. If you feel that this makes the code harder to read, I could also revert those changes to the code that use the new methods that way, that’s no problem.
If you think the additional methods encourage bad patterns in code then that’s of course a larger issue, we don’t want to do that.

Went slightly insane trying to find the cause of the test failure in IntegrationTest::testFramework(), but finally found and fixed it: The change to IndexForm uncovered a completely unrelated bug where using set() to update some of our entity properties would lead to an inconsistent internal state.
Should be fixed now, I also improved the test coverage a bit to produce a more useful error in case this would happen again.

So, if we end up not going ahead with this change, we should definitely still fix that bug.

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

drunken monkey → created an issue.

Added an MR and a change record → , please review.

📌 Move methods from Basic tracker plugin to TrackerPluginBase Active would move all methods from the Basic tracker plugin to the TrackerPluginBase class to finally make the latter actually useful. This would only affect anyone writing a tracker plugin themselves, primarily in case that plugin does not inherit from Basic currently. This change record → explains this in more detail.
Any feedback would be appreciated. I think this should be safe to do, I don’t think there are many tracker plugins out there, but it’s still a large code change.

Would be implemented in this MR.
I also wrote a change record → .

Please review.

Looks good. Merged.

This should now also be fixed, please give it another go.

Thanks a lot for reporting this problem, and sorry it took me a bit to get back to you.
Your fix looks good so far, I now also attempted the additional problem you describe in #5. Please try out the latest version of the MR and tell me whether this works for you.

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

drunken monkey → created an issue.

Implemented in this MR.

Should be fixed in this MR.

drunken monkey → created an issue.

Thanks a lot for reporting this problem!
I can reproduce it: If a datasource does not support viewing items, the SearchApiRow plugin will crash the whole page request.

However, while your patch would avoid that fatal error, I think the proper solution is slightly more complex:

1. We should amend the options form code for the plugin to not save that bogus option in the first place, since that also violates the config schema.
2. In render(), we should explicitly check whether the datasource supports viewing items and bail early otherwise. With your suggested code, we would still call $datasource->viewItem() which would likely result in an exception or error itself.

I created an MR with those fixes, please review!

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

Ah, yes, I like that even better. Changed the description and then merged.
Thanks again, everyone!

Thanks a lot, that’s perfect and very clear.
With this, the problem is very clear: The initial search result does indeed fail to filter out the node to which the user does not have access. However, as an additional fail-safe, we also (by default) check for $entity->access() in our Views integration, which would then remove that node. However, it is still present in the facet counts (and also the total result count, I bet, if you add a “Result summary” header).

I assume your index does use the “Content access” processor which normally takes care of properly excluding such items from the search results, but that uses the information from hook_node_grants() so if the Group module does not integrate with that, as @kristiaanvandeneynde hints, then it’s clear that this won’t work for group-based access restrictions.

And no, Search API does not execute an entity query (@kristiaanvandeneynde #9) so using that is not an option.
I haven’t really used the Group module yet, let alone worked with it, but based on what I know it seems like something like the proposed processor from the MR would indeed be needed to make this work properly.

Unless the Group module is amended to integrate with the node grants system after all, of course. Did it ever do that or was that never an option?

(Re-adding the “Needs tests” tag, sorry for accidentally removing it before!)

Thanks for reporting this problem!
However, I think that code is very easy to repair to also work if the datetime module is not available, no need for adding it as a dependency. Please review my MR and see if it fixes the problem for you.

However, I also added a regression test but that actually didn’t fail without the fix. Are you able to modify it in a way that demonstrates the problem – i.e., that fails when reverting the fix?

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

drunken monkey → created an issue.

Seems like a good idea to me. This seems to be almost universally followed (see below), but still good to document.

$ git ls | grep -F '.html.twig' | xargs grep '{{ [^}]*[a-z][A-Z][a-zA-Z]\+[^(a-zA-Z]'
core/modules/announcements_feed/templates/announcements.html.twig:            <div class="announcement__date">{{ announcement.datePublishedTimestamp|format_date('short') }}</div>
core/themes/olivero/templates/dataset/item-list.html.twig:      <li{{ item.attributes.addClass(listClasses) }}>{{ item.value }}</li>

This is definitely still a problem, see the recently filed 🐛 Full user input is not searched when it contains a comma "," Active .

You’re right, that’s ambiguously phrased. I changed it to this, which should also make it clearer that time spent will usually be a bit more than specified, not exactly (e.g.) 10.0 seconds:

The maximum number of seconds allowed to run indexing per index. Started batches will be completed before stopping. Defaults to -1 (no limit).

Do you think this is better, or do you have another suggestion?

Yes, this problem still seems to be present. I currently have the following (working) code in the Search API module (in src/Form/IndexFieldsForm.php), where the given route uses a CSRF token:

        $build['fields'][$key]['remove'] = [
          '#type' => 'link',
          '#title' => $this->t('Remove'),
          '#url' => Url::fromRoute('entity.search_api_index.remove_field', $route_parameters),
          '#attributes' => [
            'class' => ['use-ajax'],
          ],
        ];

However, it stops working when changing this to use style given in the issue description:

        $build['fields'][$key]['remove'] = [
          '#type' => 'button',
          '#value' => $this->t('Remove'),
          '#ajax' => [
            'url' => Url::fromRoute('entity.search_api_index.remove_field', $route_parameters),
          ],
        ];

An AJAX request to the right URL is sent, but with the wrong token value.

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

I’m not sure I understand the problem. Is it that search results are shown that the user shouldn’t see or is it just a problem in the facet, either a wrong count or the disclosure of restricted content there? Could you please clarify with some screenshots or similar? That would be great. (The steps to reproduce are a bit much.)

In case facets are the problem: There was recently a security release → fixing an Information Disclosure vulnerability → , so maybe your problem is already fixed in the latest version of the module? Which module versions are you using for Search API and Facets?

Looks good, thanks a lot for the suggestion! (And thanks, Joris, for pinging me!)
Merging.

Side note: Seems like clear() and rebuildTracker() might then also be candidates, but don’t want to slow this down. We can always add those later if they seem useful, but might not be necessary in most cases.

Thanks a lot for reporting this problem! I can reproduce it in the latest dev version, too.
As you say, [] is definitely a valid response and we should treat it as such. I’m pretty sure just fixing the method in the Api class should be safe, I cannot imagine any code relying on this faulty behavior.
Unfortunately, your patch does not apply for me, but this merge request should implement a complete solution:

• It fixes Api::sendApiRequest() to not treat a [] response as a failed request.
• It adapts MigrateServerForm to display a helpful message when an account has no apps.
• It adds test coverage for this new behavior.

Please give it a try and tell me whether this works for you.
And thanks again, in any case!

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

Finally green!
Merged.

Does anyone else want to test/review, is there anything still missing, or is this RTBC?

I just tried this new functionality for myself to make sure it works correctly, and unfortunately it seems to have major problems with search servers that use the DB backend. When indexing with multiple workers in parallel I encountered lots of errors like the following:

[warning] SQLSTATE[40001]: Serialization failure: 1213 Deadlock found when trying to get lock; try restarting transaction: INSERT INTO "search_api_db_default_index_aggregated_field" ("item_id", "value") VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array
(
    [:db_insert_placeholder_0] => entity:node/1101:en
    [:db_insert_placeholder_1] => 1754142425
)

I am using MariaDB, it might work better with other DBMSs. Unfortunately, I wasn’t able to reproduce this with just running MariaDB clients in multiple terminals and simulating indexing, so cannot really say specifically what deadlock occurs. In theory, if the tracker works correctly, no worker should ever receive the same item as another and all DB operations should affect different rows, but apparently the DB doesn’t manage to handle this correctly anyways. (Maybe I’m just using the wrong isolation level for this?)

It would be very interesting whether someone else can reproduce this behavior? Or maybe try with other DBMSs?

Anyways, I think even if it would work correctly there might be little benefit in using parallel indexing with the DB backend. Even when there were no errors, indexing was very slow when running multiple workers. So, maybe just hiding the tracker for indexes that use a DB backend might be acceptable? And maybe we should additionally add a small warning to the plugin description saying that it might not work with all backends. (Not sure how helpful that would be, though.)
Having this available for other backends, like Elasticsearch, that currently don’t come with a parallel tracker plugin would still be an improvement, even if it doesn’t work (yet) for the Database backend.

Also, I think this underscores the importance of test coverage, even though I’m still not really sure how we could make that work.

I just tried this new functionality for myself and while it seems to work really well for Solr servers there seem to be major problems with the database backend. When

Thanks, looks great!
I just cleaned up the code a little regarding coding standards. Also, while in theory the clean way to write tests, it is unfortunately rather a waste of resources to just test a single page per test method. I therefore combined all of them into a single method.
Finally, I added tests that actually make sure that the warning message still appears on forms where the entity is in fact modified.

The test failure was unrelated and is now fixed in HEAD, so should be fine after I merged latest HEAD into the fork. The weird failure is our CI’s extremely unhelpful way of saying it failed because of deprecation warnings.

Anyways, if all those changes look fine to you I can merge them.
Thanks again, in any case!

Thanks for your feedback!
Hm, maybe as a first step we can only show the option under certain circumstances – for our own “Excerpt” field and for processor-added fields containing HTML, like the “Rendered HTML output” field.
Adapted the MR accordingly, please test/review and tell me if this still looks good to you.

In any case, we can later still go back to make this option more widely available in case there is support/demand for it.
(Also, advanced users can still enable it for all fields that use the SearchApiText plugin via the config – I specifically included code to always show the option if it is already selected to avoid accidentally resetting it in this case.)

Amazing work, once again, thanks you so much!
All looks good now so I merged the MR.
Thanks a lot again!

Merged.

Implemented in this merge request.

drunken monkey → created an issue.

Merged.

drunken monkey → created an issue.

Implemented in this MR, just have to get the pipelines green.

Thanks a lot for reporting this problem, and sorry it took me a while to get back to you!

Unfortunately you are right that it doesn’t really seem easy to fix this problem ourselves as Core’s autocomplete.js doesn’t provide the required extension points.
It seems extractLastTerm() is called from both searchHandler() (to determine whether the input is long enough to even attempt to autocomplete) and sourceData() (to actually provide the autocomplete suggestions, either from cache or by sending the request).

Since both are actually set on Drupal.autocomplete.options, however, it does seem like it might in fact be possible to override them? So, in theory we could probably provide our own versions of them for our own autocomplete elements and leave the other ones unchanged.
However, if we don’t want to duplicate their code (and manually have to port any changes Core makes to them) we’d then have to set some kind of global variable that this is one of our elements, then call the parent and then also override Drupal.autocomplete.splitValues() globally to change what it does (return single token or call old method) based on that global variable.
Really not ideal, but feasible in theory. However, hopefully one of the Core issues will get fixed and it doesn’t have to come to this. I’d wait for now to see what happens in Core.

Note, also, that (it seems) we would only have to fix splitValues(), as extractLastTerm() just uses that internally. If that method only returns a single token, then that token is also what extractLastTerm() will return.

Anyways, thanks again for reporting!

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

Merged.

Hi, @cbeem,
Thanks for reporting this issue and sorry it took me a while to respond.
Unfortunately, I’m not sure how to help you here. The update works for me and, apparently, almost everyone else as it has been there without problems for over a year. So, there has to be something very specific about your site that interferes.
Unless you are able to provide further information by maybe adding debugging statements or using XDebug I don’t think we’ll be able to find out any more here. And since it finally worked, you are probably not really invested anymore, either?

I see you selected version 1.1 – was that the version you updated from? In that case, maybe the large step from 1.1 to 1.10 is the problem and it would work (reliably) if you’d first update to some intermediate version, run the updates, maybe re-save the affected entities and only then update to 1.10?

Thanks, looks great!
I also saw that the change record explicitly states → that this is fine to do even though we do not depend on Drupal 10.3 yet.
So, merged. Thanks again!

drunken monkey → created an issue.

Hi @dhruv.mittal, and thanks a lot for reporting this issue!
However, I’m afraid I cannot reproduce this problem. I tried writing a regression test, but everything I tried still passed without your fix. (Also, I think it wouldn’t be correct to replace non-breaking spaces with normal ones, as your suggested code would do. I think if we use the u modifier we’d have to explicitly list the “normal” whitespace to replace to prevent this. Also just for the sake of backwards compatibility, in case this breaks anyone’s code. The resulting string should then still be valid UTF-8. See 63042a3c.)

Could you maybe add a failing test case to HtmlFilterTest::testBasicFieldValuesProcessing() and then revert f10c2ad7 to demonstrate that your fix actually works.

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

Cleaned up the rest of Index and IndexInterface and then just merged this as a first step.

Judging by git grep -lE '@(param|var|return) (array( |$)|.*\[\])', there are now still 178 files with … 623 (potentially) suboptimal type hints left. I think form or render arrays can probably keep the simple array type hint, but probably there’s no other place where we cannot improve this (or TYPE[]) to array<KEY_TYPE, TYPE> or list<TYPE>.

So, maybe child issues for separate batches of these files, or maybe based on parent directory? What would be a good batch size?
I don’t want to divide this too finely and just create a huge flood of contrib credits, but it is tedious work and I don’t think anyone is willing to create or review a single MR for 50 files of this.

Great, CI agrees. Merged.

OK, seems it had nothing to do with discovery, actually, it’s just that the display_types property didn’t correctly default to ['normal'] in the tests for some reason, even though it works fine on the actual site.
Anyways, should be fixed in this MR.

drunken monkey → created an issue.

Merged. Thanks again!

@greggles: Thanks, it’s good to know that this at least won’t cause a security advisory.

Currently, I’m still leaning towards option 2, however, of removing the setting from the UI and just mentioning it in README.md (or maybe, maybe, in the form element description). But would be great to still hear from @sonfd what he thinks of that.

Merged. Thanks again!

@kristiaanvandeneynde: Thanks, nice job! Especially great that you found the problem with SearchApiDbUpdate8102Test, I think that would have utterly confounded me.
The tests being marked as failed is (probably) due to the deprecation, which we already fixed in HEAD. Merging in latest changes, the pipeline should be green now.

@kristiaanvandeneynde: Thanks for catching that, you’re right.
Should be fixed now.

Once again, thanks a lot for your work, looks great! I commented (a lot) on the MR but I think everything is resolved now.
If the pipelines are green and you are happy with my changes then I think we can merge.

(I was not entirely sure whether we still needed the unit test if the kernel test already covers the exact same thing but probably still good to have something that executes more quickly. In any case, the resource cost is negligible, and I just reused the same data provider method to avoid the duplicate code (in case we ever need to add additional test cases).

drunken monkey → created an issue.

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

Pipelines are green, so merged. Keeping open again in case something else comes up.

Tests are there and working, and they even found a rather large problem with the change.
Should all be good now, just need to get the pipelines green again.

drunken monkey → created an issue.

Merged, but leaving open for tests.

Implemented in this MR.

drunken monkey → created an issue.

Merged, will test with the dev snapshot.

Implemented in this MR.

Update: Token Authentication doesn’t seem to be to blame after all. Instead, it seems people are hitting SearchStax’s relatively low request size limit, which activates a fallback mechanism in the Solr backend plugin which sends the documents one at a time instead.

While affected users can just reduce their configured indexing batch size to work around this problem, I’ll investigate whether we can also provide a fix in the module code.

Merged.

TIL: Switching a plugin class from annotation to attribute discovery comes with additional dependency checking in \Drupal\Component\Plugin\Discovery\AttributeClassDiscovery::getDefinitions(). If you forgot to declare a module dependency for some test plugin, your test will fail in a way that is incredibly hard to debug.

Let’s see if pipelines pass now …

Now also running into this change → …