Fix failing pipelines

Let’s see whether just adding the attribute works with all Drupal versions. Otherwise, we’ll probably have to just ignore those errors for now, until we depend on the relevant Drupal Core version.

The Postgres fail looks like it’s just a CI problem (database not running), nothing to do with us. I’ll run the pipeline again.

OK, this seems to work and be backwards-compatible, just like the change record → promised.
Now we just need to go through around 40 other plugins and make this change.
Anyone interested in a simple, if slightly annoying, issue credit? Otherwise I’ll try to get to it when I have more time (and might just silence those deprecation warnings in the meantime).

Regarding the Postgres failure: unfortunately that seems to be persistent. Does anyone have any idea what could be going wrong there?
In the schedule config, I’m setting _TARGET_DB_VERSION to $CORE_PGSQL_MIN – is that maybe wrong?

Trying this with drupal rector

Spacing is off and can't just be fixed with phpcbf, but should be much quicker

To get this for future travellers:

1. Take raw output from the gitlab pipeline and save as pipeline-output.txt
2. grep '^ Using @' pipeline-output.txt > only-annotation-deprecations.txt
3. sed -E -n "s/^[[:space:]]*Using @([^ ]+) .*Use a (.*) attribute instead\..*$/new \\\DrupalRector\\\Drupal10\\\Rector\\\ValueObject\\\AnnotationToAttributeConfiguration('10.0.0', '10.0.0', '\1', '\2'),/p" only-annotation-deprecations.txt > rector-ready.txt
4. (install drupal rector)
5. (add output from the above to drupal rector)
6. vendor/bin/rector -c rector.php process modules/contrib/search_api

Mostly working, though not sure about that failing test whether related or not...

Thanks a lot for your work on this, much appreciated!

According to this change record → the Views plugin types were converted to attribute discovery in Drupal 10.3 so I think we need to depend on that in order to drop the BC annotations right away. However, since 10.2 is already unsupported (unless I’m mistaken it even seems like support for 10.3 was just dropped) I think this should be fine.

Surprisingly, the failing test is related: Switching our Views row plugin(s) back to use the @ViewsRow annotation does fix the test. The functionality tested is implemented in search_api_search_api_index_insert() but it seems the call to \Drupal::getContainer()->get('plugin.manager.views.row')->clearCachedDefinitions() does not work when using an attribute instead of an annotation. Not even sprinkling further such calls throughout the code (e.g., in the test method itself right after creating the new index, or in \Drupal\views_ui\Form\Ajax\Display::__construct()) fixes the test.
On the other hand, the functionality seems to work fine when trying it out manually (adding a new index and checking whether you can set its row style to “Rendered entity” without a manual cache clear) so this seems to be a tests-only problem, which likely would have to be fixed in Drupal Core.
However, I now spent several hours of precious contrib time on this and don’t have time to file a Core issue with the necessary information and (ideally) failing test case. Leaving this for either anyone else to take care of or for me to later pick up when I have the time. For now, I guess we have to leave the workaround in place to make pipelines pass.

The final remaining deprecation warnings regard our own plugin types and their lack of attributes. I added attribute discovery for backend plugins but this also was a bit of work so I had to leave it at that. If noone else currently has the time we can ignore those warnings for now and add a follow-up issue.

drunken monkey → credited tuwebo → .

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.

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.

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?

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

🐛 The new purge() method is missing from \Drupal\Core\Database\Transaction\TransactionManagerInterface Active this is the interface missing reference with workaround for others.

drunken monkey → credited nicxvan → .

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.

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.

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!

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

Great thank you!

Automatically closed - issue fixed for 2 weeks with no activity.