Problem/Motivation

#2316909: Revisit all built-in test/default views configuration in core rebuilt all the exported views, but it also showed some problems of the views schema:

  1. views.field.boolean:not should be a boolean, not a string.
  2. views_query defines schema not for the base class: QueryPluginBase, but actually for the sql implementation. Let's add an try for the sql query plugin,
    so contrib ones, can define their schema.
  3. views_query:query_comment should be a string, not a boolean
  4. views.field.field:delta_limit, delta_offset should both be integers, not strings, the delta_limit 'all' special value should be represented as 0 (zero)
  5. views_display:defaults:* should all be boolean, some of strings
  6. views_field:alter:max_length is missing, should be an integer

Proposed resolution

Fix them, see problem/motivation

Remaining tasks

Commit.

User interface changes

None.

API changes

Views schemas are fixed as per the above.

Files: 
CommentFileSizeAuthor
#62 interdiff.txt4.98 KBGábor Hojtsy
#62 2380457-views-schema-update-59.patch61.57 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,070 pass(es). View
#58 interdiff.txt1.61 KBGábor Hojtsy
#58 2380457-views-schema-update-58.patch58.93 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,030 pass(es). View
#55 interdiff.txt32.14 KBGábor Hojtsy
#55 2380457-views-schema-update-54.patch59.46 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,057 pass(es). View
#53 interdiff.txt1.06 KBGábor Hojtsy
#53 2380457-views-schema-update-53.patch58.78 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,048 pass(es). View
#48 interdiff.txt958 bytesGábor Hojtsy
#48 2380457-views-schema-update-48.patch58.02 KBGábor Hojtsy
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,036 pass(es). View
#47 2380457-views-schema-update-47.patch57.93 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,005 pass(es), 1 fail(s), and 0 exception(s). View
#44 interdiff.txt722 bytesGábor Hojtsy
#44 2380457-views-schema-update-44.patch58.18 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,000 pass(es), 1 fail(s), and 0 exception(s). View
#43 interdiff.txt4.39 KBGábor Hojtsy
#43 2380457-views-schema-update-43.patch57.48 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,047 pass(es), 1 fail(s), and 0 exception(s). View
#41 interdiff.txt43.94 KBGábor Hojtsy
#41 2380457-views-schema-update-41.patch56.04 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,930 pass(es), 10 fail(s), and 0 exception(s). View
#38 2380457-views-schema-update-37.patch12.11 KBGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,910 pass(es), 104 fail(s), and 0 exception(s). View
#37 2380457-views-schema-update-37.patch0 bytesGábor Hojtsy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View
#24 2380457-views-schema-update-24.patch12.11 KBolli
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2380457-views-schema-update-24.patch. Unable to apply patch. See the log in the details link for more information. View
#21 2380457-2380457-views-schema-update-21.patch12.4 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,027 pass(es), 102 fail(s), and 0 exception(s). View
#19 2380457-views-schema-update-19.patch12.37 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2380457-views-schema-update-19.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Gábor Hojtsy’s picture

Are you sure this is a Novice?

dawehner’s picture

Issue tags: -Novice

Ehem, its probably not. I did not expected to find so many of those problems, sorry.

vijaycs85’s picture

Status: Active » Needs review
FileSize
3.59 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,736 pass(es), 55 fail(s), and 1 exception(s). View

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.

vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Issue summary: View changes
vijaycs85’s picture

Title: Some fixes of the views schema » Some fixes of the views config & config schema
Issue summary: View changes
vijaycs85’s picture

Title: Some fixes of the views config & config schema » Some fixes of the views config schema
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 3: 2380457-views-schema-update-3.patch, failed testing.

Gábor Hojtsy’s picture

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
12.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,794 pass(es), 43 fail(s), and 1 exception(s). View
8.77 KB

Updating views according to schema changes.

Status: Needs review » Needs work

The last submitted patch, 10: 2380457-views-schema-update-10.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views/config/schema/views.query.schema.yml
@@ -1,5 +1,24 @@
 views.query.views_query:
-  type: views_query
+  type: mapping
   label: 'Views query'

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

Gábor Hojtsy’s picture

Are all changes covered by tests?

vijaycs85’s picture

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

Gábor Hojtsy’s picture

Issue tags: +Needs tests
Gábor Hojtsy’s picture

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +CapgeminiDrupalDay2014
FileSize
12.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2380457-views-schema-update-19.patch. Unable to apply patch. See the log in the details link for more information. View

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

Status: Needs review » Needs work

The last submitted patch, 19: 2380457-views-schema-update-19.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
12.4 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 82,027 pass(es), 102 fail(s), and 0 exception(s). View

Reroll of #10

Status: Needs review » Needs work

The last submitted patch, 21: 2380457-2380457-views-schema-update-21.patch, failed testing.

dawehner’s picture

