Drupal 9 Deprecated Code Report

Hi Aleh,
Please fix codestyles issues.

Thanks, @nikolas.tatianenko, fixed in #6

Hi,
thanks for the patch. I am not sure the move of the ConfigSplitDrush8Io class definition is a good idea. I know that it is not conform with coding standards and that is why it is in a codingStandardsIgnore block. The reason it is in that file is that only the drush 8 commands need it and we needed to be php5 compatible, so an anonymous class was not an option. It has nothing to do with the Drupal 9 compatibility. I don't know if drupal-check has an "ignore" annotation but I would accept a patch that added that to make the tool happy.

It would be much more interesting to see what is up with:

Line   tests/src/Kernel/ConfigSplitCliServiceTest.php
--------------
  69     Call to deprecated function file_unmanaged_save_data().

Hi guys.
Please check this #8 patch to address deprecated warning in ConfigSplitCliServiceTest.php.
Please notice that I changed `file_directory_temp()` and `file_uri_target` as well ( drupal_check didn't catch them for some reason, but they are deprecated).

As for ConfigSplitDrush8Io class, I totally understand the reason behind, but, why don't we use patch #6? It's not about D9 compatibility and only used in drush commands, I know. What are other concerns about placing the class in the src/ folder?

Was reviewed. Test works well

Thank you all for working on this!

There is a new one though:

 ------ --------------------------------------------------------------- 
  Line   src/Plugin/ConfigFilter/SplitFilter.php                        
 ------ --------------------------------------------------------------- 
  413    Call to deprecated method htaccessLines() of class             
         Drupal\Component\PhpStorage\FileStorage:                       
         in drupal:8.8.0 and is removed from drupal:9.0.0. Instead use  
         \Drupal\Component\FileSecurity\FileSecurity.                   
 ------ --------------------------------------------------------------- 

Another thing I just noticed.

+++ b/tests/src/Kernel/ConfigSplitCliServiceTest.php
@@ -66,8 +68,15 @@ class ConfigSplitCliServiceTest extends KernelTestBase {
+    catch (FileException $e) {
+      $uri = FALSE;
+    }

We don't need to catch the exception here. Tests fail when exceptions are thrown.

Updated patch as per @bircher comments in #11 and #12.

Hmmm, followup to the patch above, we'd need to specify the core_version_requirement constraint as "^8.8 || ^9", simply due to the fact that FileSecurity was introduced in 8.8 https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21File....

Patch with the version constraint applied.

Regardless of the state of this patch, could the maintainers please update the Drupal 9 porting/status field on the project homepage so that users have some assurance that there is a plan to port to Drupal 9 (or not)?

I would like this to be compatible with all supported versions of Drupal core, so we need to do some if/else to support Drupal 8.7 where the new class doesn't exist yet and Drupal 9 where the old one doesn't any more.

1. +++ b/composer.json
   @@ -25,7 +25,8 @@
   -    "drupal/config_filter": "*"
   +    "drupal/config_filter": "*",
   +    "drupal/core": "^8.8||^9"
   

   we don't need this.

2. +++ b/config_split.info.yml
   @@ -1,7 +1,7 @@
   -core: 8.x
   +core_version_requirement: ^8.8 || ^9
   

   we can leave core: 8.x, it doesn't hurt anyone.
   and we should make it ^8 || ^9

3. +++ b/src/Plugin/ConfigFilter/SplitFilter.php
   @@ -410,7 +410,7 @@ class SplitFilter extends ConfigFilterBase implements ContainerFactoryPluginInte
   -          file_put_contents($htaccess_path, PhpFileStorage::htaccessLines(TRUE));
   +          file_put_contents($htaccess_path, FileSecurity::htaccessLines(TRUE));
   

   we should wrap this in a class_exists if-clause to be compatible with both 8.7 and 9

Hi @bircher, Applied changes that you have given to patch number 15.
Also, after applying patch number 15 there is one more deprected constant CONFIG_SYNC_DIRECTORY, changed it and created a patch.
Please Review.

Looks like that last patch needed a small tweak as class_exists() expects a string.

I guess I should have paid attention to the other tests! I missed the $this in the static method on the form. Does that need to be static? Here is a patch that should cover that if it is meant to be static.

Here is the patch with test fixes (hopefully) and some clean-up (interdiff attached).
Cheers!

Great, this is coming along nicely!
Thanks for all the patches.

RE: #20, #22 We don't inject the drupal settings. We use them as the static singleton instead, no need to try to inject anything to the constructor.

RE #25:

1. +++ b/src/Form/ConfigSplitEntityForm.php
   @@ -334,8 +335,8 @@ class ConfigSplitEntityForm extends EntityForm {
        global $config_directories;
   -
   -    return strpos(rtrim($folder, '/') . '/', rtrim($config_directories[CONFIG_SYNC_DIRECTORY], '/') . '/') !== FALSE;
   +    $config_sync_directory = Settings::get('config_sync_directory');
   +    return strpos(rtrim($folder, '/') . '/', rtrim($config_directories[$config_sync_directory], '/') . '/') !== FALSE;
   

   We need to stop using the global $config_directories, and also the code here is wrong. Settings::get('config_sync_directory') already is the config directory path.

2. +++ b/tests/src/Kernel/ConfigSplitKernelTest.php
   @@ -105,7 +106,8 @@ class ConfigSplitKernelTest extends KernelTestBase {
        global $config_directories;
   -    $config_directories[CONFIG_SYNC_DIRECTORY] = $sync;
   +    $config_sync_directory = Settings::get('config_sync_directory');
   +    $config_directories[$config_sync_directory] = $sync;
   

   same here, we need to set the Settings here instead of the global variable.

3. +++ b/src/Plugin/ConfigFilter/SplitFilter.php
   @@ -410,7 +411,12 @@ class SplitFilter extends ConfigFilterBase implements ContainerFactoryPluginInte
   +          if(class_exists('FileSecurity')) {
   

   Doesn't this need the fully qualified name? so FileSecurity::class maybe?

Also I guess we need to then finally run the patch here against Drupal 9.x

Thanks for the feedback! Here is the patch according to #26.

drupal-check on latest dev:

22/22 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

------ -----------------------------------------------------------------------------------------------------------------------
Line config_split.drush.inc
------ -----------------------------------------------------------------------------------------------------------------------
Class ConfigSplitDrush8Io was not found while trying to analyse it - autoloading is probably not configured properly.
💡 Learn more at https://phpstan.org/user-guide/autoloading
------ -----------------------------------------------------------------------------------------------------------------------

------ ---------------------------------------------------------------------------------------------
Line src/Plugin/ConfigFilter/SplitFilter.php
------ ---------------------------------------------------------------------------------------------
418 Call to deprecated method htaccessLines() of class Drupal\Component\PhpStorage\FileStorage:
in drupal:8.8.0 and is removed from drupal:9.0.0. Instead use
\Drupal\Component\FileSecurity\FileSecurity.
------ ---------------------------------------------------------------------------------------------

[ERROR] Found 2 errors

But these may be false positive due to D8.7- compatibility? If so, this may confuse people making their checks to see if they can upgrade the core, some message will have to added in project description to explain that.

I believe this is Fixed.

RE #30 those are false positive due phpstan complaining about class defined in the same file and the module strategy wanting to support version below 8.8.

There is a /* @phpstan-ignore-next-line */ annotation we can use, but it works only for actual validation error and not for syntax issues, so it would work for the deprecated htaccessLines() but the autoloading error will still fires.

I don't think there is a suitable option, other than re-factoring the code in here or suggesting a patch do phpstan-drupal to ignore autoloading errors. Both not ideal.

It's OK, thank you for the explanation! There's a "Drupal 9 porting info " field in the d.o project page where it could be said which version of the module is compatible with which versions of core.