Config export key order for sequences: "orderby" does not support cases where the order actually matters

I would always recommend that code to have a priority in their values, as this makes it way more explicit.

On the other hand you are absolutely right, order matters in sequences:

Unlike a set, order matters, and exactly the same elements can appear multiple times at different positions in the sequence

.

Is there anything missing from the change record at https://www.drupal.org/node/2852566 that would help to explain this issue better?

I've just tagged #2361539: Config export key order is not predictable for sequences, add orderby property to config schema for a release notes mention.

I don't see this as an API change, it's a change in behaviour for something that was never previously defined, so just documenting it seems fine.

The question I would like to ask here: did we really guaranteed the order before? I remember having quite some issues with that in views. We ended up switching to using weights instead. I'm curious why you never ran into those ordering problems before.

Reading the typed data definition:

 From the typed
 * data API perspective sequences are handled as ordered mappings without
 * metadata about existing properties.

this really makes it clear, that its not an unorded hashmap, like many other languages use by default.

Here is a question though: They code which does the sorting:

      if ($element instanceof Sequence) {
        $data_definition = $element->getDataDefinition();
        if ($data_definition instanceof SequenceDataDefinition) {
          // Apply any sorting defined on the schema.
          switch ($data_definition->getOrderBy()) {
            case 'key':
              ksort($value);
              break;

            case 'value':
              // The PHP documentation notes that "Be careful when sorting
              // arrays with mixed types values because sort() can produce
              // unpredictable results". There is no risk here because
              // \Drupal\Core\Config\StorableConfigBase::castValue() has
              // already cast all values to the same type using the
              // configuration schema.
              sort($value);
              break;

          }
        }

and

   public function getOrderBy() {
    return isset($this->definition['orderby']) ? $this->definition['orderby'] : NULL;
  }

don't imply any kind of sorting by default. Am I missing something obvious?

While my question in #10 is still outstanding, I think having an explicit way to enforce no sorting is helpful

@cottser, @cilefen, @effulgentsia, and I discussed this issue and agreed it is not critical and also not really a bug. There is no regression; core does not yet specify orderby everywhere -- #2855675: Add orderby key to all sequences in core has not had any activity at all -- and there is no hard requirement currently that orderby should be set.

When a specific ordering other than key or value is needed, it's currently the responsibility of the implementation to enforce that order on save or to add (e.g.) a weight that is independent of the configuration storage. I was going to state that the CR at https://www.drupal.org/node/2852566 indicated this, but it looks like that got omitted somehow. So, I think we should update the change record there with that information and maybe (as @effulgentsia suggested) an example of how an implementation might enforce custom ordering.

We also have this other followup still open: #2856336: Add docs for the new orderby key for sequences in configuration schema

As for the scope of this issue, it's really a request for an API addition. However, I'm not convinced that the current patch is the right approach. Arbitrary "insert order" is not something we should be encouraging because that could vary from one update to the next and create meaningless diffs and compromise the data integrity, which is what #2361539: Config export key order is not predictable for sequences, add orderby property to config schema tries to address in the first place. The fact is that arbitrary ordering has led to lots of data integrity issues in the past so we should encourage developers to rethink why they want this "insert" order in the first place.

We could go the go route which means that unless specified, iterating over a map happens in random order, so you cannot rely on it. In this case we would enforce that contrib/custom code sorts by some parameter ... which might be quite hard, but at least it prevents any other possible future bugs.

Well, that would guarantee a config export diff on every single export, which is the kind of thing we're trying to avoid. :) Also that carries its own set of problems for us: #2571183: Deprecate random() usage in tests to avoid random test failures.

I think we can maybe go the route of deprecating the null sort order and add an example of how to implement one's own sort.

Well, that would guarantee a config export diff on every single export, which is the kind of thing we're trying to avoid. :)

I think I was suggesting somehow a read only behaviour.

Updated title and issue description with correct key name (was 'orderedby'; corrected to 'orderby').

Updating tag

I think we should make it more explicit and call it
orderby: none.

because it is not a default order, it is the absence of sortability.
This would be already much better, because then we know where the sorting is explicitly not wanted or where it is not yet defined.

Clarifying a bit: if we add none as an option then we could do sorting by default by key if keys are non numeric and value if keys are integers in Drupal 11

given the isdues could be more sorted and organised, the plan is basically to add more options for orderby.
The core extensions need values_then_keys, others need weight yet others will need some more complex algorithm. I wanted to make it so one could add a php callback, but the wise alexpott remembered that this would make it impossible for external systems to manipulate the config.

So part of the challenge is to assess what sorting algorithms we need. And none could be an option. Then "consumers" of this schema information like Config Split can just treat unsortable config as a blob and not do anything with it.
The default could then be the most complex one which tries to be smart and detect all sorts of possible sorting options at the price of performance.

well yes of course!
The default could also be value if the keys are integers and key otherwise.
The goal should be that core defines orderby for all its sequences.

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This keeps coming up with so many projects, it's annoying to still deal with lines changing their ordering in config exports. I opened #3489389: Sort config alphabetically if no orderby defined as an alternative to this, because there should be a simply, predictable default to this, instead of having to manually specify it every time (as some have suggested here).

Crediting for triage as per #13.