See the change record: New orderby key added to configuration schema for sequences.
It is recommended to add an orderby setting to all sequence definitions in config schemas. key should be used if it's an associative array, value otherwise (then the (numeric) keys will be discarded/re-assigned).
We should also go through all our included configs (especially in the Defaults module, but also in the test modules) to ensure all sequences are properly ordered there, too. Otherwise, re-saving them would lead to invalid change notices for people.
| Comment | File | Size | Author |
|---|---|---|---|
| #11 | 2860167-11--add_orderby.patch | 14 KB | drunken monkey |
Comments
Comment #2
drunken monkeyThis patch adds alphabetic sorting to the "Fields" tab. I think we should add this alongside the new "orderby" keys, since sorting by label will usually be more useful to users than sorting by key.
Maybe we should move the compare method to the fields helper service, though. Might be useful in other spots, too.
Comment #3
drunken monkeyIdea for a test to be added here: verify that re-saving our included configs (at least for the public/non-test modules) won't change the export. This should fail if we forgot to properly sort some sequence.
Comment #4
vasikeI'm not sure about the relation of the new "orderby" and the previous patch.
However i re-rolled the previous patch and add the new "orderby" into schemas.
I think based on the changes (to be done) found, there could be completions on things to do on the issue.
So still needs work (needs "documentation" first).
Comment #5
drunken monkeyThanks for the patch!
However, it doesn't seem to apply for me. Did you maybe base it on a patched version of the module? My patch in #2, on the other hand, still applies fine, so it doesn't seem to have needed a re-roll.
Furthermore, you have used
orderby: valuein a lot of places where we needkeyinstead, because it's actually an associative array. E.g.,view_mode, which is keyed by entity type and bundle (needs a description update, too, I guess).Finally, I don't really understand what you mean with the last paragraph. Could you explain again?
Comment #6
drunken monkeyComment #7
borisson_Setting this back to active, we shouldn't worry about pre 8.4 code anymore. If anyone would like to take this on, the issue summary should be sufficiently clear in what we need to do.
Comment #8
drunken monkeyAh, yeah, thanks for thinking of this!
Comment #9
drunken monkeyFinished patch attached, but could probably use some extensive manual testing, since more or less each of these
orderbys could break something if it's the wrong setting.Interdiff compared to #2.
Comment #11
drunken monkey*cough*
Comment #12
borisson_The tests seem to pass, but should we add a change notice or a db-update for this?
Everyone that has this patch applied (or updates to a new version when this is committed) will need to export their configuration. So we need to do something to tell everyone to do that. If they don't do it after an upgrade it will be a big surprise when they actually change something else and export config just to see a bunch of search api config change.
Comment #13
acbramley commentedClosed #2891234: config export of a search index has an unreliable order for plugins as a duplicate.
Considering index config changes after every save in the current state (for me processors always get reordered making it extremely hard to figure out what's changed) I feel like an update hook isn't that necessary, however it could be nice :)
Comment #14
drunken monkeyI'm prepared to be outvoted, but I also don't think an update hook is necessary. The change record might be a good idea, though – can't hurt, at least. Something like this, maybe?
Although the original Core change record also wasn't really aimed at site admins, which were probably hit a lot more by that change back then.
Comment #15
fenstratCR looks good. I'd also vote that a update hook isn't really necessary.
We're using this on a couple production sites and it's working well, makes reviewing new search_api config change so much easier!
Comment #17
drunken monkeyOK, then it's settled – a change record is enough.
Committed.
Thanks for reviewing/testing, everyone!