🇦🇹Austria @drunken monkey

drunken monkey → created an issue.

From the description of the latter:

Documentation for unsupported modules and modules only compatible with Drupal 6 or earlier should be deleted.

To me, that seems like modules that are still supported should not be moved there, even their Drupal 7 versions. (Especially, our Drupal 7 branch is not wholly unmaintained, we would still address any security issues or critical bugs.)

Thanks for your comments!
I can see your reasoning, especially if you only ever used one type of token here, but I can easily imagine other users mixing token types so we should support that, too. While potentially annoying, performance in the admin UI isn’t a primary concern.

Would you say this is RTBC then?

(The regression test doesn’t really work: Since the requirements all succeed, the deprecated constants aren’t actually used on the Status Report page.)

I think the pipelines should pass now except for “next minor” which currently fails in HEAD as well, see 🐛 Fix RenderedItemTest::testAddFieldValues() which fails against Drupal 11.3.x Active .
Let’s see.

drunken monkey → created an issue.

Was quite a bit of work to add proper DI and get everything properly organized but I think this is now ready to be reviewed.

drunken monkey → created an issue.

Merged.

This should probably wait for 📌 Switch to OOP hook implementations Active .

drunken monkey → created an issue.

Added test coverage, but then actually discovered 🐛 Bug when processor defines both datasource-independent and datasource-specific properties Active which will keep the pipelines from passing.
Anyways, testing/reviewing still welcome!

drunken monkey → created an issue.

drunken monkey → created an issue.

drunken monkey → created an issue.

Working on the test coverage.

Makes sense and looks good, thanks a lot!
Merged.

Thanks for reporting this problem!
However, for me \Drupal\views\Plugin\views\argument\ArgumentPluginBase::unpackArgumentValue() is unchanged. Did you maybe apply some MR or custom patch? Otherwise, which version are you using – ideally, could you tell me the Git commit hash?

@ifrik, @ressa: Great job, thanks a lot for your work on this!

Minor style/punctuation changes.

Thanks for reporting this issue!
However, this module relies on Drupal’s built-in autocomplete functionality for all frontend code (except for a few specific adaptions), so I’m pretty sure this will also be broken for other Drupal autocomplete elements and should therefore be fixed in Drupal core.

Please try this with some autocomplete field from Core (e.g., when adding tags to a node) and see if it works better there. Otherwise, please move the issue to the Core issue queue accordingly.

Thanks a lot for the suggestions, makes sense!
Should now all be fixed except for the last item, not sure about that one. I just unlinked the D7 docs from the Search API project page now, that’s probably a good first step. I don’t really know what the proper process is for those older versions of the documentation. Seems d.o should archive those at some point on their own accord.

OK, DI is complete and pipelines should be passing. Handing off for review.

One thing, now that we’re creating a new class anyways: Would you say IndexingBatchHelper is a better name than IndexBatchHelper? The latter sounds a bit awkward to me, or maybe more like it handles some operation on an index entity, not an indexing operation.
Also, having a slightly different name might help in distinguishing the old and the new class. (Other naming suggestions also welcome.)

Thanks for posting this suggestion!
I don’t quite understand the whole reasoning, but in principle this should always have been a service, just like the other helper services in the Utility namespace. So, yes, let’s do it.

However, just making this a service without any changes doesn’t make much sense to me. Definitely, all those methods should be instance methods, not static.
I rewrote the MR in that direction and added a change record → . Please tell me what you think.

Still needs a bit of work to get rid of static \Drupal::…() calls in the service class, but ran out of time today. Feel free to tackle it if you’re able, otherwise I’ll get to it tomorrow.

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

