Make search_api compatible with Drupal 10

I will check the errors and adjust

Seeing the 8.x-1.x branch the changes required to make the module compatible has already been update on the branch. I beleave this issue can be pass to duplicated.

Patch contains the D10 readiness fixes which are not included in the current dev branch. (reported by upgrade status)

@attilatilman I reviewed your patch and found a couple of more issues reported by the drupal rector (see attachment), leaving this as needs work.

nkoporec, as I mentioned in my last comment the patch is against the current dev branch since it has 5 more commits since the latest release. Those commits already contains changes and D10 fixes, so when you apply the patch on the latest dev version you will see only 2 issues.

1. The first with the pre-render warning, which I did not fix (false error) (If you have a suggestion how to fix it, let me know)
2. The second is the queryTemporary method usage which is fixed here: https://www.drupal.org/project/search_api/issues/3300927

Test failures on core version 9.5 are not related to the D10 readiness changes: https://www.drupal.org/project/search_api/issues/3307032

Updated locally, and found this failure when trying to enable acquia search:

PHP Fatal error:  Access level to Drupal\search_api\Plugin\views\query\SearchApiQuery::$offset must be public (as in class Drupal\views\Plugin\views\query\QueryPluginBase) in /var/www/upstreams/search_api/src/Plugin/views/query/SearchApiQuery.php on line 42

Fatal error: Access level to Drupal\search_api\Plugin\views\query\SearchApiQuery::$offset must be public (as in class Drupal\views\Plugin\views\query\QueryPluginBase) in /var/www/upstreams/search_api/src/Plugin/views/query/SearchApiQuery.php on line 42

Will keep working on it. The original issue patch and MR are invalid, so I've rebased and pushed the updates from Attilla in #8 to at least get it to install properly with composer.

Ran into a critical issue related to using Database searches with Search API. It implements Temporary Tables, which was removed in Drupal 10. https://www.drupal.org/node/3211781 This was a casualty of sqlite requring use of prefixes for non-qualified table names.

Unfortunately, there is no replacement, and I'm not sure the core devs will be convinced it should be added back in. We have a few options here...

1) Rewrite the database search queries to not require a temporary table (not sure how feasible that is)
2) Write a contrib module that implements temporary tables and have search_api_db depend on it
3) Add the temporary tables code from core directly into search_api_db, without support for sqlite.

However, we should not let search_api_db block progress on this issue. Instead, I've marked it as only compatible with Drupal 9. While search_api will still work with composer and the modules can get installed, drupal will stop any site from upgrading to Drupal 10 if this module is being used. I suggest we make this a separate issue.

Thanks a lot for your work on this, everyone!

Just two notes: the task manager service should never use access checks (seems we just missed one spot), and I also wouldn’t explicitly specify the DB module as incompatible to Drupal 10. Most of the module will work fine under Drupal 10, it will just fail in case of specific facets settings. Which, yes, should definitely still be fixed somehow – I think someone mentioned, though, that this might still be fixed in Core after all? But can neither find the conversation nor the issue anymore.

Unfortunately, testing for issue forks still doesn’t work for this project, so please do not use MRs here. (Interesting that it does seem to work for Search API Autocomplete, though.) Uploading a revised patch and interdiff for testing. If only DB tests fail, I’ll commit.

No idea what the “Input required” fail is about. Commented out these few lines and created #3313067: Fix behavior of required Views input fields in search forms as a follow-up to avoid holding up Drupal 10 compatibility over this. Also, committed #3262214: Make search_api_autocomplete compatible with Drupal 10 so there should also not be a composer require problem anymore with D10.

The comment you recalled was in #3300927: Remove usage of temporary tables from DB backend. I have no idea where that discussion happened/is happening. There is no mention of an issue related to that in #3304303: Drupal 10.0.0 requirements. Ah, found something now: #3312641: Bring back temporary tables (Connection::queryTemporary()), but as of now, that is not a critical issue and would need the attention of core maintainers.

+++ b/src/Task/TaskEvent.php
@@ -3,7 +3,7 @@
 
 use Drupal\search_api\SearchApiException;
-use Drupal\Component\EventDispatcher\Event;
+use Symfony\Contracts\EventDispatcher\Event;
 

this should stay on the drupal class, it's a BC/FC layer between symfony 4 and 6.

There's also this issue about the class properties: #3304531: Fix access level on $offset, $operator properties

Thanks once again for your help! The attached patch should address the remaining problems.

Not sure why ViewsTest is failing, it might be because of the queryTemporary() problem, but not sure. Anyways, I wouldn’t let this hold up the commit and release, and just try to figure this out later, when maybe all related modules are also D10 compatible and it’s easier to test.

+1 to committing this. That will help modules that have search api integration test (paragraphs and monitoring in my case) getting to green. If you open an issue for that views problem I can maybe have a look at a later time, no promises though.

Great, then let’s do that. Committed. Thanks a lot again, everyone!

Here are the follow-up tickets for the failing tests: #3313067: Fix behavior of required Views input fields in search forms, #3313195: Fix failing ViewsTest::testViewSorts() test on Drupal 10. Would be awesome if you could take a look, of course!

Hi,

Since this patch was merged on search_api, search_api in not compatbile with PHP 7.3 anymore due to
"protected $offset;" replaced by "public int $offset = 0;" on src/Plugin/views/query/SearchApiQuery.php

=> see commit https://git.drupalcode.org/project/search_api/-/commit/1bbfe92f8f9805709...

Even with platform set to PHP 7.3 on composer.json, the last version 1.26 of search API is installed, then it breaks the drush config:import command :
[error] ParseError: syntax error, unexpected 'int' (T_STRING), expecting function (T_FUNCTION) or const (T_CONST) in Composer\Autoload\includeFile() (line 58 of /var/www/html/inokey/web/modules/contrib/search_api/src/Plugin/views/query/SearchApiQuery.php) #0 /var/www/html/inokey/vendor/composer/ClassLoader.php(428): Composer\Autoload\includeFile('/var/www/html/i...')
#1 [internal function]: Composer\Autoload\ClassLoader->loadClass('Drupal\\search_a...')

So :
- either you have to limit the version of search_api installed by composer to 1.25 when PHP version is 7.3
- either you should not use "public int $offset = 0;" but "public $offset = 0;"

> - either you have to limit the version of search_api installed by composer to 1.25 when PHP version is 7.3

search_api.info.yml is set to require php 7.4 by this patch, apparently that doesn't automatically translate to a corresponding composer.json definition.

Note that instead of this definition, it might also be OK to just remove it entirely, it was only there because the parent did _not_ specify it, and just live with a undefined property warning on D9.

Views Bulk Operations module handles the PHP 7.4 requirement by creating two separate minor versions. Currently 4.1 is PHP 7.3 compatible, and 4.2 is PHP 7.4+ compatible. Currently sites that cannot update beyond PHP 7.3 cannot update to the latest security release of Search API.

https://www.drupal.org/project/views_bulk_operations/releases/4.1.6
https://git.drupalcode.org/project/views_bulk_operations/-/compare/4.2.0...

That would first require a new major version to be able to have semver support.

I agree it's not great, but fwiw php 7.3 is itself long out of security support and if you care about security you really should start there.

The fact that it's not promoted to composer.json is actually kind of an advantage, it should be fairly easy to patch that property and the php requirement out.

Just wanna chime in here - the site I’m working on is hosted on an enterprise environment, which I have no control over the PHP version (7.3). For now, I’ve applied the patch of the security fix to search_api 8.x-1.25.