Cannot save a view where a filter value option contains a period

Great bug report!.

That is certainly a bug in views ... the problem is that we store things with key: value in YAML, but we use the "." to specify the hierarchy of elements. One possible solution could be to store the entries using a list rather than an a key value thing (called sequence). Another possibility would be to rewrite those values, like replacing "." with "__" or so.

Can the report be simplified? It sounds like this could be reproduced with just an options field on nodes, where one option is '.' -- and dispense with the custom entity part.

Confirming that this is reproducible with just nodes:

- create a text options field on a node type
- add an option consisting of just a period, '.'
- create a view of nodes
- add a filter on the field, and select the period as the filter value
- save the view

BOOM:

> Drupal\Core\Config\ConfigValueException: . key contains a dot which is not supported. in Drupal\Core\Config\ConfigBase->validateKeys() (line 211 of core/lib/Drupal/Core/Config/ConfigBase.php).

The Config system planned throws exceptions each times it finds a dot in a key - Normal

if (strpos($key, '.') !== FALSE) {
  throw new ConfigValueException("$key key contains a dot which is not supported.");
}

I guess we sloud escape the dot at the time the filter is saved.

Further clarification: a filter value for a text filter, such as node title, is fine. That's because the period is stored as a value, not an array key.

The problem is specifically when the filter has you choose from options, and the problem is caused by the way that the checkboxes form element returns its value: an array where the keys are the option storage values, and the values are either 0 for unchecked, or the key for checked.

> I guess we sloud escape the dot at the time the filter is saved.

Another option would be to process the checkboxes element value into a numeric array of the checked values, in other words:

array_values(array_filter(form values))

Here are some failing tests for the handler that is used in the steps described by @joachim in #5 (list_field) and the base handler (in_operator).

I think to replace "." with anyother character will require lot more changes into core functions like set, validate,setData etc...
While other options suggested by @joachim would be better option and we should give a try.

@Lendude / @dawehner what you suggest on this?

I occurs to me that we should compare how other module store a value that's from a form checkboxes element.

For instance, Menu UI module has a checkboxes form element in the node type settings. Those get saved like this:

third_party_settings:
  menu_ui:
    available_menus:
      - footer
      - main
    parent: 'main:'

So filtering as I suggested in #8 does look like the right thing to do.

Subscribe++

Subscribe
Need a fix

Just updating the target version.

Updating target

This is a bug. It's 8.9.x

Rerolled the TEST ONLY patch from #9.

Here's a fix. I wonder if we need to provide an update path. Any thoughts?

Also, this is Major as it breaks the ability so save a View in such circumstances.

Added an update path. Fixing default configs.

Made the check faster in ViewsConfigUpdater (this also fixes a lot of tests).

Forgot a copy/paste leftover.

Fixing all failed tests.

Optimize the ViewsConfigUpdater to avoid creating a plugin instance.

Adding some return types. Reorder the methods inside ViewsConfigUpdater

Updated the IS a bit to make the problem and proposed solution a bit clearer

The patch looks really good, nothing to nitpick. We have a fix, tests for the fix, update path and test for the update path.

This patch needs a reroll. The solution does indeed look really nice.