Thanks for reporting this problem!
I assume this happens every time you try it? If so, could you please add some debugging code to \Drupal\searchstax\Service\Api::sendApiRequest() to retrieve the $json_body sent to the SearchStax server? (Or, even better, get it from the Guzzle client directly to exclude even more potential sources of problems. The error message very much sounds like invalid JSON is being sent but that would be very strange indeed.

Thanks for reporting this problem!
Is it possible that you have a problem similar to ✨ Do not delete items when loading fails Fixed ? If so, you could try setting the hidden delete_on_fail index option to false and see if this resolves the problem. Just add the following line to your index config and import it:

options:
  delete_on_fail: false    # <-- add this
  …

Your issue description sounds like indexing might work correctly but the item later gets deleted again from Solr. So this setting might help – in that case, you should probably investigate why loading of the items sometimes fails.
Otherwise, are there other systems that might delete documents from Solr?

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.

“Drupal 9” is not 100% current anymore.

Thanks for posting this issue!

However, this was already discussed in 🌱 Add type hints to all Search API interfaces/classes for Drupal 9 Active , please feel free to add your example code there. However, I don’t think just adding these type hints without issuing deprecation warnings first is compatible with our BC policy as this would break a lot of contrib modules and custom code.
It seems like adding parameter type hints should be OK (maybe even in a minor version increment, but probably safer to also have a major version increase for this), but adding return type hints to interfaces or parent classes immediately causes fatal errors for all unadapted child classes.
So, probably something like #[\ReturnTypeWillChange] will be needed, but in any case we need a good, clear plan for this and lots of time as a buffer for contrib/custom code to be adapted. But let’s just discuss this in the other issue. Thanks!

I think this should be fixed with the MR in 🐛 Accounts with no Apps break migrations Active , please see there. If that MR works for you, I can merge it. (The same bug seems to break both migration and version check for accounts with no apps.)

Thanks for posting this suggestion, does seem to make sense.
However, since Drupal 7 has reached EOL there will not be any more updates to this module’s D7 version unless to fix critical bugs or security vulnerabilities.

Thanks for posting this suggestion!
However, wouldn’t an “Aggregated field” already work for this use case?

Thanks for reporting this issue!
However, I have to admit I’m a bit stumped by this. No idea why this wouldn’t be working, I think we use a very straight-forward way of rendering the items, nothing that should interfere here. However, I have almost zero expertise regarding theming, so there might still be a bug in there. You can check out the code yourself in \Drupal\search_api\Plugin\search_api\processor\RenderedItem::addFieldValues() but it’s basically this:

$build = $this->getEntityTypeManager()->getViewBuilder($this->getEntityTypeId())->view($entity, $view_mode, $entity->language()->getId());
$value = $this->getRenderer()->renderInIsolation($build);

However, I now see one potential source of problems: To keep indexing consistent we switch to the default theme for this whole operation, so if you created your template for a different theme then it won’t be used. Could that be the issue here?

Thanks for reporting this problem, @gxleano, and thanks @ishani patel for providing the MR.
Seems simple enough, and it’s clear how this would happen. We can probably also get away with not providing a regression test for this.
So, merged. Thanks again!

Please tell me if this addresses your concerns and we can close this (maybe as a fixed support request) or whether you still think this needs to be changed.

I think this was specifically changed back in 🐛 Problems when executing Search API tasks during install, updates Fixed since people reported problems when tasks are executed during site install. So, I think this is expected behavior.

Thanks a lot for reporting this problem and providing a fix!
Seems quite a few people are running into this, so sorry it took me a bit to get back to you.
Anyways, just made a tiny change regarding code style (I prefer $value === NULL over is_null($value)) and then merged.
Thanks again!

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

Thanks for posting this suggestion!
What do you think about this?

I guess we didn’t initially add documentation for those events as a) they are dynamic and b) should not be used like regular events. But I guess for the sake of completeness, and to help people find information in case they see these events and have no idea what to make of them, adding some documentation can’t hurt.

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

Sounds like it might be useful, sure. Thanks for the suggestion!
However, I generally avoid adding features that might be too niche so I’ll wait until a few more people interest in this. You can already work on it if you want but would then of course run the risk that it might not get merged after all.

Great idea, thanks! Didn’t realize it was that easy to get a token browser dialog …
Anyways, I think we can improve on the types shown, but otherwise this looks great. Please check out my revisions and tell me what you think – if they look good to you I’ll merge.

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

Great, thanks again!
Merged.

I’ll look at the other issue soon, too.

Thanks, that’s a great suggestion! The code also already looks quite good. I just made a few minor adjustments, among them:

• I think we can do without $this->addingFieldValues by just passing FALSE to $itemn->getFields().
• To make property path collisions less likely, maybe we should just prefix the property path with search_api_ like we do for some other properties? Not sure why we don’t do it for all of them, does seem like a good idea in general to avoid problems, but probably too much of a BC hassle to change that now for the existing, datasource-independent property.

Also, before I can merge this it does also need test coverage. Would you mind adding that? I think it should be relatively simple to add this to the existing CustomValueTest.

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

Thanks for opening this discussion, and thanks to @mxr576 for pointing us to it during his BoF at DrupalCon Vienna.

@alvar0hurtad0 makes a very good point: Even if solely relying on usage counts was a good option, a system where just 14-16 modules actually get a multiplicator seems pretty pointless, discounting large swaths of modules that are very widely used. Drupal itself has just 685k reported installs at this point so this only boosts modules that are installed on more than 30% of sites (and all but five of them with just a factor of 2). For instance, 18 more modules are used currently by at least 20% of sites (137k).

And yes, I agree, ideally this would focus on a lot more than just usage counts, especially since they are so unreliable: how complex the module is, how well-maintained, how critical it is to Drupal as a whole or to a typical site that uses it, how large the sites that use it typically are, etc.
And then we get to the fact that the work required for solving a single issue also varies vastly, from (important) one-line fixes to complex new features, so getting ten contribution credited for a small module might still be much easier than working on a larger issue in one of the most important modules, let alone in Core.

