Drupal 9 compatibilities report

please review

changes made as per suggestion in #10

I found some issues in the patch from #16 and I've tried to fix them.

Summary of the changes:

• For the Form classes, I used the class method messenger() instead of using \Drupal::messenger(). This is because FormBase uses the MessengerTrait.
• For the Views filter plugin "name_fulltext" I injected the database service (instead of using \Drupal::database()). I copied this code from Drupal\views\Plugin\views\filter\StringFilter.
• All tests are moved from src/Tests to tests/src/Unit, tests/src/Kernel, tests/src/Functional and tests/src/Traits.

There are some additional deprecations reported by the Upgrade Status module which I have not fixed:

Name Field

Check manually

I put this in the issue summary.

I haven't tested the module manually after making the changes. Let's first see if the tests will pass.

Let me know if anyone is keen to co-maintain, I'm time short atm.

I looked at the test failures and started working on fixing them.

A good replacement for $element->asXML() could be $element->getHtml(). It happens that in WebTestBase a call to the method xpath() returned a \SimpleXMLElement, but in BrowserTestBase a \Behat\Mink\Element\NodeElement is returned. I did this at all locations, but locally the tests failed on an other thing after that.

Convert code to use Mink

I see that xpath is used a lot for checking if certain fields or other elements exist in the HTML output. In Drupal browser tests that are based on PHPUnit, Mink is used to do these checks. I therefore think that all code that is using xpath should be converted to use Mink. I added several @todo's in the code.

Also added $defaultTheme to NameTestBase.

@Alan D.
I'm not sure I'll have the time to help maintaining this module. I was just looking into this to see if I already could update a relatively small site to Drupal 9.

This should fix NameAdminTest.

Contains also a single coding standards fix: properties $web_user and $admin_user have been renamed to $webUser and $adminUser.

Still needs work because of failing NameWidgetTest.

This should fix NameWidgetTest.

Additionally, I also fixed the following issues:

• Support for asserting against non-boolean values in ::assertTrue is deprecated in drupal:8.8.0 and is removed from drupal:9.0.0. Use a different assert method, for example, ::assertNotEmpty(). See https://www.drupal.org/node/3082086
• Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was name_element_pre_render. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725
• Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was Drupal\name\Plugin\Field\FieldType\NameItem::fieldSettingsFormPreRender. Support for this callback implementation is deprecated in 8.8.0 and will be removed in Drupal 9.0.0. See https://www.drupal.org/node/2966725
• Replaced some assertTrue() calls with better alternatives, for example with assertInstanceOf().
• And some coding standards and spelling.

I realize this patch is becoming quite big - which makes it harder to review. If you want me to divide it into multiple chunks, I can do that.
I could do for example the following divisions:

• Deprecation fixes in /src (thus excluding tests).
• Text/spelling corrections.
• Coding standard fixes (some).
• #pre_render callbacks fixes
• Test conversions.

The patch for converting the tests would remain big, I'm afraid.

Still left to do: convert theme functions to twig templates. That could be big patch by itself, so I would say that this should be postponed to a follow-up.

Patch applies to 2.x-dev, and in conjunction with https://www.drupal.org/files/issues/2020-04-17/name-3128599-2.patch (which simply adds the core_version_requirement), I can successfully add and configure a name field on a content type.

Looks good to me.

This looks good in combination with the patch that adds the core_version_requirement to the module's info.yml file.
It would be great to get these committed, so that folks don't need to include the patches via composer, because, in my experience, and possibly others, even though a patch adds the `core_version_requirement` value, composer still complains that it's not D9 supported.

In my understanding, there is a process that happens on the packagist repo when composer pulls down a module, where based on the `core_version_requirement`, it sets the values for the module in the composer.lock file, such as

"require": {
    "drupal/core": "^8 | ^9"
},

But, when adding patches, even though the info.yml is right, the .lock file still contains the old values

"require": {
    "drupal/core": "^8.8"
},

Which will be an issue during the D9 upgrade process.

Thanks everyone!

I didn't review but with no critical issues for the 8.x branch I've pushed these through directly.

As an aside, being out of the loop for a year or more, should I be creating a new branch for D9+ now? My lazy search didn't show much info and the project patch guide seems outdated.

Alen D. no need for a new branch, you can just tag a new release candidate, eq: 8.x-1.0-rc3

Cool thanks, albeit I may branch to 1.x once D8 is depreciated :)

I'll leave dev as is for a while before tagging up a new release.

@Alan D.
If you want to switch to semantic versioning, you would need to increase the major version. But for D9 support you don’t necessarily have to switch.

From https://www.drupal.org/node/1015226#semver-transition:

When your project switches to using semantic versioning, you must increment your major version. For example, if the latest release is 8.x-3.5, your next stable release will be 4.0.0

8.x-* projects can be compatible with Drupal 9, so you don’t need to switch right away if you aren’t ready to start a new major version release series.

Thanks :)

I still get this warning after cache rebuild:

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_name_item_list(). See https://www.drupal.org/node/1831138 in Drupal\Core\Theme\Registry->processExtension() (line 498 of core/lib/Drupal/Core/Theme/Registry.php).

I guess this is due to the functions defined in name.theme.inc.

@mrshowerman
Correct, we need to open a follow-up issue for that. I noted this in comment #21:

Still left to do: convert theme functions to twig templates. That could be big patch by itself, so I would say that this should be postponed to a follow-up.

Do you want to create a new issue for that?

Note that theme functions will keep working during the D9 release cycle, so there is less urgency to change it right now. But the sooner it is done, the better, I guess.

Ah, I see someone already opened a follow-up: #3168948: Remove theme functions deprecated in Drupal 9

Thanks @MegaChriz for pointing this out. Added the follow-up as a related issue.

For anyone else wondering, the theme-related error message can be silenced with a quick/temporary patch:

diff --git a/core/lib/Drupal/Core/Theme/Registry.php b/core/lib/Drupal/Core/Theme/Registry.php
index 094780a10..15c88b029 100644
--- a/core/lib/Drupal/Core/Theme/Registry.php
+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -495,7 +495,7 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
         // if the theme hook specifies a function callback instead, check to
         // ensure the function actually exists.
         if (isset($info['function'])) {
-          trigger_error(sprintf('Theme functions are deprecated in drupal:8.0.0 and are removed from drupal:10.0.0. Use Twig templates instead of %s(). See https://www.drupal.org/node/1831138', $info['function']), E_USER_DEPRECATED);
+          @trigger_error(sprintf('Theme functions are deprecated in drupal:8.0.0 and are removed from drupal:10.0.0. Use Twig templates instead of %s(). See https://www.drupal.org/node/1831138', $info['function']), E_USER_DEPRECATED);
           if (!function_exists($info['function'])) {
             throw new \BadFunctionCallException(sprintf(
               'Theme hook "%s" refers to a theme function callback that does not exist: "%s"',

Hopefully, someone will fix this soon?