Problem/Motivation

views.argument_validator.numeric
Should be an array and is defined as a boolean.
We will also add here #2654300: User and node argument validators have broken schema and #2531850: Schema for the taxonomy term name argument validator "transform" option is missing, and then add a test for validating it.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 interdiff-argument_validator-2663290-11-14.txt695 bytesedurenye
#14 argument_validator-2663290-14.patch9.14 KBedurenye
#11 interdiff-argument_validator-2663290-6-11.txt630 bytesedurenye
#11 argument_validator-2663290-11.patch9.08 KBedurenye
#6 interdiff-argument_validator-2663290-3-6.txt592 bytesedurenye
#6 argument_validator-2663290-6.patch8.98 KBedurenye
#3 argument_validator-2663290-3.patch9.03 KBedurenye
#3 argument_validator-2663290-3-test_only.patch6.32 KBedurenye

Comments

edurenye created an issue. See original summary.

edurenye’s picture

edurenye commented
Title: Numeric argument validator schema is broken » Argument validator schemas are broken
Assigned: Unassigned » edurenye
Issue summary: View changes

I'm merging this issues in one as will be easier to fix, review and add tests.

edurenye’s picture

edurenye commented
Status: Active » Needs review
StatusFileSize
new argument_validator-2663290-3-test_only.patch6.32 KB
new argument_validator-2663290-3.patch9.03 KB

Merged the fixes and added a tests.
I'll add the test of taxonomy term over the test that will be added here #2640994: Fix label token replacement for views entity reference arguments

The last submitted patch, 3: argument_validator-2663290-3-test_only.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/modules/user/config/schema/user.views.schema.yml
@@ -27,13 +27,10 @@ views.argument.user__roles_rid:
+  type: views.argument_validator_entity

+++ b/core/modules/views/config/schema/views.argument_validator.schema.yml
@@ -15,8 +15,10 @@ views.argument_validator.php:
 views.argument_validator.numeric:
-  type: boolean
+  type: sequence
   label: 'Numeric'
+  sequence:
+    type: string

It's not a sequence. A sequence is a list of identical elements. Options are always a mapping.

The test coverage is quite implicit, simply by having an otherwise unused test view by having those options configured. We should at least add a comment to the view why it exists. Would also like to know from @dawehner if that's enough for him. IMHO, it's better to have these issues fixed with minimal test coverage instead of not fixed :)

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new argument_validator-2663290-6.patch8.98 KB
new interdiff-argument_validator-2663290-3-6.txt592 bytes

Ok, changed to mapping.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Assigned: edurenye » dawehner

Ok, lets get some feedback from @dawehner :)

dawehner’s picture

dawehner
Primary language German
commented

This is looking great in general!

  1. +++ b/core/modules/node/config/schema/node.views.schema.yml
@@ -73,26 +73,6 @@ views.argument_default.node:
-views.argument_validator.node:

    This one indeed no longer exists. 

  2. +++ b/core/modules/views/config/schema/views.argument_validator.schema.yml
@@ -15,7 +15,7 @@ views.argument_validator.php:
 views.argument_validator.numeric:
-  type: boolean
+  type: mapping
   label: 'Numeric'

    This is a sign that we should have a default config schema using views.argument_validator.*: instead

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

We're not very consistent on in regards to default wildcard schema for plugins like that. I know we do it for blocks but not in many other places.

I kind of like forcing people to explicitly define the schema, bugs like #2663376: The core block_settings schema defines schema for block_content blocks don't happen then :)

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Reviewed & tested by the community

We're not very consistent on in regards to default wildcard schema for plugins like that. I know we do it for blocks but not in many other places.

Well, my point is, if we touch that example anyway, why not provide a generic one. I think in views we added more and more over time, but sure, let's keep it.

edurenye’s picture

edurenye commented
StatusFileSize
new argument_validator-2663290-11.patch9.08 KB
new interdiff-argument_validator-2663290-6-11.txt630 bytes

If you want the default, here is, I think is good to have the default one.

dawehner’s picture

dawehner
Primary language German
commented

Thank you @edurenye!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

This is still missing a reply to #5 on the test view. Agreed it at least needs a comment.

edurenye’s picture

edurenye commented
Status: Needs work » Needs review
StatusFileSize
new argument_validator-2663290-14.patch9.14 KB
new interdiff-argument_validator-2663290-11-14.txt695 bytes

Added a comment in the description.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Adding that as the description is an interesting idea (I would have used a yaml comment at the top of the file). Works for me, lets see if @catch agrees.

dawehner’s picture

dawehner
Primary language German
commented

Added a comment in the description.

This isn't a bad idea!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Description works for me. Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 1e8c0d3 on 8.1.x 
    Issue #2663290 by edurenye, Berdir, dawehner: Argument validator schemas...

  • catch committed 7def671 on 8.0.x 
    Issue #2663290 by edurenye, Berdir, dawehner: Argument validator schemas...

Status: Fixed » Closed (fixed)

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