Some fixes of the views config schema

Are you sure this is a Novice?

Here is an update for items in issue summary:

1. FIXED.
2. FIXED - we got null by default in default views, however null is a valid boolean I guess?
3. NOT FIXED - Agreed, we need to move all elements under views_query to views.query.views_query.
4. FIXED
5. FIXED
6. FIXED
7.
8.NOT FIXED - it is defined as boolean?

views.filter_value.boolean:
  type: boolean

9. FIXED - It's there(core/modules/views/config/schema/views.data_types.schema.yml:503), but as string instead of integer.

Found and opened #2381973: View wizard creates 'invalid' views out of the box, missing plugin_ids, insecure permissions.

Updating views according to schema changes.

Are all changes covered by tests?

... sorry but didn't we talked about moving this to sql?

it is in sql @dawehner, we removed the base data type views_query.

Are all changes covered by tests?

Nope, we just got the ConfigDefaultTest for now.

So the test views have a test at #2325269: Test and fix views in test_views directories against their configuration schema, let's move test view changes there? This issue can do the non-test view changes, which do have test coverage. The others have the coverage there.

I realise this changes schemas and adapts test views. However once #2325269: Test and fix views in test_views directories against their configuration schema lands, test views will actually have schema testing, so I would suggest to continue doing this issue once that lands, when all the changes will be covered by tests :) Win!

As for the pager_options boolean to sequence change here, that is actually a leftover in my understanding. It is being removed in #2331793: Changing pager settings for this display only also changes pager settings for other display (and previously in #2385107: Cleanup legacy [plugin]_options from views displays).

Parts of this may also be applicable in #2385805: Views tests don't pass strict schema checking which concerns itself for the fails reproducible with current tests, so not ones that this may fix that are not currently tested. I will keep an eye if this has good fixes for there in which case we found the test counterpart. Will take care to include commit credit suggestions as appropriate.

Ok, let's try the same patch in #10 again and fix the failed test cases?

Reroll of #10

#2331793: Changing pager settings for this display only also changes pager settings for other display landed, reroll.

@vijaycs85: still working on this one?

I have some fails here https://qa.drupal.org/pifr/test/956818 for my contrib style plugin which are related to views schema this http://www.privatepaste.com/497076a1f0 fixes the issue for me locally.
@dawehner can we add these to list? views.view.*:display.default.display_options.style.provider and views.view.*:display.default.display_options.fields.name.provider

Provider should no longer exist in your views, just remove it.

Thanks @Berdir and @Gábor Hojtsy it did fix the tests. https://qa.drupal.org/pifr/test/956898

I don't think we want to write an upgrade path for this, and I don't think we want broken Views schema until 9.x (i.e. if this ended up 'upgrade path deadline', so bumping to critical. Looks pretty close.

Are the things to fix collected "randomly" in the summary or is there some pattern to them? The issue title does not sound very confident that this is in some way exhaustive. Then we would need yet another issue critical to discover more of them? Or how do we plan to go about this?

Following on from #32, config schema affects default config (as shown in the patch), so changing it in a way that wouldn't be consistent with default configuration is something we should almost certainly not do after 8.0.0 since it could break contrib/custom modules and install profiles.

@catch: I do agree it would be better if these are fixed, I don't see the pattern yet and therefore its hard to see if this is exhaustive and how would we find others. Or I'm missing some stuff here.

@Gabor I'm having the same issue as you with that, not clear at all how we know whether we got everything or not.

Rerolled as a start.

More than 0 bytes are involved with this issue, but this was clearly the quickest way to get the issue to green...

Fixed incorrectly boolean query_comment and incorrectly string max_length values found.

Fixed delta_offset values where they were strings and should be ints.
Unfortunately delta_limit cannot be an int, unless you want to use null instead of 'all'. @dawehner?
Fixed places where not should be boolean not string.

Should still fail at least on one remaining query schema.

Adding schema for query_test too.

field_langcode_add_to_query was removed in #2394883: Language setup for entity and field based rendering in views is independent, confusing UI, lacking test coverage just now, so no need to fix it anymore :D Rerolled.

Oh, the query test schema was already defined but used an undefined base type. Fixing that instead.

Added remaining tasks based on what is not yet covered in the patch.

No, the pager_options one needs to go, it was resolved in #2385107: Cleanup legacy [plugin]_options from views displays.

As for views.filter.boolean:

views.filter.boolean:
  type: views_filter
  label: 'Boolean'

which is

views_filter:
  type: views_handler
  mapping:
    value:
      type: views.filter_value.[%parent.plugin_id]
      label: 'Value'
    ....

And then

views.filter_value.boolean:
  type: boolean

So this one looks all good, removing from the todo list.

And finally on views_display:defaults:* should all be boolean only display_description was incorrectly label typed there. Should be boolean. Let's see what will this one break.

Wow, awesome work everyone. Will try to review this later. For now, a shameless plug:
#2418481: Views more text cannot be translated

@dawehner looked at the patch and says max_length should be 0 where it is null now, and looking at its uses, makes sense. It does not have a unique null value to differentiate any condition.

re: delta_limit: if we are to change the type, we should do it here, this is critical so we can match the config to the PHP and the other way around without needing upgrade path stuff

re: views query, we removed the base type for views queries because the base class does not have any options supported; so we could add one empty mapping for data modeling's sake if you prefer, but there are no base options, so that is why the SQL one is also defined out of a mapping, not some base type

Restoring the views_query base type as an empty mapping as per @dawehner's suggestion.

Using 0 as the special case for delta_limit instead of 'all' as suggested.

Reuploading #59 patches which d.o ate.

We introduced the full-on schema testing which failed plenty above when we changed schema only, so I think that coverage is plenty.

I dont know why #59 didn't get sent to the testbot, but I added it as an example on the d.o issue #2420915: patches on issues not being sent to testbot for testing

views.field.field:delta_limit may be 'all' so left as string, discuss

That looks like it has been resolved.

Done that. Sorry for not having it updated.

Ok this one looks like an enormous pain in the patootie to re-roll, so let's get it in.

Committed and pushed to 8.0.x. Thanks!

Yay, thanks. Published change record.