@tadean

Should we keep the type hint of ExtensionPathResolver and make it nullable, ?ExtensionPathResolver ? A symptom of the change in the MR removing the type hint is that you can now pass anything you want the final argument and it works, with a somewhat unexpected deprecation warning:

Passing Drupal\Core\Config\ConfigFactory to the $extension_path_resolver parameter of ConfigCollector::__construct() is deprecated in config_provider:3.0.0-alpha2

I believe that NULL default arguments are also deprecated in PHP 8.4

Thanks @joegraduate !

Verified that implementers of ConfigProviderBase receive the expected deprecation warnings:

 ------ ----------------------------------------------------------------------------------------------------------------- 
  Line   web/modules/custom/test_provider/src/Plugin/ConfigProvider/TestConfigProvider.php  
 ------ ----------------------------------------------------------------------------------------------------------------- 
  332    Call to deprecated method drupalGetProfile() of class Drupal\config_provider\Plugin\ConfigProviderBase:          
         in config_provider:3.0.0-alpha2 and is removed from config_provider:3.1.0.                                       
           ConfigProviderBase::installProfile should be used instead.                                                     
  333    Call to deprecated method drupalGetPath() of class Drupal\config_provider\Plugin\ConfigProviderBase:             
         in config_provider:3.0.0-alpha2 and is removed from config_provider:3.1.0.                                       
           ConfigProviderBase::extensionPathResolver::getPath() should be used                                            
           instead.                                                                                                       
 ------ -----------------------------------------------------------------------------------------------------------------

@skrug @joegraduate It appears that the changes in this commit to ConfigCollector which adds ExtensionPathResolver as a final argument are breaking this feature within config_sync, because config_sync subclasses ConfigCollector via Drupal\config_sync\Plugin\SyncConfigCollector.

Without that API change of the additional argument also being present in config_sync in the config_sync.collector service, the drush command gives the following errors:

drush cd-update --update-mode=1
                                              
  The "--update-mode" option does not exist.

and:

drush cd-update

In Container.php line 149:
                                                                                                                      
  Circular reference detected for service "config_sync.lister", path: "config_sync.lister -> config_sync.collector".

Thanks for working on this @trackleft2 !
Tested out a few aspects of this:

Verified that services are still being registered:

$ drush devel:services
...
- config_snapshot.config_sync.module.pathauto
- config_snapshot.config_sync.module.redirect
- config_snapshot.config_sync.module.search
- config_snapshot.config_sync.module.shortcut
...

Verified that errors are logged if unhandled exceptions occur elsewhere in provisioning the storage:

Error initializing config storage: Failed to retrieve config storage.
(And - separate error forced):
Error initializing config storage: An unrelated error somewhere else occurred.

Verified that errors are logged in the case of any of snapshotSet, extensionType, and extensionName being absent from malformed snapshots (in separate cases):

Error processing config ID config_snapshot.snapshot..module.search: Invalid snapshot data for config ID config_snapshot.snapshot..module.search
Error processing config ID config_snapshot.snapshot.config_sync..search: Invalid snapshot data for config ID config_snapshot.snapshot.config_sync..search
Error processing config ID config_snapshot.snapshot.config_sync.module.: Invalid snapshot data for config ID config_snapshot.snapshot.config_sync.module.

The only thing I noted was that the PHP error_log is used, rather than the Drupal log. But given that this is the service discovery phase, I'm not sure if using the Drupal log channel would even make sense. Perhaps that's why you selected the PHP error log to begin with?

The proposed change in MR !14 successfully resolved the error in my testing. Diffs are now generated with drush cd-update --preview=diff

Thanks @joegraduate!

It looked like MR !4 might potentially have had an issue with configuration items that were also falsy. This seems resolved in MR !5 with the strict check. It seems reasonable that the PHPStan issues can be addressed separately from the test failure.

I tested out MR !5 and the snapshot storage still seems to work as expected. !5 looks good to me.

Added potential patch.

tadean → created an issue.

Added potential patch approach for the issue - thanks to @trackleft2 for suggestion.

tadean → created an issue.

Thank you for your work on this! Tested out MR !10 locally with Drush 12.1.3. Verified the original issue:

