🇦🇹Austria @drunken monkey

Looks great, thanks!
I think we don’t even need the empty() as the variable is always set, but otherwise looks perfect.
Merged. Thanks again!

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

Not sure how to get rid of those last warnings in the “max PHP” PHPUnit run (caused by Functional tests that print output for deprecations which doesn’t seem to adhere to the “ignore deprecations” file), but it’s definitely an improvement. So, merged.

Merged.
Should now be fixed (i.e., a warning should be displayed on the “SearchStax settings” page, if applicable) in the dev snapshot of the module and the fix will also be included in the upcoming 1.7.2 (or 1.8.0) release.

Merged.
Should now be fixed in the dev snapshot of the module and the fix will also be included in the upcoming 1.7.2 (or 1.8.0) release.

Thanks for reopening!
The error message is a bit misleading, but the root cause of the error seems to be that you have the current page’s language, en-us, is not one of the languages configured in your SearchStax app – as, indeed, it is not a language code supported by SearchStax in general. (It seems the supported language codes would be en, en_ca, en_hk and en_gb.)
We’ll look into how to best resolve this issue for you.

Should be fixed in this MR.

drunken monkey → created an issue.

drunken monkey → created an issue.

drunken monkey → created an issue. See original summary → .

drunken monkey → created an issue.

Sorry, I know there was a long delay since you created this, but would be great if you could still test/review before I merge it.

Thanks, that’s very helpful! Though, unfortunately, those steps didn’t work for me, at least not reliably. In most times, things still worked fine, just want I got an error, but then it broke the whole site with a WSoD until I cleared the cache again:

Error: Call to a member function getContextDefinition() on null in Drupal\views\Plugin\views\argument\ArgumentPluginBase->getContextDefinition() (line 1443 of core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php).

Useful insofar as the argument validator plugin should have nothing to do with the Search API, which would point towards this being a problem in Views in general. However, with the unconditional synchronous GET request at the top of _search_api_views_get_handlers() you actually have some kind of infinite loop, I guess? So, not sure how this ever even works partly, to be honest. And, not sure whether that’s a good indicator of the actual problems caused by an ill-timed website visit.
I amended the debugging code to this:

