Config schema of argument_default plugins is incorrect

Patch attached fixes the proposed solution.

+++ b/core/modules/node/config/schema/node.views.schema.yml
@@ -67,11 +67,8 @@ views.argument.node_vid:
 views.argument_default.node:
-  type: sequence
+  type: mapping
   label: 'Content ID from URL'
-  sequence:
-    type: string
-    label: 'Nid'
 

+++ b/core/modules/user/config/schema/user.views.schema.yml
@@ -53,7 +53,7 @@ views.argument_default.user:
 views.argument_default.current_user:
-  type: boolean
+  type: mapping
   label: 'User ID from logged in user'
 
 views.argument_default.node:

IMHO those expose any kind of configuration so why can't we just drop those entries entirely?

According to our discussion I think we should have views.argument_default.* and then remove the other pointless ones.

Hi tstoeckler/dawehner,
Could you suggest if "argument_default" in other modules also needed to be corrected.

No these are the only ones needed in Drupal core.

Reuploading the patch, though already created by Sagar Ramgade.

@see https://www.drupal.org/node/2623568#comment-10612250 for my previous feedback.

new patch considering comment 10612250

Updated patch.

#6 is still not addressed :(

Thank you @dawehner for reviewing it. is #6 says about to remove all instance of views.argument_default.* from user.views.schema.yml ?

No, its about adding a views.argument_default.* entry

Ok, so I'm not sure if this is right, but I have added views.argument_default.* and removed views.argument_default.node and views.argument_default.current_user.

Have I missed anything?

I added a test. Not sure if there is a better way to test, I'm open to suggestions.

Ahh that was a bad test_no_fix patch... Ignore that one for this one.

Man apologies all round, I am doing badly with my patches today :/

Voilà!

This looks quite nice ... do you think some kind of test would be though helpful here?

Did we tested other default arguments? Have no idea.

Sure, we have \Drupal\user\Tests\Views\ArgumentDefaultTest for example.

It is always good to add some form of tests :)

Added test for query parameter. The others are tested.

Thanks a lot @claudiu.cristea. I know writing tests can be painful sometimes, but its worth on the longrun.

Looks like this would need an upgrade path, no?

@xjm, we are not changing any configuration object (view) here we're only providing schema (data definition) for existing views config objects. Nothing will change in the way they are structured. New configs/views will have the same structure as the existing ones.

I'm setting this back to RTBC for the above reasons but if you feel I'm wrong, feel free to set it back to NW.

Hmm but if it doesn't need an upgrade path then how come our existing schema tests didn't catch this? Or if the schema is wrong, then how can all config saved with the wrong schema be right?

It is totally possible I am confused about something, though, so leaving at RTBC and assigning to alexpott.

Hmm but if it doesn't need an upgrade path then how come our existing schema tests didn't catch this? Or if the schema is wrong, then how can all config saved with the wrong schema be right?

We had no single instance of code using that plugin, so there was nothing to be validated. We added a test here.

Yep there's no upgrade path necessary here because the change should result in no config changes. If the new schema was enforcing a data type other than string then an upgrade path would be necessary.

+++ b/core/modules/node/config/schema/node.views.schema.yml
@@ -66,13 +66,6 @@ views.argument.node_vid:
-views.argument_default.node:
-  type: sequence
-  label: 'Content ID from URL'
-  sequence:
-    type: string
-    label: 'Nid'

+++ b/core/modules/user/config/schema/user.views.schema.yml
@@ -49,10 +49,6 @@ views.argument_default.user:
-views.argument_default.current_user:
-  type: boolean
-  label: 'User ID from logged in user'

These two cases are not tested. In the test view - which appears to be testing a node type argument.

Also please make test only patches.

Correct me if I'm wrong:

1. views.argument_default.node is tested in \Drupal\views\Tests\Plugin\ArgumentDefaultTest::testArgumentDefaultNode()
2. views.argument_default.current_user is tested in \Drupal\user\Tests\Views\ArgumentDefaultTest::test_plugin_argument_default_current_user()

Improved the test to check also the fallback. Added "test only" patch.

The test-only failed. The patch passed.

core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_default_node.yml uses

          default_argument_type: node
          default_argument_options: {  }

and core/modules/user/tests/modules/user_test_views/test_views/views.view.test_plugin_argument_default_current_user.yml uses no default_argument_options at all, which is why this schema was never triggered.

I hope #2623568-56: Config schema of argument_default plugins is incorrect explains why the test coverage is good enough. We cannot enlarge the test coverage, unless we basically want to retest schema validation as a concept.

Committed 4fc22ad and pushed to 8.3.x. Thanks!

Whilst there is no necessary upgrade path for 8.3.x because caches will be cleared we'd need a specific function for 8.2.x so I've not committed this to 8.2.x. If we want to fix this there then we need to ensure a cache flush occurs.

Here's the cache rebuild.

The patch has been once reviewed,. I don't know if needs a new review for that 8.2.x addition. I'm tentatively RTBC it.

Committed 6663917 and pushed to 8.2.x. Thanks!

I've also committed the interdiff to 8.3.x since I'm not too wild about 8.3.x missing an update that is in 8.3.x. Committed 04a771a and pushed to 8.3.x. Thanks!