Test for compatibility with Drupal 9

> and PublishComment seems like it’s the fault of Core (still triggered, of course, every time a comment is published)

Stumbled on this when working on the tests in the other issue. That CR is not too instructive, but reading the source enlightened me.

We have to replace "comment_publish_action" with "entity:publish_action:comment" in ProcessorTestBase.php, then the lights are green.

Plugin\ConfigurableTrait' not found

What?!? Don’t really understand that.

When I look through the console output

https://dispatcher.drupalci.org/job/drupal8_contrib_patches/790/console

I see that it is not the first error.

failed database connections are bad

in short I think testbot may have failed so I am retesting

09:04:44 ---------------- Starting dbcreate ----------------
09:04:44 Directory created at /var/lib/drupalci/workspace/jenkins-drupal8_contrib_patches-790/ancillary/dbcreate
09:04:44 Attempting to connect to database server.
09:04:44 SQLSTATE[HY000] [2002] Connection refused
09:04:44 Could not connect to database server.
09:04:44 Sleeping 10 seconds to allow service to start.
09:04:54 Attempting to connect to database server.

Attaching remaining one fix patch and own deprecations (search_api) patch.

Attaching combined patch and interdiff.

Before applying #19, I was getting 29 errors.

After applying the patch, most of those errors have gone except following which are in search_api.drush.inc,

➜  d9 git:(8.8.x) ✗ vendor/bin/drupal-check -d modules/contrib/search_api


 363/363 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ -------------------------------------------------
  Line   search_api.drush.inc
 ------ -------------------------------------------------
  304    Call to deprecated function drush_get_option().
  305    Call to deprecated function drush_get_option().
  367    Call to deprecated function drush_print().
  385    Call to deprecated function drush_print().
  401    Call to deprecated function drush_print().
  417    Call to deprecated function drush_print().
  435    Call to deprecated function drush_print().
 ------ -------------------------------------------------


 [ERROR] Found 7 errors

Not sure if we are supposed to fix the above errors or not?

Marking this RTBC as the drush errors are from *.drush.inc file which is used by Drush8 only and Drupal9 will use drush 9 which has different setup (service file, command class etc).

We need to remove the Drush 8 support. Drush 8 is for Drupal 8.3 and lower which is EOL. Ref. https://docs.drush.org/en/master/install/#drupal-compatibility

I created a separate ticket for removing Drush 8: #3077570: Remove Drush 8 support from the 8.x branch

Maybe "postponed" is a better status. We can run this test again when #3077570: Remove Drush 8 support from the 8.x branch is in.

I was mistaken, Drush 8 is still supported, but not recommended. We can move the removal of it in here and switch to Drush 10 when Drupal 9 is released.

Removed Drush 8.

With the latest patch I'm still seeing 3 lines being flagged. Not sure if this is PHPStan acting up or if there's something to actually fix.

$ drupal-check .
365/365 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
------ --------------------------------------------------------------------------------------------
Line   src/Plugin/search_api/processor/Stopwords.php
------ --------------------------------------------------------------------------------------------
23     Class Drupal\Core\Security\TrustedCallbackInterface not found and could not be autoloaded.
------ --------------------------------------------------------------------------------------------
------ ---------------------------------------------------------------------------------------------
Line   tests/src/Kernel/Processor/AddHierarchyTest.php
------ ---------------------------------------------------------------------------------------------
23     Class Drupal\Tests\taxonomy\Traits\TaxonomyTestTrait not found and could not be autoloaded.
------ ---------------------------------------------------------------------------------------------
------ ---------------------------------------------------------------------------------------------
Line   tests/src/Kernel/Views/TaxonomyTermArgumentTest.php
------ ---------------------------------------------------------------------------------------------
14     Class Drupal\Tests\taxonomy\Traits\TaxonomyTestTrait not found and could not be autoloaded.
------ ---------------------------------------------------------------------------------------------
[ERROR] Found 3 errors