+++ b/core/modules/views/tests/src/Functional/Plugin/FilterTest.php
@@ -155,7 +155,6 @@ public function testFilterQuery() {
-    $view = Views::getView('test_filter_in_operator_ui');

This seems out-of-scope. We need to work why this variable is unused.

+++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
@@ -309,15 +309,13 @@ public function acceptExposedInput($input) {
     // Drupal's FAPI system automatically puts '0' in for any checkbox that
-    // was not set, and the key to the checkbox if it is set.
-    // Unfortunately, this means that if the key to that checkbox is 0,
-    // we are unable to tell if that checkbox was set or not.
+    // was not set, and the key to the checkbox if it is set. Unfortunately,
+    // this means that if the key to that checkbox is 0, we are unable to tell
+    // if that checkbox was set or not.
 
-    // Luckily, the '#value' on the checkboxes form actually contains
-    // *only* a list of checkboxes that were set, and we can use that
-    // instead.
-
...
+    // Luckily, the '#value' on the checkboxes form actually contains *only* a
+    // list of checkboxes that were set, and we can use that instead.

The comment changes here seem out-of-scope. It looks like the comments have not changed at all. Only the code has. Making changes like this increases reviewer load.

Weird, the patch applied cleanly against 8.9.x. Maybe you tried against 9.1.x? As this is a bug I think it should be 8.9.x.

This seems out-of-scope. We need to work why this variable is unused.

During development, I had that test failed. I've noticed the unused variable. Added back.

The comment changes here seem out-of-scope. It looks like the comments have not changed at all. Only the code has. Making changes like this increases reviewer load.

True, the code line is the only one in the method. The comment looks a little untidy as it comes to our coding standards. Reverted.

Removing the tag

@claudiu.cristea we have to commit the fix to 9.1.x first. So we need a 9.1.x patch before we can fix this on any other branch. Also as this contains an update function it'll need special consideration for backport to 8.9.x and 9.0.x. i.e release manager approval.

OK. I'm not up-to-date with the rules on D8>D9 transition. Here's the 9.1.x patch.

Nice, feedback addressed, back to RTBC assuming this comes back green

Tagging for 8.9.x backport.

Committed 104bca1 and pushed to 9.1.x. Thanks!

Going to dicuss with other committers about the backport situation.

So the config update function looks fine, but there are quite a lot of changes here overall including a new public method on a class.

@catch yes but it is a new method on a plugin - something that is declared as @internal and not covered by our BC promise.Yes we go out of way to not break stuff but at the same time this bug has no work around.

1. +++ b/core/modules/views/src/Plugin/views/filter/Bundle.php
   @@ -128,7 +128,7 @@ public function calculateDependencies() {
   -    foreach (array_keys($this->value) as $bundle) {
   +    foreach ($this->value as $bundle) {
   

   While reviewing this for backport I've got more and more concerned about potential BC impacts of this change. If we need to make this fix here - then what about contrib. InOperator is extended from a lot - see http://grep.xnddx.ru/search?text=InOperator&filename= - so there's a very good chance of this causing issues.

   I think we need to consider only fixing the configuration but maintaining the same structure once the config is loaded. We did something similar for fields and list values.

2. +++ b/core/modules/views/src/ViewsConfigUpdater.php
   @@ -477,4 +494,49 @@ protected function mapOperatorFromSingleToMultiple($single_operator) {
   +      // Only process if the keys are the same as the values.
   +      if ($values === array_keys($handler['value'])) {
   

   This conditional part gives me concern too. Why is this necessary? If someone resaved a view via the UI isn't the array_values() going to kick in the keys be ignored. Or does this exist to make this reentrant? So we can tell a fixed array from a not fixed one? If so I think we should consider documenting this different.

   Also I think we should check the class before checking the values as that is more logical and less surprising. It might not be as performant but I'm not sure that it matters too much - or if it really is more performant this way around.

Given the BC concerns in point 1 and after discussions with @catch I've decided to revert this for now. Better than regretting it later. It'd be great if people who've worked on the issue can have a think about contrib and possible BC impacts and ways we can mitigate them. Sorry doing the revert but I think it's the best course of action given the concerns.

I think we need to consider only fixing the configuration but maintaining the same structure once the config is loaded. We did something similar for fields and list values.

@alexpott, could you, please, point me to code for fields and list values?

Based on comment #45, I patched up all the 37 files and I still get the same error
Drupal\Core\Config\ConfigValueException: es_attachment.data key contains a dot which is not supported. in Drupal\Core\Config\ConfigBase->validateKeys() (line 211 of /opt/rh/httpd24/root/var/www/html/dcu/web/core/lib/Drupal/Core/Config/ConfigBase.php).
I just updated my core to 9.2-dev