🇦🇹Austria @drunken monkey

As a contrib maintainer, I’d have appreciated some heads-up before this is changed again and I just find out when merging an MR the next time. Especially since I’ve now already adapted my custom tooling to work with the old new format and am now supposed to change it again (or spend extra time editing the commit message by hand every single time I merge an MR).
A note about this change on the Contribution Record screen, maybe above the formatted commit message, would have also already helped so I don’t have to hunt around various d.o issue queue to find the correct ticket for the change.

Is the format (at least of the first line) now considered stable or should I wait a bit before adopting the new format?
And is there a way of following such changes affecting all issues and MRs without following all Drupal Core issues, maybe a Slack channel?

Some more deprecations and PHPStan errors to fix, too.

Thanks for reporting this issue, @baernhardj! However, I do need one bit of clarification:

Create a node with the access role authenticated and publish it

As far as I’m aware, this is not standard functionality. Normally, you don’t have the option of setting an “access role” when you create a node. Can you tell me which module you’re using for that?

Are there any others with good experiences using MR 154? @abramm?

I had one comment, otherwise this looks great, thanks!

I think this still needs a small edit to make sure lines we touch here are correct afterwards.
However, since the typo was there before, you might also argue that it is unrelated, in which case I would also consider this RTBC.
Thanks for opening in any case!

Thanks for sharing!
Can we mark this as fixed or did you want something investigated? In the latter case, please reopen with more detailed steps to reproduce, or maybe clarifying screenshots.

Thanks for your comments, I now replied.
Will you continue working on this? Otherwise, feel free to hand off to me at any time. I’m just not familiar with the Key module so would need a thorough review for my code afterwards.

Ah, you’re right, thanks for figuring that out! No idea, how it happened, though. Guess I’ll just have to pay closer attention in the future.
Unfortunately, it doesn’t seem to be possible to change the source branch of a merge request, so we’re stuck with this. Sorry.

Anyways, thanks a lot for your help and feedback, this is now merged.
Thanks again, everyone!

Would have been great to get feedback, but I think I’m still happy enough with the code to merge.

@eric_a: Seems we’re currently pretty undecided on that, so seems like a good thing for which to put a standard in place. And probably to use the one from FIG, then. (Even though I personally prefer it with the space.)

$ git ls | grep '\.php$' | xargs grep -hPo '\bfn ?\(' | sort | uniq -c
    108 fn (
     85 fn(

drunken monkey → created an issue.

Thanks for reporting!
I rebased the MR, please try again.

Yeah, then it should probably be safe to ignore that aspect for now. (Probably, uninstalling the Key module would break other modules/functionality as well.)
Thanks!

I created a merge request for adapting the associated autocomplete search when switching a search view to a new index. Seems like this isn’t your problem anymore, but should save some headache for other users. Needing to delete the autocomplete configuration before being migrating is a) hard to figure out and b) unnecessarily cumbersome. Seems like it would be pretty simple for us to take care of that. (Unfortunately, I think there will be other modules affected, too, with similar problems, and I don’t think there is a good solution except fixing them one by one.)

Your current problem seems like something else entirely, not actually related to the migration module.
For testing purposes, can you try editing your Drupal search server and using a read-only token for the “Read & write token key” setting? You can’t use that permanently, since indexing won’t work, but does it fix the autocomplete functionality? (It seems like the /emsuggest endpoint that SearchStax provides actually only works with read-only tokens, so that would be something we’d need to fix, too.)

Thanks a lot for trying it out, good to hear it worked, and also for reviewing the MR so thoroughly!
I hope I could answer all your questions there, please tell me if you now think the update function’s behavior makes sense or if you would like to change it.

Should now all be fixed and I also added an update test.
Please test/review.

And yes, very strange that the MR is not linked. It’s also not linked from the contribution record. Seems like something went wrong there.
Anyways, should not really hamper us.

I added the update hook and adapted the existing test. Not sure if any more test coverage is really needed – potentially for the update hook, but that’s not very complex (and a pain to test).
In any case, feel free to already give this a try or review my code.

I have something working now in this draft MR which can already be tested in case anyone is interested.
It’s still missing the update hook and test coverage (probably also test adaptions, so I expect failures there), but the basic functionality should be there. Just make sure to manually set the new auto-suggest core setting on all servers, then you should be able to use autocomplete with all of them.
There is also a change record → , feedback on that would be also welcome.

I should be able to complete the missing pieces by the end of the week, or possibly already tomorrow – but thought I’d share what I have right away in case someone has feedback on the basic direction.

Thanks for reporting this problem!
However, (1) should actually never be the case (unless there is a very long word/token), and for (2) I’d need an example – it might be on purpose, but that’s hard to tell with this little information.
Would you be able to add failing tests for this to either HighlightTest (or HighlightExcerptTestif that’s easier)?

Thanks, looks good to me!

Thanks!

Great, thanks for the feedback!
Merged.
Thanks again!

Sorry for the long delay. Does this problem still occur for you?
This should have been fixed on the SearchStax side.

Thank you, that would be great!
We should probably also have additional test coverage for the different update paths – we do have one that tests activating the Key module after this one, but what about the other way around? Or, most tricky to test, having both installed and then running the update function to move our configuration to the Key module.
I can take care of those tests, too, though, if you prefer.

Also, now that I think about it: Do (or should) we have some hook that moves the configuration back into this module if the Key module is uninstalled? Or is that too uncommon a use case?
Currently, it seems like this would break the SearchStax functionality, correct?

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

Thanks for reporting this problem!
I’ll try to reproduce locally and see what the issue is or how to best fix it.

Discussed with John directly, I’m taking over. I hope to have something early next week.

Ah, right, sorry, that’s GET parameters, not the body. Then even stranger …
I forwarded this to the internal SearchStax team for input, I don’t really know what to make of this myself.
Sorry for the delay.

🐛 Bug when processor defines both datasource-independent and datasource-specific properties Active is being merged, so tests should pass after that.

Any feedback on my changes, does this look good to you?

Any feedback on my suggested MR?

Merged.
Thanks a lot again for your help on this!

Great to hear, thanks!
Merged.
Thanks a lot again for this nice improvement!

Yes, some clear instructions both for users and contrib maintainers would really be great.

Excellent, thanks a lot!
Merged.
Thanks again!

Merged the MR, testing will happen in dev version.

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.