Beta phase evaluation
Issue category | Bug because the extra nested level in schemas for sequences is arbitrary. You can only define one element. It is a syntax gimmick that makes schema writing harder to understand. |
---|---|
Disruption | Disruptive for contributed modules which already include schemas. The change is very easy to make though. There is no sizable work ongoing on core schemas, so no disruption expected for other core patches. |
Problem/Motivation
The config schema definitions for sequences are unnecessary nested and it is confusing in comparison to the mapping element. It is true that in mappings, each element has its own type defined and in sequences this nested list item represents that you define the type/label for the item, but this was found to be confusing because it suggests there may be other item types defined, which is not the case. We can describe the data with a simpler construct.
Proposed resolution
Remove the nested list level.
Remaining tasks
None.
User interface changes
None
API changes
Sequences will need to be defined in a simpler way. See https://www.drupal.org/node/2442603 for change notice.
Comment | File | Size | Author |
---|---|---|---|
#47 | 2414539-diff-46-47.txt | 1 KB | vijaycs85 |
#47 | 2414539-schema-sequence-cleanup-47.patch | 64.39 KB | vijaycs85 |
Comments
Comment #1
webflo CreditAttribution: webflo commentedComment #3
webflo CreditAttribution: webflo commentedFound another sequence in
views.data_types.schema.yml
Comment #4
webflo CreditAttribution: webflo commentedComment #6
webflo CreditAttribution: webflo commentedComment #8
vijaycs85+1 to the idea as sequence itself means no index and '-' does the same. However not very sure how this would affect all the places where we've sequence.
Comment #9
webflo CreditAttribution: webflo commentedRerolled and fixed the two failures in filter module.
Comment #10
webflo CreditAttribution: webflo commentedComment #11
Gábor HojtsyI agree this is a DX improvement. I am not sure about the impact vs. disruption. (In other words if it should be done "this late"). Should have a beta evaluation to outline that.
Comment #12
tstoecklerIf this is deemed to break-y I think we should implement this is a BC-compatible way:
I.e. change
to
Not actually advocating for that because I think the current implementation is just a bug, that just happens to work. Of course, I cannot argue with the fact that this will break config schemas, so...
Comment #13
Gábor HojtsyI think with such BC layer, config schemas would be either this way or that way which will confuse the hell out of people. They would need to learn both syntaxes to understand schemas. And at that point having one (more obscure) syntax is better compared to having two of which one is more obscure.
Comment #14
tstoecklerHmm... I disagree. If we change all the core implementations there will not be "two syntaxes". People copy from core anyway. There will be some contrib definitions that follow the old pattern, but either people do not notice it or they copy-paste that, which will also work. I don't think it's fair to call this "learning two syntaxes". I'm willing to bet that most people will not even notice the difference unless specifically pointed to it. In fact if people actually tried to understand and learn the schema structure this issue would have been opened and fixed ages ago, because the current way is simply wrong.
Just my two cents.
Comment #15
Gábor HojtsyInstead of us trying to argue about it, core commiters will need to convinced, most likely @alexpott, because this is "his area". The summary needs a beta evaluation and then we can give this to Alex.
Comment #16
michaellenahan CreditAttribution: michaellenahan commentedI'm a developer with very little experience of Drupal 8, I came across this problem today in a D8 training workshop.
I would argue that the benefit of removing these dash characters '-' from the various schema files would be *enormous*. Why? Because it is very confusing to new learners of Drupal 8.
During the training workshop, my colleagues and I were curious to know why these dash characters exist in the schema files, always in the same position beneath 'sequence'.
Coming across this issue on drupal.org, we realised that it is simply an error that was made during D8 development.
It would be terrible if we had to wait until D9 for this to be fixed. As far as I can see, these dashes only exist in error in the schema files.
So, the entire model of how the schema files are built is broken, and for no good reason. I understand that it is late for such changes, but the sooner this error is corrected the better it will be for everybody.
Below, I've added an attempt at a beta phase evaluation.
Beta phase evaluation
The existence of faulty dash '-' characters in schema yml files is a bug.
The developer experience of encountering such a needless bug in such a fundamental part of the configuration system is awful.
If this isn't removed now, the problem will continue for years ahead, with developers actually being *forced* to write syntactically inconsistent schema yml files.
The disruption may be mitigated by some simple backward compatability code.
Comment #17
Gábor Hojtsy@tstoeckler, @michaellenahan: I would test the full-on change with no backwards compatibility on core committers first. I think it makes a lot more sense to not let people write schemas that are said to be so confusing, right? It would be really painful to understand those contribs especially if core does not use that format anymore.
Comment #19
Gábor HojtsyComment #21
Gábor Hojtsy@webflo: wanna roll a new version?
I added a more spelled out beta evaluation in the summary.
Comment #22
webflo CreditAttribution: webflo commentedComment #24
webflo CreditAttribution: webflo commentedComment #25
Gábor HojtsyAdded a change notice draft at https://www.drupal.org/node/2442603
Comment #26
Gábor HojtsyAdded this to the issue summary for balanced evaluation: It is true that in mappings, each element has its own type defined and in sequences this nested list item represents that you define the type/label for the item, but this was found to be confusing and we can describe the data with a simpler construct.
Comment #27
Gábor HojtsyLet's see what core committers have to say on this then.
Comment #28
tstoecklerAwesome, thanks!
Re #26: The comparison does not hold up, though, because we only support a single type of "thing" in a sequence, i.e. you can't have a thing of type "a" at index 0 and a thing of type "b" at index 1. This is what is suggested by having to specify a list explicitly in the schema, but it's not true.
Comment #29
Gábor HojtsySee summary update then :)
Comment #30
tstoecklerThanks a lot!!!!
Comment #31
alexpottWhat do people think about supporting both syntaxes? So we don't break every module that has schema already?
Something like:
in
Drupal\Core\Config\Schema\Sequence
I love the change it really does make it easier to grok a schema.
Comment #32
BerdirThat was discussed above, I guess the argument against this is that there might be a mix of both versions then in core/contrib, which would be confusing.
Possibly put that in the issue summary if it's not there yet?
Comment #33
alexpottAlso people are going to need to be eagle-eyed to spot the difference in the before and after sections in the CR
Comment #34
alexpottThinking about this and using our beta policy as a guide. This is not a bug - nothing is broken, it's just that something is awkward. As I said I think the change is a great improvement but I don;t think we should break d8 contrib for this. I think we should deprecate the old format and have a BC layer. The deprecation should be for 9.x. It is not onerous to support. We also need to add a test for the bc later.
Comment #35
vijaycs85Here we go.
Comment #38
vijaycs85sigh... Updating schema error.
Comment #39
Gábor Hojtsy@alexpott: right, that is the same reason they will be confused if different syntaxes are possibly mixed even in the same schema file. I think this change is valuable even with a BC layer added so definitely not work blocking because I don't like the BC layer idea, but nevertheless I did not grow to like it (yet?) :/
Comment #40
tstoecklerThanks @vijaycs85! #35 neatly proves that the added "test" is in fact sufficient.
Comment #41
alexpottThe beta evaluation is out of date. Also I think it is worth having a CR to say that the new format is preferred and that the old format will be deprecated in 9.x. It'd be great if somehow it was easy to spot the difference. Plus we should add an @todo in the code and create an issue postponed to 9.x
Comment #42
Gábor HojtsyUpdated change notice with backwards compatibility / depreciation info. Not sure a separate change notice for only that change would be shorter? It would still need to explain all the changes? Also made the sequence changes easier to spot with added line-end comments. There is not much else one can do in code tags. We can opt to not post this example with code tags maybe if we want to do formatting on it... https://www.drupal.org/node/2442603
Comment #43
webflo CreditAttribution: webflo commentedAdded the missing @todo to #2444979: Trigger deprecation notice for old schema definition for sequence
Comment #44
tstoecklerThanks!
Comment #45
alexpottCan we get more explicit
sequence_bc
testing inConfigSchemaTest::testConfigSaveWithSchema()
. We need to add an entry to$untyped_values
and$typed_values
.Comment #46
vijaycs85Nice one @alexpott. Seems ternary expressions aren't happy when add more than one which fails sequence_bc.
For more info, check example 4 of Ternary Operator section in PHP documentation - Non-obvious Ternary Behaviour.
Comment #47
vijaycs85or this. Personally I prefer #46.
Comment #48
pingers CreditAttribution: pingers commented+1 #46.
I think this is where you meant to link to about the ternary operator example - http://php.net/manual/en/language.operators.comparison.php#example-138
Comment #49
vijaycs85Thanks @pingers. Updated the link.
Comment #50
tstoecklerYes, I like #46 better as well. Thanks @vijaycs85!
Comment #51
alexpottI like #46 too - nice and explicit.
Committed 28e35f6 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #53
Gábor HojtsyUpdated docs at https://www.drupal.org/node/1905070/revisions/view/8039739/8224703
Comment #54
Gábor HojtsyAlso updated the cheat sheet to use the new format at http://hojtsy.hu/blog/2014-dec-12/drupal-8-configuration-schema-cheat-sheet, https://www.drupal.org/node/1905070/revisions/view/8224703/8224749 and https://www.drupal.org/node/2391795/revisions/view/8039745/8224753
Comment #56
hass CreditAttribution: hass commentedhttps://www.drupal.org/node/1905070 is not updated to the 1.4 sheet and has incorrect sequence documentation.
Comment #57
hass CreditAttribution: hass commentedDamn it. I found any other doc page with 1.3 but cannot find it now.
Comment #58
vijaycs85thanks @hass, updated the document.