$ drush config-sync-list-updates
In LegacyServiceInstantiator.php line 282:                       
  You have requested a non-existent parameter "config.import.commands". 

Verified that with the changes in MR !10 the error does not occur, and config_sync reports expected changes correctly:

$ drush config-sync-list-updates
 ----------------- ---------------- ----------------- 
  Extension         Operation type   Label            
 ----------------- ---------------- ----------------- 
  my_test_content   update           My Content Type  
 ----------------- ---------------- -----------------

config_sync tests pass:

PHPUnit 9.6.10 by Sebastian Bergmann and contributors.

Testing /usr/local/config_sync
....                                                                4 / 4 (100%)

There is one minor phpcs complaint about the create() function:
74 | ERROR | [x] Missing function doc comment (Drupal.Commenting.FunctionComment.Missing)

And one minor complaint about the $state. in the docblock of the constructor:
58 | ERROR | [x] Doc comment parameter name "$state." must not end with a dot (Drupal.Commenting.FunctionComment.ParamNameDot)
This error was unrelated and not introduced by this MR though.

Thanks for all the promising work on this issue! I tested the refactored config-distro-update command against Drush 12.1.2, used in conjunction with the related patch from the config_sync issue 🐛 Config Sync breaks with Drush 12, need to refactor not to use ConfigImportCommands Fixed to avoid the similar error and was able to import configuration changes from a custom module without error:

$ drush config-distro-update
+------------+---------------------------+-----------+
| Collection | Config                    | Operation |
+------------+---------------------------+-----------+
|            | node.type.my_content_type | Update    |
+------------+---------------------------+-----------+

 Import the listed configuration changes? (yes/no) [yes]:
 > yes

 [notice] Synchronized configuration: update node.type.my_content_type.
 [notice] Finalizing configuration synchronization.
 [success] The configuration was imported successfully.

I verified manually that the configuration seemed to be imported correctly.

joegraduate → credited tadean → .

joegraduate → credited tadean → .

Thank you for these great patches @a.dmitriiev !

I tested both of these patches out with the config_distro_ignore functionality. In my testing it appears that the patch in #3144145-6: Consider replacing fnmatch usage → has an issue where any ignore rule will match any config name (e.g. foo will match node.type.my_content_type). The original patch in #3144145-2: Consider replacing fnmatch usage → seemed to work in almost all cases but has a minor issue where a partial match now matches a complete config name (e.g. content matches node.type.my_content_type because the generated regex pattern was missing the ^$

I have updated the patch in #3144145-2: Consider replacing fnmatch usage → to fix the substring issue, use preg_quote, and for coding standards. I have removed the case insensitivity flag because fnmatch() wasn't originally being used with FNM_CASEFOLD. I think in some cases (windows platforms, maybe?) the original implementation using fnmatch() might have been case insensitive in some places and not others.

Most of the fnmatch() replacements in Drupal have not been a complete reimplementation of the shell-style patterns as regex, but this patch retains the wildcard functionality on * and ?, which I think is useful for the config_distro_ignore use case.

This patch refactors isActiveStorageContext() to make use of Drupal\Component\DependencyInjection\ReverseContainer for reverse lookup of the storage service. Since this branch is currently cites compatibility with D9 in general, I have attempted to make the change backwards compatible, with the following approaches:

• In 9.5.1+, ReverseContainer will be used
• In 9.5.0, as ReverseContainer does not exist, getServiceIdMapping() will be used
• in 9.4 and below, it will fall back on the previous approach of the _serviceId property

At the point when only 9.5.1+ compatibility is needed, I think these deprecated fallbacks should probably be removed.

The patch includes the new test from @joegraduate thanks!

In my testing with 9.4.15, 9.5.0, and 9.5.1+ this seems to fix the original broken config_sync → test ( 📌 Address 3.0.x-dev test failures since Drupal 9.5.x Fixed ) that revealed this issue.

testUpdateModeFullReset() in config_sync is failing correctly; I think I have traced the reason for the failure to an apparent issue in config_normalizer related to a change in 9.5.x related to the _serviceId property:

🐛 isActiveStorageContext() fails in 9.5.x Fixed

tadean → created an issue.

I think this looks pretty promising, and seems to address the test failures. Setting this to RTBC.