Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webflo’s picture

FileSize
61.86 KB

Status: Needs review » Needs work

The last submitted patch, 1: 2414539-1.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
62.11 KB
561 bytes

Found another sequence in views.data_types.schema.yml

webflo’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 3: 2414539-2.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
62.79 KB
966 bytes

Status: Needs review » Needs work

The last submitted patch, 6: 2414539-6.patch, failed testing.

vijaycs85’s picture

+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.

webflo’s picture

Status: Needs work » Needs review
FileSize
923 bytes
63.69 KB

Rerolled and fixed the two failures in filter module.

webflo’s picture

Issue tags: +DX, +Configuration schema
Gábor Hojtsy’s picture

I 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.

tstoeckler’s picture

If this is deemed to break-y I think we should implement this is a BC-compatible way:
I.e. change

$definition = isset($this->definition['sequence']) ? $this->definition['sequence'] : array();

to

if (isset($this->definition['sequence'])) {
  $definition = $this->definition['sequence'];
}
else {
  // BC-layer.
  $definition = isset($this->definition['sequence'][0]) ? $this->definition['sequence'][0] : array();
}

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...

Gábor Hojtsy’s picture

I 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.

tstoeckler’s picture

Hmm... 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.

Gábor Hojtsy’s picture

Status: Needs review » Needs work

Instead 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.

michaellenahan’s picture

Status: Needs work » Needs review

I'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

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because ...

The existence of faulty dash '-' characters in schema yml files is a bug.

Prioritized changes The main goal of this issue is usability and improved developer experience.

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.

Disruption Disruption will exist for those who have created schema yml files already.

The disruption may be mitigated by some simple backward compatability code.

Gábor Hojtsy’s picture

@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.

Gábor Hojtsy queued 9: 2414539-9.patch for re-testing.

Gábor Hojtsy’s picture

Issue tags: +sprint

Status: Needs review » Needs work

The last submitted patch, 9: 2414539-9.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Issue tags: -Needs beta evaluation

@webflo: wanna roll a new version?

I added a more spelled out beta evaluation in the summary.

webflo’s picture

Status: Needs work » Needs review
FileSize
61.01 KB

Status: Needs review » Needs work

The last submitted patch, 22: 2414539-22.patch, failed testing.

webflo’s picture

Status: Needs work » Needs review
FileSize
63.53 KB
Gábor Hojtsy’s picture

Issue summary: View changes

Added a change notice draft at https://www.drupal.org/node/2442603

Gábor Hojtsy’s picture

Issue summary: View changes

Added 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.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Let's see what core committers have to say on this then.

tstoeckler’s picture

Awesome, 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.

Gábor Hojtsy’s picture

Issue summary: View changes

See summary update then :)

tstoeckler’s picture

Thanks a lot!!!!

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

What do people think about supporting both syntaxes? So we don't break every module that has schema already?

Something like:

    $definition = isset($this->definition['sequence'][0]) ? $this->definition['sequence'][0] : isset($this->definition['sequence']) ? $this->definition['sequence'] : array();

in Drupal\Core\Config\Schema\Sequence

I love the change it really does make it easier to grok a schema.

Berdir’s picture

That 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?

alexpott’s picture

Also people are going to need to be eagle-eyed to spot the difference in the before and after sections in the CR

alexpott’s picture

Category: Bug report » Task
Status: Needs review » Needs work

Thinking 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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
708 bytes
63.74 KB
1.5 KB

Here we go.

The last submitted patch, 35: 2414539-schema-sequence-cleanup-35-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 35: 2414539-schema-sequence-cleanup-35.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
63.67 KB

sigh... Updating schema error.

Gábor Hojtsy’s picture

Also people are going to need to be eagle-eyed to spot the difference in the before and after sections in the CR

@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?) :/

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @vijaycs85! #35 neatly proves that the added "test" is in fact sufficient.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

The 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

Gábor Hojtsy’s picture

Updated 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

webflo’s picture

Status: Needs work » Needs review
FileSize
63.72 KB
834 bytes
tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs issue summary update

Can we get more explicit sequence_bc testing in ConfigSchemaTest::testConfigSaveWithSchema(). We need to add an entry to $untyped_values and $typed_values.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
64.46 KB
1.93 KB

Nice 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.

vijaycs85’s picture

FileSize
64.39 KB
1 KB

or this. Personally I prefer #46.

pingers’s picture

+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

vijaycs85’s picture

Thanks @pingers. Updated the link.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Yes, I like #46 better as well. Thanks @vijaycs85!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I 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.

  • alexpott committed 28e35f6 on 8.0.x
    Issue #2414539 by webflo, vijaycs85: Simplify schema definition for...
Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Status: Fixed » Closed (fixed)

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

hass’s picture

Status: Closed (fixed) » Needs work

https://www.drupal.org/node/1905070 is not updated to the 1.4 sheet and has incorrect sequence documentation.

hass’s picture

Status: Needs work » Closed (fixed)

Damn it. I found any other doc page with 1.3 but cannot find it now.

vijaycs85’s picture

thanks @hass, updated the document.