Problem

  • This stack of operations breaks the originally intended goal of being able to diff configuration files via git/$vcs:
    $config = config('image.style.foo');
    
    # yields:
    # name: foo
    # effects:
    #   flow:
    #     upsidedown: 2
    #   slide:
    #     left: yes
    
    // Remove the flow effect.
    $config->clear('effects.flow');
    $config->save();
    
    # yields:
    # name: foo
    # effects:
    #   slide:
    #     left: yes
    
    // Decide to re-add a flow effect.
    $config->set('effects.flow', $effect);
    $config->save();
    
    # yields:
    # name: foo
    # effects:
    #   slide:
    #     left: yes
    #   flow:
    #     upsidedown: 2
    

    Hint: 'flow' was in a different spot before. Thus, this produces the diff:

    --- image.style.foo.yml
    +++ image.style.foo.yml
    -  flow:
    -    upsidedown: 2
       slide:
         left: yes
    +  flow:
    +    upsidedown: 2
    
  • Most of the configuration cannot rely on the defined order in config objects, because the order is arbitrary, so we're using 'weight' properties in sub-keys either way when data needs to be used in a certain order at runtime.

Goal

  • Find a solution that prevents the config files from being different, while holding identical data.
Files: 
CommentFileSizeAuthor
#9 config.next_.sortkeys.9.patch2.66 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.next_.sortkeys.9.patch. Unable to apply patch. See the log in the details link for more information. View
#2 drupal8.config-file-diff.2.patch699 bytessun
PASSED: [[SimpleTest]]: [MySQL] 36,586 pass(es). View

Comments

sun’s picture

Issue tags: +Configuration system
sun’s picture

Status: Active » Needs review
FileSize
699 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,586 pass(es). View

This would be one way to approach it.

beejeebus’s picture

yeah, that could work. though i'd prefer we recurse into $data, rather than stopping at 2 levels.

sun’s picture

Assigned: Unassigned » chx

Safely recursing into all nested keys that are arrays and sorting them by key (retaining keys and indexes/associations) is a typical question for @chx ;) (I wasn't able to figure out how that could work without a lame helper function)

beejeebus’s picture

this sort of lame helper function?

function sort_deep(&$unsorted) {
  ksort($unsorted);
  foreach ($unsorted as &$key) {
    if (is_array($key)) {
      sort_deep($key);
    }
  }
}
sun’s picture

yeah ;)

All of that being said: Doing this in FileStorage only means that we're adding a constraint to the config system, basically telling that no developer should rely on the order in which keys are returned, because the actual order of keys is different between FileStorage and DatabaseStorage.

This inherently means that a strict comparison like the following will never be TRUE:

FileStorage->read('image.style.foo') === DatabaseStorage->read('image.style.foo')

Of course, we could move the sorting into DrupalConfig::save(), so it applies to every storage. However, I almost guess that that would be kinda incompatible with the idea of clustering/splitting keys into separate records (if that's going to happen), since those multiple records would simply be retrieved/collated into one item/object, and any particular order wouldn't be retained.

catch’s picture

Couldn't we make the database backend to order by key ASC or something when it retrieves them?

sun’s picture

heh. That indeed sounds like the direct equivalent to the nested key sort! :)

So, let's move this into DrupalConfig::save() ?

sun’s picture

Assigned: chx » Unassigned
FileSize
2.66 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch config.next_.sortkeys.9.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, config.next_.sortkeys.9.patch, failed testing.

sun’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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

sun’s picture

Status: Closed (fixed) » Active

#1785560: Remove the sorting of configuration keys reverted this entire patch.

It's possible that we might be able to resolve a part of the problem via #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*, but certainly not for all config objects.

I'm therefore re-opening this issue to make sure the topic is not forgotten.

To quote myself from http://drupal.org/node/1785560#comment-6569464:

The fix from that [this] issue had the following effect:

  1. All keys in the entire config object are re-sorted on every save.
  2. Sorting removed the "feature" of native array index associations. It inherently required us to add weight: 0 sub-nodes for collections that have an explicit, defined order.
  3. At the same time, ensuring an always consistent key order was a key enabler for diff operations between config objects, allowing you to see the actual change instead of a node/collection being removed at one position and re-inserted in a different position.

This is just to explain the current code/architecture and reasoning behind it, so we understand what we're changing here. I'm all-in for removing the sorting.

However, I think we should re-open [this issue] then, because the issue is essentially not fixed anymore (the exact patch over there is rolled back), and we should probably try to find an alternate solution for the issue then, as I believe that the ability to produce clean diffs for config objects is a fundamental facility, which not only affects developers working with $VCS but also a potential Config UI (e.g., Features module had this capability since the very beginning).

One alternate solution is to "re-instantiate" a "supposed to be same" key order in config objects by clearing them out altogether before writing new data into them; i.e., #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*. That is, however, for dynamic config objects (Configurables) only, since only those have an additional business logic layer wrapped on top of them, which is supposed to be used for any updates to a corresponding config object; having the resulting impact of always storing config object keys in the order the properties are defined on a classed ConfigEntity object.

I feel uncomfortable with not addressing the resulting consequences of this patch here, but I'm happy with unblocking other issues and moving forward. For this compromise, I'd want to increase the priority of #1601168: Orphan/obsolete keys and sub-keys *within* configuration objects are retained *forever*, but alas, it's filed as a major bug already.

mtift’s picture

Issue summary: View changes

core/lib/Drupal/Core/Config/ConfigObject.php no longer exists (or was renamed). And I think we are solving this problem in #2227731: Normalize configuration data during config writes.

So I think we can close this issue?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.