So, I think we can agree that this system is far from perfect, but a lot of the categories that one might feel should influence a project’s weight are very hard to quantify objectively.

And it’s very unclear to me how we would move forward here. Would there be a clear path forward for adapting this system, at least slowly?
If, for instance, we could at least agree on a tweak to how usage counts affect a project’s weight? That at least seems to be the easiest to quantify.

Just merged 🐛 Fix hidden bug when entity properties are updated via ConfigEntityBase::set() Needs review and the pipeline is passing now.
So, merged. Thanks again for your input on this, Joris!

Looks great, thanks!
I just added some test coverage and then merged. Thanks again!

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

Merged.

I decided to just go ahead with the current solution. As stated in #14, we can always go back and extend the functionality later.
So, merged. Thanks again, everyone!

Merged.

Great, thanks a lot!
I just made minimal changes and then merged.
Thanks again!

See above: My first stop would be the Solr server logs, but it’s very hard to give any general advice on this.

Merged to HEAD, being tested.

drunken monkey → created an issue.

Does anyone still want to test or review this MR?

Good idea to add a README.md file, thanks! I expanded it slightly and amended the code style.
Also, you’re right, this might currently be not discoverable enough for people unfamiliar with the Search API ecosystem. In your MR, I’m now changing the settings form to always display the “Auto-suggest core” field but disable it if the Search API Autocomplete module is not installed (along with an explanation to install it).
What do you say, do these changes make sense to you?

Thanks a lot for proposing this new feature! This does sound potentially useful, and simple enough to implement, so we can definitely go ahead with this.

However, I do have a few remarks:

• I don’t think using (and, therefore, moving) splitIntoWords() is necessary here. There is in fact already a $words defined in SearchApiFulltext::validateExposed(), a few lines further down. Just moving that up should be enough.
  The DB backend will only use splitIntoWords() if the “Tokenizer” processor is not active, which is a bad idea anyways, so a the simple preg_split() already used in the method should be fine. Just move the definition of $words up a few rows.
• Also, I think the new check should come after the Unicode::validateUtf8() check, it’s too early in the method currently. Right before // Only continue if there is a minimum word length set. seems good.
• I don’t think an update hook is needed, though. The defineOptions() change should be enough to keep existing views from breaking.
• Finally, some test coverage would be nice. We don’t always add this for Views functionality, but it seems easy enough in this case to add it to ViewsTest::testSearchView() – just adapt the used view accordingly and then add a request with too many keywords.

In any case, thanks a lot again!

Thanks for reporting this problem.
However, since Drupal 7 has now reached EOL there won’t be any more bug fixes, let alone feature requests, added to the Drupal 7 version of this module, except in case of security vulnerabilities or other critical bugs.

The issue I have with node grants, and why Group does not support it, is that it only works for nodes.

Ah, of course, that makes sense. No way around that, then, of course.

@drunken monkey, is it then safe to assume that search_api does not leverage query access for entity types other than nodes?

Yes, exactly. The Search API is entirely independent of the entity query system, one reason for this being that it also supports indexing items that aren’t entities. (I have never given this much thought and the idea of integrating Search API with the entity query system does sound intriguing, but I don‘t think it would really work well enough. And, either way, it would probably be a huge effort at this point.)

One way around this that I can think of is to grab the resulting IDs from the index, run an entity query with access checks turned on and then use the intersection to show results and counts. Not sure how easy that would be to pull off, though.

That would be an idea, but unfortunately I think there are too many problems that this would cause: To make paging work properly, the search query would have to retrieve all result IDs, not just the requested page, and we would have to do paging after the entity query has run. Since there could be thousands of results I don’t think this would be viable, especially since Search API results often don’t just include the ID and other short information, but highlighting or even the whole field data of the entity.
And even apart from that, this would still not do anything to fix facet counts, which would still not take Group access restrictions into account.

So, I think a dedicated processor to make Search API work with Group access restrictions in particular is really the only viable way forward.
We just have to decide which module this should live in, and add test coverage.

Regarding which module to add this to: The Search API specifics the MR relies on are very stable so I think this should be very low maintenance from the Search API side. So, adding this to the Group module should be fine, and if you ping me I’d be happy to help with any issues that arise due to changes in the Search API. (Or in case of other bugs that seem like they might relate to the Search API part of the code.) In any case, test coverage is essential so we are quickly alerted to any breaking changes.
On the other hand, if you prefer it that way and you likewise think it unlikely that the Group module’s API used in this code will change we could also add it to the Search API, and I would contact you in case of any bugs.

Will you be in Vienna next week? If helpful, we could also discuss this there.

That’s great to hear, thanks for reporting back!
Can someone else confirm, can this issue be closed?

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

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.