That is postponed on #2331793: Changing pager settings for this display only also changes pager settings for other display

+++ b/core/modules/views/config/schema/views.query.schema.yml
@@ -1,5 +1,24 @@
 views.query.views_query:
-  type: views_query
+  type: mapping
   label: 'Views query'
+  mapping:
+    query_comment:
+      type: string
+      label: 'Query comment'

Urgs, someone seriously should consider to rename the sql based query plugin.

olli’s picture

Status: Needs work » Needs review
FileSize
12.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2380457-views-schema-update-24.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 24: 2380457-views-schema-update-24.patch, failed testing.

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

@vijaycs85: still working on this one?

Status: Needs review » Needs work

The last submitted patch, 24: 2380457-views-schema-update-24.patch, failed testing.

jibran’s picture

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

Berdir’s picture

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

jibran’s picture

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

catch’s picture

Priority: Major » Critical

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.

Gábor Hojtsy’s picture

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?

catch’s picture

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.

Gábor Hojtsy’s picture

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

catch’s picture

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
0 bytes
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Setup environment: Test cancelled by admin prior to completion. View

Rerolled as a start.

Gábor Hojtsy’s picture

FileSize
12.11 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,910 pass(es), 104 fail(s), and 0 exception(s). View

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

The last submitted patch, 37: 2380457-views-schema-update-37.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 38: 2380457-views-schema-update-37.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
56.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 85,930 pass(es), 10 fail(s), and 0 exception(s). View
43.94 KB

Fixed incorrectly boolean query_comment and incorrectly string max_length values found.

Status: Needs review » Needs work

The last submitted patch, 41: 2380457-views-schema-update-41.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
57.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,047 pass(es), 1 fail(s), and 0 exception(s). View
4.39 KB

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.

Gábor Hojtsy’s picture

FileSize
58.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,000 pass(es), 1 fail(s), and 0 exception(s). View
722 bytes

Adding schema for query_test too.

The last submitted patch, 43: 2380457-views-schema-update-43.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 44: 2380457-views-schema-update-44.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
57.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,005 pass(es), 1 fail(s), and 0 exception(s). View

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.

Gábor Hojtsy’s picture

FileSize
58.02 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,036 pass(es). View
958 bytes

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

The last submitted patch, 47: 2380457-views-schema-update-47.patch, failed testing.

Gábor Hojtsy’s picture

Issue summary: View changes

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

Gábor Hojtsy’s picture

Issue summary: View changes

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

Gábor Hojtsy’s picture

Issue summary: View changes

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.

Gábor Hojtsy’s picture

Issue summary: View changes
FileSize
58.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,048 pass(es). View
1.06 KB

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.

tstoeckler’s picture

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

Gábor Hojtsy’s picture

FileSize
59.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,057 pass(es). View
32.14 KB

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

dawehner’s picture

grr ... d.o. protected when you just post a comment

We discussed about delta_limit and IMHO we could make it a integer on a follow up, by assigning 0 or -1 as the 'all' value.
In case we do that we maybe also want to allow to setup this up using by extending the existing select field.

  1. +++ b/core/modules/views/tests/modules/views_test_data/config/schema/views_test_data.views.schema.yml
    @@ -63,7 +63,7 @@ views.display.display_no_area_test:
     views.query.query_test:
    -  type: views_query
    +  type: mapping
       label: 'Views query test options'
       mapping:
    

    ... Technically this should be certainly a views query, shouldn't it, just by looking at the class hierarchy

Gábor Hojtsy’s picture

Status: Needs review » Needs work

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
58.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,030 pass(es). View
1.61 KB

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

Gábor Hojtsy’s picture

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

dawehner’s picture

Thank you for adapting it!

In an ideal world I would -1, because this is what other people use for unlimited (aka. other constant) but I think its fine for now.
Opened a related description follow up, see #2420865: Add a description for views field to explain the magic "0/all" values

dawehner’s picture

@Gábor Hojtsy
No idea how I managed to kill your files, do you mind to reupload them?
Does the needs tests tag still apply to this issue?

Gábor Hojtsy’s picture

FileSize
61.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 86,070 pass(es). View
4.98 KB

Reuploading #59 patches which d.o ate.

Gábor Hojtsy’s picture

Issue tags: -Needs tests

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

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Fair.

YesCT’s picture

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

dawehner’s picture

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

Well in that case this is the symptom which happens if you accidentally remove a file, no idea though how this happens, to be honest.

alexpott’s picture

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

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

That looks like it has been resolved.

Gábor Hojtsy’s picture

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

Done that. Sorry for not having it updated.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • webchick committed e45522c on 8.0.x
    Issue #2380457 by Gábor Hojtsy, vijaycs85, olli, dawehner: Some fixes of...
Gábor Hojtsy’s picture

Yay, thanks. Published change record.

Status: Fixed » Closed (fixed)

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