1. +++ b/src/ParamConverter/SearchApiConverter.php
   @@ -57,7 +62,16 @@ class SearchApiConverter extends EntityConverter implements ParamConverterInterf
      public function convert($value, $definition, $name, array $defaults) {
        /** @var \Drupal\search_api\IndexInterface $entity */
   -    $storage = $this->entityManager->getStorage('search_api_index');
   +    try {
   +      $storage = $this->entityTypeManager->getStorage('search_api_index');
   +    }
   +    // @todo Use a multi-catch once we depend on PHP 7.1+.
   +    catch (InvalidPluginDefinitionException $e) {
   +      return NULL;
   +    }
   +    catch (PluginNotFoundException $e) {
   +      return NULL;
   +    }
   

   the try/catch here isn't rally necessary? I know that phpstorm by default complains about it, but this is a hardcoded entity type provided by this module, if this would fail then you would have much more severe issues.

2. +++ b/src/Plugin/ConfigurablePluginBase.php
   @@ -16,6 +16,7 @@ abstract class ConfigurablePluginBase extends HideablePluginBase implements Conf
      // useful. Since PHP 5 complains when adding this trait along with its
      // "parent" trait to the same class, we just add it here in case a child class
      // does need it.
   +  // @todo Change this once we depend on PHP 7.0+.
      use PluginDependencyTrait;
   

   Drupal 8.8 now requires 7.0, so I suppose that can be cleaned up?

3. +++ b/src/Plugin/PluginFormTrait.php
   @@ -38,7 +38,7 @@ trait PluginFormTrait {
       */
      public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
   -    if ($this instanceof DrupalConfigurablePluginInterface) {
   +    if ($this instanceof ConfigurableInterface) {
          $this->setConfiguration($form_state->getValues());
        }
   

   not sure what your plan is on how when you want to commit it, this can be a bit tricky in combination with other modules, if they didn't update yet then this might no longer work. The opposite is also true, if they removed their deprecation then this would be broken. You'd eed to check for both interfaces to be backwards compatible yourself. Ugly stuff.

4. +++ b/src/Processor/FieldsProcessorPluginBase.php
   @@ -41,7 +42,7 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
     * - preprocess_query
     */
   -abstract class FieldsProcessorPluginBase extends ProcessorPluginBase implements PluginFormInterface {
   +abstract class FieldsProcessorPluginBase extends ProcessorPluginBase implements PluginFormInterface, TrustedCallbackInterface {
    
   ...
    
   

   this uses TrustedCallbackInterface :)

   I guess the results above were run with Drupal 8.7, where this interface doesn't exist.

5. +++ b/src/UnsavedIndexConfiguration.php
   @@ -127,15 +130,24 @@ class UnsavedIndexConfiguration implements IndexInterface, UnsavedConfigurationI
   +    try {
   +      return $this->getEntityTypeManager()->getStorage('user')->load($uid);
   +    }
   +    catch (InvalidPluginDefinitionException $e) {
   +      return NULL;
   +    }
   +    catch (PluginNotFoundException $e) {
   +      return NULL;
   +    }
   

   same here. if the user storage doesn't exist then you'd never get here..

Last, again not sure what your plans with this are exactly. what I do in my projects is add a version dependency on drupal:system (>= 8.8), but actually in this case, I think you want to add https://www.drupal.org/node/3070687 to mark it as compatible with ^8.8 | ^9.0, then you should be able to actually install it on D9.

Core will not add new deprecations in 8.9, and if then only for removal in Drupal 10.

And yes, to be compatible with 9.0, you have to fix 8.8 deprecations as well.

My plan with my modules is to wait a bit after 8.8 but then relatively soon switch to 8.8 as the minimally supported version, to be able to start testing on 9.0. Probably do another 8.7-compatible release before that, if there's anything useful to be released.

If someone wants bugfixes/new features, then they have to update to the latest Drupal core version. 8.6 only has security-support, I'm not sure if there even was an 8.6 release since 8.7 came out, the only ugly thing would be in case you need to do a security release in that timeframe. But you could keep it only in dev for some time, and a security release could be based on the latest release?

Per a Slack discussion with Gábor Hojtsy regarding usage of D9 tags (Drupal 9, Drupal 9 compatibility, Drupal 9 readiness, etc.), "Drupal 9 compatibility" should be used for contributed projects that need updating and "Drupal 9" was the old tag for D8 issues before the D9 branch was ready. Doing tag cleanup here based on that discussion.

> Huh, weird. The change record leaves the core: key in place in its examples, and I also don’t see a note anywhere that says we have to remove it. But I guess the test bot knows better …

The core key must not be used if the you require a specific minor/patch version *and* you can not require a version lower than 8.7.7 with it, because older versions don't support it.

You can see that the second example with 8.8 does not have a core key.

This is explicitly enforced to avoid a possibly broken requirement (requiring 8.8 but you could then still install it on 8.7.6 and earlier as these versions don't check for the new key). You can require just ^8 and combine it with a drupal:system version requirement if you just need 8.6 or so. But you need 8.8 anyway here :)

1. +++ /dev/null
   @@ -1,449 +0,0 @@
   -
   -/**
   - * Implements hook_drush_command().
   - */
   -function search_api_drush_command() {
   -  $items = [];
   

   I still feel like removing drush8 support could be done separately/later. Having these files around when you use drush 9 shouldn't break anything and we are still on drush8 for most of our projects, so we'd lose access to these commands :) But your decision.

2. +++ b/search_api.info.yml
   @@ -2,7 +2,7 @@ type: module
    configure: search_api.overview
    dependencies:
   -  - drupal:system (>=8.7.4)
   +  - drupal:system (>=8.8)
   

   the drupal:system dependency can be removed now, core_version_requirement covers that.

#43 looks good to me.

Since #3096609: Allow contrib test modules to not need a core or core_version_requirement key was committed, there is no need for a core or a core_version_requirement key in the tests.

Decided to not change the thing @chr.fritsch pointed out in #46, but I removed some calls to $this->assertInternalType() in tests. It's deprecated in favor of type-specific methods as of PHPUnit 7, I think...but since we don't necessarily consistently have that version of PHPUnit running on testbot, I used $this->assertSame($type, gettype($value)) instead.

I've had a look at the patch in comment #46. Everything looks good.

After running the patched code against drupal-check, the only errors that are reported are for drush. As this is being addressed in #3077570: Remove Drush 8 support from the 8.x branch I'm marking as RTBC.

There's a new one for theme functions. While it has been changed to a D10 deprecation due to the late enforcement, it's also safe to remove and is very noisy, somehow it's not a suppressed deprecation message in D9:

User deprecated function: Theme functions are deprecated in drupal:8.0.0 and are removed from drupal:10.0.0. Use Twig templates instead of theme_search_api_index(). See https://www.drupal.org/node/1831138 in Drupal\Core\Theme\Registry->processExtension() (line 498 of web/core/lib/Drupal/Core/Theme/Registry.php)"

Also, same as my question in #3121071: Replace theme_inline_entity_form_entity_table() with twig template, I wondering if these could now just be inlined into the respective forms with #type table. Also depends on whether or not you see this as an API change, but going from a theme function to a template would break any customization on this anyway. Either way, makes sense to do this in a separate issue, we'll see about working on it if you confirm about moving them into the forms.

Thanks for the feedback, we'll have a look at that.

I did run the whole test suite against D9 and found some extra issues.

* The update path test needs to be updated to use the 8.8.0 dump because that's the lowest number that can be updated to Drupal 9
* RenderedItemTest needs an explicit dependency on the path_alias module as that's only automatically installed on D8 in kernel tests (and there's no deprecation message for that). It is a required module and is automatically installed in functional tests.
* \Drupal\Tests\search_api\Unit\Processor\TokenizerTest::testPreprocessSearchQuery failed because it asserted on the old, non-namespaced classs name of MockObject. I updated that, but not 100% sure right now if that will work on D8.8 too but I think so. If not then it could also simply be removed or converted to a @var, it's just there so that method autocomplete/validation works in an IDE.

I also noticed that there some tests that require other contrib modules like search_api_autocomplete and language_fallback_fix, so until these modules also declare themself as D9 compatible you won't be able to run a test run against D9 on DrupalCI.

this conflicted with #3044782: Increase PHP version requirement to 7.0, and since this now requires 8.8 which also requires PHP 7.0, I've removed the explicit php: 7.0 part again.

On the major version, can't provide that much input. I personally have no plans to push out new major versions of my modules any time soon. It does create considerable overhead both for the maintainer and users to switch to a new major version. you'll either need to mark the old branch as unsupported or at least provide eventual security updates for it and so on.

> Calling it 8.x-1.x still, when D9 is already supported, doesn’t make much sense.

That's going to be very common for now. As far as composer is concerned, it's already just 1.x, I could imagine that drupal.org UI could be updated at some point to start cutting of the 8.x- prefix in the UI to remove that confusion. We'll see.

Thanks for the great work!

Nitpick: Although the issue is crossed out indicating it's fixed, the project page says:

The module’s dev version will soon be made fully compatible with Drupal 9 (see #3023170: Test for compatibility with Drupal 9).

so the text could be updated with the exciting news. :)

Any updates to when we'll have a release for this? Currently if I'm using Search API Solr on D9 it pull the dev version which can't be installed because Search API Solr has a version condition for this module: search_api:search_api (>=8.x-1.15).

composer should allow 1.x-dev as being compatible with that and search_api_solr 4.x removed that version dependency afaik, from the .info.yml at lesat.

@Berdir, yes, Composer pulls 1.x-dev fine, but I can't enable the module on Drupal because Drupal doesn't recognize dev versions (https://www.drupal.org/project/drupal/issues/1013302): I get the message: "incompatible with version )". See this issue for more info: https://www.drupal.org/project/search_api_solr/issues/3142313

Also, both V3 (which is the one I'm actually using because I'm working on Acquia Search Solr) and V4 (https://git.drupalcode.org/project/search_api_solr/-/blob/4.x/search_api...) have this dependency.