Problem/Motivation

Should be usable via Drush.

Proposed resolution

The current patch introduces a Drush 9 class.

Remaining tasks

User interface changes

API changes

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

flocondetoile’s picture

Version: » 8.x-1.x-dev
Status: Active » Needs review
FileSize
6.14 KB

Hello,

Attached a patch which add support for Drush. Adding this support permit me to find some little bugs for unsafe configuration changes. See #2703277: Unsafe configuration never listed and/or updated
Thanks for your review

nedjo’s picture

Looks great, thanks!

I'm trying to find time for major refactoring in #2615146: Analyze diffs to do safe synchronization of customized configuration and #2678946: Repurpose core's config synchronization, so I'm slightly unsure of how that would affect these commands, but I'm comfortable meantime adding these.

james.williams’s picture

FileSize
6.16 KB

The previous patch didn't apply well -- here's a fixed version that creates the drush file afresh.

nedjo’s picture

Status: Needs review » Needs work

Didn't get this applied before major refactoring in #2793027: Refactor to use Configuration Provider and #2678946: Repurpose core's config synchronization. Will now need to be reworked to use new approaches

james.williams’s picture

Exciting stuff, thanks for doing that work! Is there any other related work likely to come in the next few days/weeks that is worth holding out for first? I wouldn't want to go through refactoring this twice within a short space of time.

Just to document it somewhere too whilst I'm here, since my last patch, I've also been thinking that it would be good to have an option to the drush command like --module-dir, to allow 'reverting' all modules within/below a certain directory (e.g. within an install profile's modules/custom directory, which is my use case), to replace what we used to do with 'drush fra'.

nedjo’s picture

Is there any other related work likely to come in the next few days/weeks that is worth holding out for first?

I think the major refactoring is done.

I've also been thinking that it would be good to have an option to the drush command like --module-dir, to allow 'reverting' all modules within/below a certain directory

Two relevant changes in the recent refactoring.

First, I've removed the code that was specific to a particular extension. Instead, all changes from all extensionns are merged all at once.

The main reason for this change is dependency handling. Often, particularly with features, a given module's configuration may depend on that provided by other modules, so updates are best done as a set.

Also, for directories other than config/install, changes in a particular module may trigger changes in other modules. For example, configuration provided by Module A may satisfy config dependencies of Module B's config/optional items, meaning, again, that updates are best done as a set.

Second, this module can no longer be used for reverting in the sense we used the term in D7. Instead, all changes are merged into the active configuration, retaining any overrides that have been made.

The initial Drush work seems to be two commands, one to initialize the merged configuration storage and one to run imports. Because we're reusing core's config sync importing, the second should be very similar to what's already in drush config-import.

james.williams’s picture

Ah. Ok - thanks for the update. Unfortunately I was using config_sync for the very use case of 'reverting' individual modules, or groups of modules. I do understand that dependencies can be an issue with that kind of workflow, but we're working to keep on top of those to ensure our config declares its dependencies appropriately. At least in our workflow with config_sync before the refactor, dependencies were recognised & processed, so issues could be avoided if they were at least declared accurately enough. So I'll look at what options we have for our intended workflow - it's likely that our focus will shift to other projects since you have a clear intention for config_sync now, that looks great, but just doesn't match what we were trying to achieve.

nedjo’s picture

@james.williams: I think it's worth at least exploring if/how config_sync could continue to meet your use case. I've just posted two issues to try to capture at least most of what would be needed:

I'm definitely open to doing this and at first glance it doesn't look prohibitively difficult. Could you look over the two issues and see if they capture what you'd need?

james.williams’s picture

@nedjo thanks for your understanding. In reality, I'm waiting to see where #2388833: Pull reusable routines out of configuration import/export form builders (core) and #2388253: Pull reusable config handling into new service (config_devel) end up going, as it looks to me as those will direct any workflow I'm aiming to use. Nothing against your module(s) or the direction they're heading, they just don't quite fit and I'd rather stay closer to core + config_devel (since that's virtually core for most of us, I expect!) where possible.

