Drupal 9 Deprecated Code Report

It seems all we need is core_version_requirement property in *.info.yml

The patch looks right. I tested by installing on both Drupal 8.9 and 9.0. It works in both.

I hope to have this module available in Drupal 9.

Thanks for testing! I checked the deprecations with the patch using the Upgrade Status module and there are some listed so moving this back to "Needs work". Maybe those should be fixed in a follow up issue though?

CUSTOM PROJECTS
--------------------------------------------------------------------------------
Fences
Scanned on Sat, 05/23/2020 - 20:42.

5 warnings found.

modules/contrib/fences/src/TagManager.php:
┌──────────┬──────┬─────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                           │
├──────────┼──────┼─────────────────────────────────────────────────────────────┤
│ Check    │ 18   │ Drupal\fences\TagManager must override __construct if using │
│ manually │      │ YAML plugins.                                               │
│          │      │                                                             │
└──────────┴──────┴─────────────────────────────────────────────────────────────┘

modules/contrib/fences/tests/src/Functional/IntegrationTest.php:
┌──────────┬──────┬──────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                            │
├──────────┼──────┼──────────────────────────────────────────────────────────────┤
│ Check    │      │ Class Drupal\FunctionalJavascriptTests\WebDriverTestBase not │
│ manually │      │ found and could not be autoloaded.                           │
│          │      │                                                              │
│ Check    │ 13   │ Class Drupal\FunctionalJavascriptTests\WebDriverTestBase not │
│ manually │      │ found and could not be autoloaded.                           │
│          │      │                                                              │
└──────────┴──────┴──────────────────────────────────────────────────────────────┘

modules/contrib/fences/tests/src/Kernel/FieldOutputTest.php:
┌──────────┬──────┬─────────────────────────────────────────────────────────────┐
│  STATUS  │ LINE │                           MESSAGE                           │
├──────────┼──────┼─────────────────────────────────────────────────────────────┤
│ Check    │      │ Class Drupal\KernelTests\KernelTestBase not found and could │
│ manually │      │ not be autoloaded.                                          │
│          │      │                                                             │
│ Check    │ 17   │ Class Drupal\KernelTests\KernelTestBase not found and could │
│ manually │      │ not be autoloaded.                                          │
│          │      │                                                             │
└──────────┴──────┴─────────────────────────────────────────────────────────────┘

1. "Drupal\fences\TagManager must override __construct if using YAML plugins." I checked manually, and the class does override the __construct() method. See snippet below.
2. "Class Drupal\FunctionalJavascriptTests\WebDriverTestBase not found and could not be autoloaded." This is a very confusing message, since that class certainly exists and is the appropriate base class for a functional JavaScript test. I triple-checked the namespace, since I thought it should be Drupal\Tests\FunctionalJavascriptTests, but Drupal\FunctionalJavascriptTests is correct.

   I did find two other problems, and the attached patch corrects them:

   1. Missing $defaultTheme property: since the test checks core markup, I used stable. See the change record.
   2. The namespace was Drupal\Tests\fences\Functional instead of Drupal\Tests\fences\FunctionalJavascript. Before and after the patch, the namespace matches the directory.
3. "Class Drupal\KernelTests\KernelTestBase not found and could not be autoloaded." Again, the message is confusing: there is nothing wrong with the namespace or the class.

From TagManager.php:

  public function __construct(ModuleHandlerInterface $module_handler, ThemeHandlerInterface $theme_handler, CacheBackendInterface $cache_backend) {
    $this->moduleHandler = $module_handler;
    $this->themeHandler = $theme_handler;
    $this->setCacheBackend($cache_backend, 'fences', ['fences']);
  }

I ran the kernel test in both Drupal 8.9.x and 9.1.x. It passed using both the patch in #2 and the attached patch. The attached patch does make one change that affects the kernel test: it moves the trait from tests/src/Kernel to tests/src/Traits, following PHPUnit file structure, namespace, and required metadata

I have not set up my environment to run javascript tests locally, but the testbot should be able to run it.

Running the kernel test on Drupal 9.1.x did generate one deprecation notice: before Drupal 10.0.0, we will have to declare the return type (void) for the setUp() method.

No good deed goes unpunished. Now that the functional JavaScript test is running, it fails.

Since I cannot run the test locally, I am adding a debug line even though I know this version of the test will fail.

More of the same.

oops.

More of the same.

Maybe, instead of trying to figure out where in the process all those tags are being stripped, I should search for elements by CSS selector and test their text.

More of the same.

And more.

More of the same.

Sometimes it is a good idea to take a break and come back later.

Still trying to debug the tests.

Progress. With a few more iterations, I should be able to fix the test.

More of the same.

More of the same.

