Deprecated Code Report and Drupal 9 readiness

Thanks! Looks good to me.

Just a couple of things

1. +++ b/src/ConfigImporterExporter.php
   @@ -169,7 +169,7 @@ class ConfigImporterExporter {
   -    return $this->configManager->getEntityManager()->getStorage($entity_type_id);
   +    return $this->configManager->getEntityTypeManager()->getStorage($entity_type_id);
   

   Why is this code calling the config manager to get the entity type manager??

2. +++ b/tests/src/Kernel/ConfigDevelSubscriberEntityTest.php
   @@ -23,7 +23,7 @@ class ConfigDevelSubscriberEntityTest extends ConfigDevelSubscriberTestBase {
   +    $entity = \Drupal::entityTypeManager()->getStorage('config_test')->load('test');
   

   Inside a test, shouldn't a service be got with $this->container->get()?

Hello

Before applying patch from comment 2

root@5583cd89f436:/project# drush us-a config_devel
 [notice] Processing /project/app/modules/custom/config_devel.
 15/15 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


================================================================================
Configuration development,  8.8.6
Scanned on Fri, 05/22/2020 - 12:17

FILE: modules/custom/config_devel/drush/config_devel.drush.inc

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 143  Call to deprecated function drush_log().
--------------------------------------------------------------------------------
Fix now        253  Call to deprecated function drupal_get_profile(). Deprecated
                    in drupal:8.3.0 and is removed from drupal:9.0.0. Use the
                    install_profile container parameter or
                    \Drupal::installProfile() instead. If you are accessing the
                    value before it is written to configuration during the
                    installer use the $install_state global. If you need to
                    access the value before container is available you can use
                    BootstrapConfigStorageFactory to load the value directly
                    from configuration.
--------------------------------------------------------------------------------

FILE: modules/custom/config_devel/src/ConfigImporterExporter.php

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix now        256  Call to deprecated method getEntityManager() of class
                    Drupal\Core\Config\ConfigManagerInterface. Deprecated in
                    drupal:8.7.0 and is removed from drupal:9.0.0. Use
                    \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeMan
                    ager() instead.
--------------------------------------------------------------------------------

FILE:
modules/custom/config_devel/src/EventSubscriber/ConfigDevelSubscriberBase.php

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix now        45   Call to deprecated method getEntityManager() of class
                    Drupal\Core\Config\ConfigManagerInterface. Deprecated in
                    drupal:8.7.0 and is removed from drupal:9.0.0. Use
                    \Drupal\Core\Config\ConfigManagerInterface::getEntityTypeMan
                    ager() instead.
--------------------------------------------------------------------------------

FILE:
modules/custom/config_devel/tests/src/Kernel/ConfigDevelSubscriberEntityTest.php

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix now        26   Call to deprecated function entity_load(). Deprecated in
                    drupal:8.0.0 and is removed from drupal:9.0.0. Use the
                    entity type storage's load() method.
--------------------------------------------------------------------------------

FILE:
modules/custom/config_devel/tests/src/Kernel/ConfigDevelSubscriberTestBase.php

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Fix now        32   Call to deprecated function drupal_mkdir(). Deprecated in
                    drupal:8.0.0 and is removed from drupal:9.0.0. Use
                    Drupal\Core\File\FileSystem::mkdir().
--------------------------------------------------------------------------------

FILE: modules/custom/config_devel/tests/src/Unit/ConfigDevelTestBase.php

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 33   Call to deprecated method getMock() of class
                    Drupal\Tests\UnitTestCase. Deprecated in drupal:8.5.0 and is
                    removed from drupal:9.0.0. Use
                    Drupal\Tests\PhpunitCompatibilityTrait::createMock()
                    instead.
--------------------------------------------------------------------------------
Check manually 35   Call to deprecated method getMock() of class
                    Drupal\Tests\UnitTestCase. Deprecated in drupal:8.5.0 and is
                    removed from drupal:9.0.0. Use
                    Drupal\Tests\PhpunitCompatibilityTrait::createMock()
                    instead.
--------------------------------------------------------------------------------
Check manually 37   Call to deprecated method getMock() of class
                    Drupal\Tests\UnitTestCase. Deprecated in drupal:8.5.0 and is
                    removed from drupal:9.0.0. Use
                    Drupal\Tests\PhpunitCompatibilityTrait::createMock()
                    instead.
--------------------------------------------------------------------------------

FILE: modules/custom/config_devel/config_devel.info.yml

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 0    Add core_version_requirement: ^8 || ^9 to
                    config_devel.info.yml to designate that the module is
                    compatible with Drupal 9. See
                    https://drupal.org/node/3070687.
--------------------------------------------------------------------------------

After applying the patch.

root@5583cd89f436:/project# drush us-a config_devel
 [notice] Processing /project/app/modules/custom/config_devel.
 15/15 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


================================================================================
Configuration development,  8.8.6
Scanned on Fri, 05/22/2020 - 12:18

FILE: modules/custom/config_devel/drush/config_devel.drush.inc

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 143  Call to deprecated function drush_log().
--------------------------------------------------------------------------------

FILE: modules/custom/config_devel/config_devel.info.yml

STATUS         LINE                           MESSAGE
--------------------------------------------------------------------------------
Check manually 0    Add core_version_requirement: ^8 || ^9 to
                    config_devel.info.yml to designate that the module is
                    compatible with Drupal 9. See
                    https://drupal.org/node/3070687.
--------------------------------------------------------------------------------

drush_log is only for Drush 8. So maybe it can be removed. It will be at the maintainers will.

For the core_version_requirement, I will make a dedicated issue.

So I take reviews from comment 4 into account.

First point:

Why is this code calling the config manager to get the entity type manager??

Good question, I was changing the code to have it injected directly but if it is in Core's ConfigManagerInterface, it can be relied on, so I removed my changes. This question should be asked in Core's issue queue.

Second point:

In core/tests/Drupal/KernelTests/Core/Entity/EntityKernelTestBase.php

$this->entityTypeManager = $this->container->get('entity_type.manager');

So yes it is that way that is should be loaded. The problem is the rest of the codebase needs to be updated too. Maybe in another issue to not block this one?

Reviewing this.

I closed #3138949: Drupal 9 readiness as duplicated of this one, as in here there is already work going on and both are tackling D9 deprecation issues.

So updating the issue summary to reflect this.

I must say @Grimreaper has a point on both responses to #4. The code was already using $this->configManager->entityManager() so makes sense for consistency to just a simple replacement to $this->configManagr->entityTypeManager(). It's a core API, so why not rather than injecting a new service to the already long list?

@Grimreaper about the test, if you can create a followup issue where to tackle all Drupal::service() to $this->container->get() there will be more chances the maintainers will accept this work.

Then this issue Needs Work due:

FILE: web/modules/contrib/config_devel/drush/config_devel.drush.inc

STATUS LINE MESSAGE
--------------------------------------------------------------------------------
Check manually 143 Call to deprecated function drush_log().
--------------------------------------------------------------------------------

FILE: web/modules/contrib/config_devel/config_devel.info.yml

STATUS LINE MESSAGE
--------------------------------------------------------------------------------
Check manually 0 Add core_version_requirement: ^8 || ^9 to
modules/contrib/config_devel/config_devel.info.yml to
designate that the module is compatible with Drupal 9. See
https://drupal.org/node/3070687.
--------------------------------------------------------------------------------

Hello,

@Gambry, thanks for the review.

Follow up issue created: #3139434: Coding standard: service usage in tests

About the change in the .info.yml, I can update the patch with what I post on the duplicated issue.

But before that, can I have a feedback on the point on Drush 8 please?

As the deprecated code is only used by Drush 8, there is no need to remove it. Even if personally I am in favor of dropping Drush 8 support , I need feedback on that. Thanks.

@@ -23,7 +23,7 @@ class ConfigDevelSubscriberEntityTest extends ConfigDevelSubscriberTestBase {
    * {@inheritdoc}
    */
   protected function doAssert(array $data, array $exported_data) {
-    $entity = entity_load('config_test', 'test', TRUE);
+    $entity = \Drupal::entityTypeManager()->getStorage('config_test')->load('test');

I don't think this quite does the same thing as what it replaces. The third TRUE argument in entity_load() is an instruction to reset the internal cache for the entity type.

The patch generated by drupal-rector uses this an extra resetCache() step before loading the entity.

\Drupal::service('entity_type.manager')->getStorage('config_test')->resetCache(['test'])->load('test')

Does this make more sense for this test case? I don't know enough to answer that myself; I just noticed it when comparing this patch with the one Drupal rector generated. See #3146855: Automated Drupal 9 compatibility fixes.

On the drush_log() point, it's not necessary to remove this for D9 compatibility. The drush.inc file will only be loaded by Drush 8, so it will never be loaded with Drupal 9. So it's harmless to leave it in place, and it's up to the maintainer how long to support Drush 8. That makes it out of scope here, I'd say.

This patch adds the core_version_requirement, which is the last missing step needed for D9 compatibility. I don't see any benefit to doing that in a separate issue. Now it's just one patch to follow.

This patch doesn't attempt to address #4 or #11.

Comments #6 and #9 about leaving #4 to a follow-up sound reasonable to me. Now that D9 is out, there's a greater urgency to get a D9-compatible release out.

The actual patch would help. (Doh!)

Just a recap:

About comments on #4.1

From #9

The code was already using $this->configManager->entityManager() so makes sense for consistency to just a simple replacement to $this->configManagr->entityTypeManager(). It's a core API, so why not rather than injecting a new service to the already long list?

About comments on #4.2

From #10

Follow up issue created: #3139434: Coding standard: service usage in tests

About drush deprecations comments

The deprecations are on config_devel.drush.inc which is loaded by Drush < 9. Drush >=9 reads drush.services.yml - and it's class - where deprecations don't exist. So for folk using this module on D8 with Drush 8 there is no risk, and D9 users on Drush 10 won't use deprecated code.

About comments on #11 about entity_load() resetting the case

Tests are green, so better for the maintainer to decide if worth chaining ->resetCache(['test']) or not. This can be done on commit.

I believe all the outstanding issues are addressed, or have valid answers, so back to RTBC.

There is another change needed for D9 compatibility: ConfigImporter now takes the module extension list as a constructor parameter. An optional parameter in D8 has become a required parameter in D9. This one won't have been picked up by automated scans for deprecated code; I discovered it the hard way :-)

It's a simple enough change, just injecting an extra service into ConfigImporterExporter

The test probably needs to be updated too.

Updated the test, it also needed the extra constructor param.

The part with DRUPAL_MINIMUM_PHP - I ran into an undefined constant error, but I don't grok where it was needed. Anyway I noticed that a few tests in core had this defined at the end of the test file, so I just copied that.

Hello everyone,

I am back on this issue after helping other modules to be D9 compatible.

I have read the comments since my last one (#10).

Thanks a lot @gambry and @andrewmacpherson for pushing the issue forward.

So I guess, now we need to wait for maintainers feedback. Or am I missing something?

Regards,

Yep, it looks so!

web/modules/contrib/config_devel/drush/config_devel.drush.inc 143 Call to deprecated function drush_log().

web/modules/contrib/config_devel/src/Event/ConfigDevelSaveEvent.php 10 Class Drupal\config_devel\Event\ConfigDevelSaveEvent extends deprecated class Symfony\Component\EventDispatcher\Event: since Symfony 4.3, use "Symfony\Contracts\EventDispatcher\Event" instead

@gigimaor #11 already mention changing the .drush.inc file is not needed, as Drush 10 won't care.
So it's up to the maintainer to change it or not.

Hiding your files in the meanwhile. This issue is still RTBC.

Quick maintainer fly-by: the most recent comment with patches has TWO patches both of which are red. Further back there's a green one.

Could someone tell an overloaded maintainer which patch is actually RTBC? Thanks! :)

Patch from comment 18 #3094340-18: Deprecated Code Report and Drupal 9 readiness :).

If you need help maintaining this project, I can help. I am not familiar with the Config API code, but coding standards, project page standards, cleaning stuff, etc, is ok for me.

Committed!

Thanks everyone!

@Grimreaper I'll give you a ping on slack! :)