function _search_api_views_get_handlers(FieldInterface $field) {
  if (empty($_GET['skip_debug'])) {
    \Drupal::httpClient()->get('http://drupal.localhost/search/content?keys=macto&skip_debug=true');
  }

But then it promptly failed to produce any negative effects whatsoever. Sorry, not sure what’s different between our setups.

Are you able to reproduce something like this for a Core view (i.e., without any Search API plugins, or Search API not even installed), too?
That would be very interesting. As it is, this is still rather tricky to debug.

CI is happy, so merged.

Thanks, that’s a great start!
However, just looking at Index and IndexInterface I saw a bunch of more places where types were too unspecific (and even found a bug, 🐛 Fix Index::setProcessors() to not assume $processors is keyed by plugin ID Active , though that‘s beside the point). Not really sure how we can get close to a complete MR for this. Seems like a ton of work, and very hard to review and validate. But just doing this piecemeal is also a bit unsatisfactory (and hard to lose track).

Suggestions are welcome.

Should be fixed in this MR.

drunken monkey → created an issue.

Great, thanks a lot!
I also removed the now outdated ignoreErrors directive from phpstan.neon.
Merged.

Perfect, thanks!
Merged.

No worries, thanks for reporting back. And good to hear it works for you, too.
Merged.
Thanks again, everyone!

Great to hear, thanks for reporting back!
Merged.

drunken monkey → created an issue.

Looks great, thanks! Merged.

drunken monkey → created an issue.

Merged.

Implemented in this MR.

drunken monkey → created an issue.

Merged to the dev version, will re-open in case something is still not working.

Should be fixed in this merge request.

Thanks for posting this issue already with a working MR. Seems like a sensible feature, you’re right, especially since there is no UI so there is only a tiny change.
I just changed the key to something I think works a bit better and added a note to the README.md file. Please review and tell me what you think.

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

drunken monkey → created an issue.

If someone could confirm that the latest version of the MR works for them then I could merge this.

The “Needs review” status is just for issues that have a working MR or patch.

Would be great to get just a quick confirmation that the code in the MR (patch here) works.

@scott_euser: Thanks for the feedback, good to hear! And yes, sorry, it was @vegardjo who made that first fix, not you. Thanks for pointing it out.
Merged my new MR.
If someone still wants to argue for the sub-module, please open a new issue with additional information as explained.

You’re probably right, no need to do both at the same time. Thanks!
Merged.

Merged the MR and will re-open in case there are any issues.

We do depend on Drupal 10.3 at this point.

We do depend on Drupal 8.6 at this point.

drunken monkey → created an issue.

@hfernandes: I did read those comments, but they don’t answer my question: Are there any actual problems for users? You just state that “this is problematic” without explaining why.
This module provides a bunch of classes integrating with specific modules we do not depend upon. I do not want to create a new submodule for each of them, and haven’t so far. (To be fair, not sure if any of the others actually extend a class that would be unavailable.) In the case of the taxonomy-related Views plugins, there were actual problems so we had to work around them, see search_api_views_plugins_argument_alter() and search_api_views_plugins_filter_alter(). Maybe we could do something like this for the REST data row style, too, if necessary. However, it currently seems like we really need to just add that single check (which does make immediate and perfect sense, of course) and we’re fine, so I am loathe to add an entire new submodule just on the basis of “this is problematic”.

@scott_euser: Yes, if you want to get this resolved quickly then a new MR with just this one change seems like the better option.
I’ve created a new MR with just your initial commit. Please ping me on Slack if you’re happy with that (it seems people were happy with it originally, too, so no further tests/reviews should be necessary) and I’ll merge it right away.
We can then leave this issue open to complete the discussion regarding a submodule or other additional measures.

The linked issue was fixed, so is this still relevant to you?
Otherwise, please close.

Thanks for the feedback, good to hear!
I think I’ll still leave it open for a few days more before merging, just to be on the safe side.

I also now stumbled over #3099471: Deprecate "hidden" definition key in favor of "no_ui" → , which I had forgotten but makes sense – all other of our plugin types now have a $no_ui property, but processors only have $hidden, which we now had to make explicit. (Up to now we had silently accepted both.)
The good thing about this move is that people will have to manually (or semi-manually) switch from annotations to attributes anyways, so now would probably be a perfect moment to switch to $no_ui, if we want. Or to add both but deprecate $hidden right away.
Not sure whether the additional consistency is worth the hassle, though. Any opinions on this?

Thanks for reporting this problem and sorry it took me so long to get back to you.

Unfortunately, what you’re describing is not really a problem in this module, as the forms that deal with the server entities are actually provided by the Search API module. However, the problems with config overrides go much beyond that: while we already fixed some of the more serious problems for Search API in #2682369: Fix problems with overridden config entities → , there are pretty complex problems with the whole concept for any kind of config entity. See the Core issues 🐛 Inconsistencies when updating overridden config entities Needs work and 🐛 Prevent saving config entities when configuration overrides are applied Needs work which try to address these problems for Drupal in general. As you can see, they have been opened in 2016/2017 – the problem is just very, very tricky to resolve in a way that does not lead to new problems in edge cases. (I also wrote a blog post about the various problems with config entity overrides back then, in case you’re interested in a more thorough explanation.)

All that is to say that we can’t really fix these problems in this module, your best bet is to follow (and potentially help) those Core issues, especially 🐛 Prevent saving config entities when configuration overrides are applied Needs work (as the other one has been stalling for years).

The difference between the search server settings form and the “Performance” page in your screenshot is that the former handles a config entity, which is a lot more complex than a form for a simple config object as in the latter case. So, for normal config objects, Core already includes a pretty good solution to at least warn users about what’s happening.
Which brings me to the one thing we can actually do in this issue, for which I have just created an MR: fix the “SearchStax settings” page (/admin/config/search/searchstax) to display the same warning as the “Performance” page for any overridden values on that page. (Which is what the issue title actually sounded like to me.)

Linking 🐛 Prevent saving config entities when configuration overrides are applied Needs work , which I think would solve the same problem in a more radical, but therefore probably simpler way: By preventing overridden config entities from being updated at all.

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

Thanks for reporting, and sorry it took me so long to get back to you!
I could reproduce the problem (after a few tries) and can now see where this is coming from. Should be fixed in this merge request, along with another problem with this form (which can appear when deleting migrated servers/indexes).

Please give it a try!

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

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

Makes sense and looks good, thanks!
Merged.

Feel free to open a separate issue for increasing the default limit. You’re right, 10 does seem pretty low, though 500 seems a little high. Maybe we can get some input from others and land on a good compromise.
I think my thinking was probably to pick a low number so that it won’t lead to any errors even when the checks take a bit longer each, because users running into problems because of the low value will usually have a larger site and know more what they’re doing, hopefully. But if we can find a number that is higher but still pretty safe, that would of course be better.

drunken monkey → created an issue.

drunken monkey → created an issue.

Thanks a lot, looks great and makes sense.
Merged.
Thanks again!

Thanks for reporting this issue.
However, I see this a bit differently. To me, the ellipses not only suggest that text was actually truncated, but also that the field value from which the snippet was taken is not the only text on the entire entity. Which is nearly always the case.
I’m open to hearing others’ opinions on this, though, I certainly don’t have a strong opinion either way. Also, if you think it’s not that uncommon to have just a single text value for a result item then we can certainly think about removing the ellipses (if appropriate) in this case – at least if it doesn’t complicate the code too much.

This works fine for me. Are you sure you can reproduce this on a clean installation of Drupal?
Otherwise, it seems rather like your theme or some other site-specific code gets in the way. We mostly just use Drupal’s own autocomplete functionality and I don’t think the adaptions we do make are likely to break keyboard navigation.

That being said, it is of course possible we could make some improvements to make it easier for themes to keep this functionality working, but I’d need details for that. (I’m also not very good with frontend code in general, so always open for suggestions from frontend experts.)

Makes sense, thanks!
Merged.

Thanks for suggesting this change, makes sense.
However, wouldn’t it make more sense to default to the default cron limit? Though I guess that might also be 0, so there needs to be a last fallback of 50 regardless. Anyways, changed the code to try the default cron limit first.
Please test/review and tell me what you think!

(Note: Seems that, annoyingly, both user experience and UX are commonly used as tags, so changing to use both here. (The latter is significantly more common, though.))

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

No, the two indexes should be entirely separate, no idea what’s going wrong there.
If you still have the problem, can you maybe elaborate a bit more and maybe provide screenshots for what you are doing?
Asking on Slack → is also often helpful for such problems.

Thanks a lot for reporting this problem and already providing fixes!
However, I’m not sure I fully understand why the new module is needed? It seemed vegardjo’s initial fix was already working fine.

Are there any actual problems when just applying the small fix to search_api_views_plugins_row_alter() and the REST module is not installed? Could I have steps to reproduce for those?

📌 Add attribute discovery support for our plugin types Active would add attribute discovery support for all our plugin types. The change should be fully backwards-compatible, but it would still be great to have this confirmed by a few others who are implementing Search API plugins before merging.

Opened an MR with the code from 🐛 Fix failing pipelines Active .

Thanks for reporting this problem and providing a patch.
Creating an MR so the test bot can take a look.

From the code, I’m not sure how this would ever happen, though? I guess the backend’s indexItems() method returns item IDs as indexed that were not even passed to it in $items? That seems like a potential bug, or at least very hacky solution, in and of itself – but still, we should of course handle that reasonably.

Pipelines finally passed again (hard to revert just the correct subset of changes if so many files are affected). Merged.
Thanks again, everyone!

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

Pretty sure this is acceptable, yes, see here → . There is even the base class, so most likely there will be absolutely zero impact for module developers.

Pipelines are finally passing on the MR.

Gave it some thought, though, and I will really split the attribute discovery (for our own plugin types) into a separate issue – created 📌 Add attribute discovery support for our plugin types Active for that.
I will remove those changes from the MR, and if pipelines then pass I’ll merge and we can finally have CI again in this module!

Tried to fix the deprecations and, in the process of this, added attribute discovery for all remaining plugin types. Wasn’t quite as cumbersome as I feared, but still a bit of work.
Unfortunately, due to 🐛 Deprecation warning in DefaultPluginManager::__construct() is too unspecific Active , I then still needed to ignore the deprecation message since the Search API Autocomplete module doesn’t support attribute discovery yet.

Not sure yet whether I really want to merge attribute discovery as part of this, it does seem like a distruptive change that might easily break stuff downstream. Maybe I’ll extract this part into a different issue after all.

drunken monkey → created an issue.

Added an MR that adds the method to the interface and which includes a test demonstrating that mocking transaction is currently broken.

I opened an MR with the trivial code change.

drunken monkey → created an issue.

Doesn't sound fun! Couldn't see anything at a glance causing the remaining fails :(

I asked on Slack and nicxvan → says it’s likely to be deprecations. In which case I think they changed the test/CI output in the last month to make this more difficult to spot, since before I definitely had it on the radar that deprecations still needed to be addressed.
Anyways, working on it.

Created ✨ Allow explicit COMMIT in Transaction objects Active to hopefully fix the purge() problem in Core. But we’ll of course need the workaround in any case, at least for now – I can’t imagine the Core issue will get fixed that quickly.

So, does anyone here have any clue why those two phpunit jobs still fail even though all of their tests pass?

drunken monkey → created an issue.

The fatal error in BasicTrackerTest::testExceptionHandling() with Drupal 11.3 was a real doozy, but I figured it out eventually:

After the test finishes (and only then), all mock objects are destroyed, which in the case of $transaction calls the __destruct() method. It seems PHPUnit does not (and will not – I tried) mock that method, so this calls the actual \Drupal\Core\Database\Transaction::__destruct() implementation, which got changed in ✨ Allow explicit COMMIT in Transaction objects Active to call purge() instead of unpile():

  public function __destruct() {
    $this->connection->transactionManager()->purge($this->name, $this->id);
  }

$this->connection in this case is our mock, and since we didn’t mock transactionManager() it seems PHPUnit will just supply any valid implementation, i.e., it creates a mock method that just returns a mock object implementing \Drupal\Core\Database\Transaction\TransactionManagerInterface, as specified in the method’s contract (i.e. return type hint).
The problem with that is that TransactionManagerInterface doesn‘t actually contain the purge() method, that method was only added to the TransactionManagerBase class for some reason. So, when purge() is called on that new mock object, it leads to a fatal error.

What then cost me another two hours to figure out is why just mocking the $connection->transactionManager() and returning a TransactionManagerBase mock object doesn’t fix this: PHPUnit (for some reason) actually resets the invocation handlers of all mock objects after the test has finished (see \PHPUnit\Framework\TestCase::verifyMockObjects() and \PHPUnit\Framework\TestCase::shouldInvocationMockerBeReset()), so the connection object of $transaction at the time its destructor is called has none of our custom method mocks and will still fall back to the default implementation for transactionManager() as described above.
I found two possible solutions to this: first, it seems calling $this->setDependencyInput([$connection]); in the test method keeps PHPUnit from unsetting the invocation handler of that mock object; second, just using a stub instead of a mock for $connection works, too. The second solution seemed a bit cleaner, but even more inscrutable, so I went with the former for now. Happy with either, though.

Integrating the code by tuwebo → from 🐛 Replace deprecated REQUIREMENT_ERROR constant in search_api_db_defaults_requirements() for Drupal 11+ compatibility Active as part of the necessary fixes (since a new failure popped up during my vacation). Also fixing the failure in SearchApiBulkFormTest.

Since this is part of fixing the pipelines, I’m integrating this fix into 🐛 Fix failing pipelines Active . Maybe an MR with all fixes will help resolve the failure in the MR?

Thanks a lot for your work on this, your changes look good to me.
Did you also test the MR on an actual site or did you just fix the tests? If you can confirm this works for you I think this would be RTBC.

However, in any case we have to wait for 🐛 Replace deprecated REQUIREMENT_ERROR constant in search_api_db_defaults_requirements() for Drupal 11+ compatibility Active (and possible 🐛 Fix failing pipelines Active ?) to fix the pipelines.

I rewrote the code to use DeprecationHelper, which I think is the “proper” way to do this. I also temporarily enabled testing against other Drupal versions to make sure this is compatible with all versions we want to support.
However, tests are still failing and I have no idea, why.

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

Should be fixed in this MR.

drunken monkey → created an issue.

drunken monkey → created an issue.

Confirmed to work now, will be part of the 1.7.1 release.

Confirmed to work now, will be part of the 1.7.1 release.

Seems like mxr576 accidentally set this to RTBC in #15, so setting back to NW. I’m leaning towards “Closed (won’t fix)”, though – the ddev lead developer saying it’s a bad idea sounds hard to beat.
If you can come up with a single file that you’d say is almost as useful as your current proposal (as mkalkbrenner suggests in #14), and if it’s easy enough to maintain I might still consider it, but would still need stronger support than now.

ensures all contributors run the same clean, isolated, and predictable environment.

I don’t plan on using ddev for my local contrib dev environment, so that’s actually more a con than a pro in my books.

Does anyone else have any input on this?

Thanks for posting the patch!
Can someone confirm that the patch resolveds the problem for them?

@ravi shankar karnati: Can you confirm the patch works for you, then?