This version of the functional JavaScript test does not depend much on core markup. I think that is an advantage over the existing test. Mostly it just checks for the HTML elements and classes added by Fences.

I am hiding all of my test patches and providing an interdiff comparing to the patch in #5.

I think that fixing the tests, as I have done, is outside the original scope of the issue. But some things, like providing the $defaultTheme property, are in scope, so defining what is in and out of scope is challenging. I suggest we go ahead and commit the patch as is, or with whatever tweaks are suggested in further reviews.

When I test with Drupal 9, the testbot reports,

drupal/fences dev-2.x requires drupal/core ^8.7.0

I think that comes from the dependencies in the .info.yml file. If we are going to require 8.7, we might as well require 8.7.7, since then we no longer need the core: 8.x line.

It looks like a bug that

dependencies:
  - drupal:system (>=8.7.0)

is translated into drupal/core ^8.7.0, but maybe that is not exactly what is going on.

I asked on the #d9readiness channel, and the test failure is expected: the Drupal core requirement comes from the branch, not the patch.

IMO either the patch in #19 or the patch in #20 is good to go, depending on whether the maintainers want to support 8.7.0 through 8.7.6.

Thanks for the patches! There are a lot! :) I'm not going to be able to fully review but let me review as best I can.

1) Regarding #5:

a) Re 5.3, I checked in with Gábor Hojtsy about this in Slack and he said that the autoload errors are often wrong and can probably be ignored.

b) Re 5.1, maybe this is the same as the autoload false errors.

c) Re 5.2.1 (Missing $defaultTheme property), good catch.

d) Re 5.2.2 (FunctionalJavascript), looks like this is just changing Functional to FunctionalJavascript which doesn't seem required but just cleanup. Searching for Functional after patching only shows FunctionalJavascript so all instances were changed.

2) Regarding #19 vs #20:

I don't know if the maintainers will want to support ^8 or ^8.7.7 so having both options is good.

3) As for fixing the test, I guess the test was failing so fixing it here makes sense. I'm not sure that is the best way to fix it so I'll leave that for someone else to chime in on.

4) Patch from both #19 and #20 apply cleanly.

[mac:kristen:fences]$ patch -p1 < 3101948-19.patch 
patching file fences.info.yml
patching file tests/src/Functional/IntegrationTest.php
patching file tests/src/Kernel/FieldOutputTest.php
patching file tests/src/Kernel/StripWhitespaceTrait.php

[mac:kristen:fences]$ patch -p1 < 3101948-20.patch 
patching file fences.info.yml
patching file tests/src/Functional/IntegrationTest.php
patching file tests/src/Kernel/FieldOutputTest.php
patching file tests/src/Kernel/StripWhitespaceTrait.php

5) Regarding moving StripWhitespaceTrait from Kernel to Traits. I assume that is not required but just cleanup. I searched the code for StripWhitespaceTrait and see all the namespaces changed after patching.

6) What's the best way to manually test Fences quickly?

7) Leaving "Needs review" so someone can take a look at the test restructuring.

Thanks for the patches! There are a lot!

It is safe to ignore the patches in Comments #7 through #18. That is why I hid them. I am not set up to run functional JavaScript tests locally, so the only way I can debug those tests is by uploading patches for the testbot to try.

1. I think the warnings from the automated test are either fixed or can be ignored. I wonder whether some of the warnings will go away now that I fixed the directory structure. I had trouble installing the Upgrade Check module myself. Maybe someone else can re-run the tests.
2. I agree.
3. As I said in #19, it is hard to decide the scope clearly.
4. Er, doesn't the testbot already confirm that the patches apply cleanly?
5. I think moving the trait around does not really matter. According to PHPUnit file structure, namespace, and required metadata,
   

   Each test suite will discover the trait if it is placed in the tests/src/Traits directory and the trait will have a namespace of \Drupal\Tests\$extension\Traits.

   Maybe that means we do not need a use statement at the top of the file if the trait is in the right namespace. I did not test that. But I took that to mean that tests/src/Traits is the expected location for traits in tests.

6. I test by installing the Umami demo profile so that I have some content to work with. Then Manage display for the Recipe content type, click on the gear/settings icon for a field. I like to set the Ingredients field to use <ul> as the Field Tag and <li> as the Field Item Tag. Remember to use both Update and Save buttons.
7. Of course I was hoping for a quick RTBC, but that is not always for the best.

Regarding 23.4, sure, they applied cleanly right then. For core or projects with lots of commits, this can change quickly so I always check it and report it. :)

I'm fine with dropping support for 8.7.0 - 8.7.6, so #20 looks great to me.

Thanks all.

Hi Sam152,

I guess a new rc2 release would make sense then to make it usable under Drupal 9?