So for now, rather than adjust to using config_provider/config_sync, which never quite fit our usage/intention correctly anyway, I've rolled my own drush command to cover the 'features revert' / 'revert all' style workflow that we used to use: http://github.com/computerminds/cm_config_tools - this also incorporates ideas taken from elsewhere like https://www.previousnext.com.au/blog/introducing-drush-cmi-tools and leverages common functionality in config_update too.

Sorry to not take this any further!

nedjo’s picture

@james.williams: thanks for the update. Even if it doesn't end up meeting your own requirements, this conversation's been a useful spur to improvements here in config_sync, so it's all good.

james.williams’s picture

yay :-)

I'll certainly continue to keep an eye on developments in config_sync!

pfrenssen’s picture

FileSize
3.81 KB

I started working on a new drush integration that will work with the new approach in Config Sync, and with Drush 9. This isn't ready yet. The listing of config is done, but the update command is only partly implemented. I need to look into how I can run a batch process to apply the config updates from the merged config storage.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
6.34 KB

I completed the Drush commands by getting inspiration from the core config import commands.

nedjo’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

This looks great!

+++ b/src/Commands/ConfigSyncCommands.php
@@ -0,0 +1,161 @@
+  public function syncUpdate($options = ['retain-overrides' => FALSE]) {

For 'retain-overrides', in ConfigSyncInitializerInterface, the corresponding parameter is named $retain_active_overrides and the default value is TRUE. It also defaults to TRUE in the UI:

      $form['retain_active_overrides'] = [
        '#type' => 'checkbox',
        '#title' => $this->t('Retain customizations'),
        '#default_value' => TRUE,
        '#description' => $this->t('If you select this option, configuration updates will be merged into the active configuration, retaining any customizations.'),
      ];

We should be consistent with the naming and default value. Since we're in alpha it's fine to change the interface.

Let's use 'retain-overrides' in Drush and change retain_active_overrides to retain_overrides in the interface and elsewhere in the code base. It's shorter to enter at the Drush command line.

For the default value, there are probably two main use cases for Configuration Synchronizer. The first is to safely merge configuration changes from extensions into the active configuration. In this use case, the desired value is TRUE. The second is to revert the site's configuration to the current state of extension-provided versions. In this use case, the desired value is FALSE.

Since I wrote the module primarily for the first use case, I put it as the default. It may turn out use case 2 is equally or more used, but it's probably too early to tell. For now I suggest we switch the default to TRUE in the drush command.

nedjo’s picture

Status: Reviewed & tested by the community » Needs review
pfrenssen’s picture

Very good suggestion, we should default to retaining the overrides. We have to flip the polarity of the option though, since it is a boolean option it is typical in POSIX to not assign a value to it, but to just include / omit the option. I think the reason for it is because it can be confusing to remember the value to turn it off: it can be "no", or "0" or "FALSE", ...

So instead of something like this:

$ drush cs-update --retain-overrides=no

It would probably be better to do the inverse, something like this:

$ drush cs-update --discard-overrides

What do you think?

nedjo’s picture

Agreed, that sounds like the way to go.

pfrenssen’s picture

FileSize
6.51 KB
1.29 KB

Flipped the polarity of the option.

nedjo’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Nice, thanks! This will fill a major gap in the module. Unless there are other pending changes, once this is committed, we should put out a new alpha release.

  • pfrenssen committed 142baeb on 8.x-1.x
    Issue #2445463 by pfrenssen, james.williams, flocondetoile: Add Drush...
pfrenssen’s picture

Status: Reviewed & tested by the community » Fixed

Thanks! I've committed the patch and had a look through the open issues but there is nothing in RTBC or needs review any more, so this is indeed a good moment for rolling a new release. I'll take care of it.

nedjo’s picture

Thanks! I've added a note about using Drush to the project page. Please feel free to refine that, and also to add your company to the "supporting organizations" for the project.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.