Datetime module missing view plugin config schema

Little confused why this wasn't caught when the strict config checking got enabled.

> Little confused why this wasn't caught when the strict config checking got enabled.

Me too.

We noticed this in a custom install profile, where we run config schema validation on all our default configuration.

I think this kind of works because views has wildcard schema definitions for most of its things, for example this:

views.sort.*:
  type: views_sort
  label: 'Default sort'

So it works as long as you don't add special settings keys.

In our case it failed because of the missing schema for the granularity setting in the sort plugin.

The test fails are because the type definition is wrong in the current patch, we're working on that.

+++ b/core/modules/datetime/config/schema/datetime.views.schema.yml
@@ -0,0 +1,32 @@
+views.argument.datetime_year:
+  type: views.argument.date_year

I am not 100% convinced of these changes: \Drupal\datetime\Plugin\views\argument\YearDate extends \Drupal\datetime\Plugin\views\Argument\Date and not \Drupal\views\Plugin\views\argument\YearDate

Assigning to @dawehner for final review. I think these look good at this point.

+++ b/core/modules/datetime/tests/modules/datetime_test/test_views/views.view.test_sort_datetime.yml
@@ -33,6 +33,7 @@ display:
+          granularity: second

This is fairly minimal test coverage, somebody could remove this and everything would still be green and we'd have no indication that we lost coverage.
Would probably be a good idea to have a test that verifies that this granularity is set, that way we'd have a failing test if somebody removed the setting from the view.

Also it only 'tests' the sort schema, not sure what type of coverage is expected for the other schema.

I agree with @Lendude's assessment of the fragility of this test, so marking NW to work for #14.

Just marked #2721339: Missing schema for field_date_value as a duplicate. There was the start of some more explicit tests there.

I started a test, but I can't get one to fail..

This does the trick for me.

I think this is good to go.

Promoting to major since this soft-blocks a major: #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields

Adding review credits

Fixed on commit

diff --git a/core/modules/datetime/tests/src/Kernel/Views/DateTimeSchemaTest.php b/core/modules/datetime/tests/src/Kernel/Views/DateTimeSchemaTest.php
index 7641c33..f763c49 100644
--- a/core/modules/datetime/tests/src/Kernel/Views/DateTimeSchemaTest.php
+++ b/core/modules/datetime/tests/src/Kernel/Views/DateTimeSchemaTest.php
@@ -17,7 +17,7 @@ class DateTimeSchemaTest extends DateTimeHandlerTestBase {
   /**
    * {@inheritdoc}
    */
-  public static $testViews = ['test_argument_datetime','test_filter_datetime', 'test_sort_datetime'];
+  public static $testViews = ['test_argument_datetime', 'test_filter_datetime', 'test_sort_datetime'];
 
   /**
    * Test argument plugin schema.

Committed 93045af and pushed to 8.5.x.

Leaving at RTBC and moving to 8.4 for possible backport.

$ git pull
$ git checkout 8.4.x
$ git cherry-pick 93045af
$ git diff origin/8.4.x > 2895544-26.patch

Just uploading this so we have runs against 8.4.x

This is currently a major bug with a clean cherry-pick, so I think this should be good against 8.4.x. I can't think of a potential disruption?

So if someone somehow had a view that had these saved in a different format than what the schema requires, or a child instance that didn't comply with the new schemata, what would happen?

Adding credit for @kgoel who worked on the duplicate. (It's helpful when closing duplicates to add a list of contributors from the duplicate to the summary so committers can credit them too.) Thanks!

What is the case for backporting it? That it's needed for the other major bugfixes like #2786577: The Views integration Datetime Range fields should extend the views integration for regular Datetime fields?

So if someone somehow had a view that had these saved in a different format than what the schema requires, or a child instance that didn't comply with the new schemata, what would happen?

I *think* it would be casted on the next save / import, but if directly validated, it will not be valid anymore. Would be good to verify though.

Let's manually test for #31 before we backport this, to see what happens and evaluate the risk. Depending on what happens we might also consider an upgrade path?

Also probably should have a CR, and ideally the CR would include information on the impact for existing data as well.

Thanks!

Case for backporting is that is causes contrib/end product config schema tests to fail, e.g. granularity on a datetime sort. Sticking with failing tests, removing test coverage or having to patch core until 8.5.0 is not a good alternative.

I'm fine with backporting if somebody can verify #31 manually as @xjm requested in #32.

Only critical fixes will be backported to 8.4.x anymore, so closing this and setting back to 8.5.x.

I could not find any existing change records for similar changes in the past, IMHO this doesn't need one. We didn't *change* the schema, we added missing schema. Anyone we already validates his schema would have had fails before no matter what type they were.

Feel free to re-open if you disagree but nothing happened in 2 month here so I doubt it will if we re-open.