Spaces in exposed filter ID cause filter not to work.

Followed the steps to reproduce and could reproduce the issue.

Also, this could be an upgrade issue as I don't believe there was any restriction on filter IDs in D7 (unverified).

Checked and this is just as broken in D7, so don't know if that would be an issue.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -614,7 +614,7 @@ public function buildExposeForm(&$form, FormStateInterface $form_state) {
+      '#description' => $this->t('This will appear in the URL after the ? to identify this filter. Cannot be blank. Only letters, digits and the dot ("."), hyphen ("-"), underscore ("_"), and tilda ("~") characters are allowed.'),

Isn't ~ called a 'tilde'?

Adding a patch with just a test if the override for the identifier works. Will see if I can update that to fail when supplied with an identifier with spaces or other restricted characters.

Here a version of the test only patch to illustrate the problem. The current version of the patch doesn't fix this, so that's something to look at.

#7 good point. And instead of the manual str_replace/strtolower stuff I did, we should probably use Html::cleanCssIdentifier

Working with @mikeker at the PNW Drupal Summit - assigning this issue over to me.

This patch combines #2 and #4. I'm going to apply suggestions from comments #3 and #7 now.

Fixes issues raised in comment #3.

Forgot to set it to "Needs review" - Doh!

I'm not sure what's wrong with the test, but I've got to leave this issue for now.

Thanks for taking a look at this @amyvs!

Did a rewrite of this. Got rid of the strtolower stuff, got rid of some code duplication, added some tests for the validation rules (old and new).

The original test with the space inserted directly into the settings still fails though. Would have figuered that adding a validate() function to the handler would stop it, but that doesn't seem to to be the case.

Still needs work, but want to see what the testbot has to say so far.

+++ b/core/modules/views/src/Tests/Plugin/ExposedFormTest.php
@@ -85,9 +85,68 @@ public function testSubmitButton() {
-  public function testResetButton() {
+  public function ptestResetButton() {

bleh, added this by mistake

+++ b/core/modules/views/src/Plugin/views/filter/InOperator.php
@@ -415,7 +415,7 @@ protected function opEmpty() {
-    $errors = array();
+    $errors = parent::validate();

@mikeker Ah yeah! Good catch. And yes, the expected value in the test was just left over copy/paste that I hadn't bothered to clean up since the test wasn't doing what I wanted anyway, sloppy me.

Rechecked that Drupal\views\Plugin\views\filter\InOperator is indeed the only override of FilterPluginBase::validate() so no other methods need to be updated.

Looks good now. Would love to RTBC but wrote a bit to much of the patch to justify that. Adding VDC tag to maybe get some extra eyes on this.

+++ b/core/modules/views/src/Tests/Plugin/ExposedFormTest.php
@@ -85,6 +85,69 @@ public function testSubmitButton() {
+    $random = $this->randomMachineName();
...
+    $random = $this->randomMachineName() . ' ' . $this->randomMachineName();

Let's not use a random string, but rather use a dedicated known bad value

@dawehner something like this?

Yeah exactly, thank you!

This issue doesn't really sounds problematic for a release. Feel free to add the 'rc target triage' if you want to.

per @dawehner in #26, +rc target triage

Discussed with @effulgentsia and we don't think this change needs to be made during RC since it is fairly low impact. It looks like this would be okay during a patch release (it just adds a protected method and fixes internal behavior), so untagging and leaving RTBC for more thorough review after 8.0.0.

What about existing views? Do we need to think about update paths here?

Well sure, there is the chance that there are existing broken views out there, but in that case someone really entered something and then never actually tested the result on a real website.

So for existing views if you try to save via the UI, you'd get a validation error then have to fix it manually.

Since this isn't validated on the configuration entity itself, then it's not going to affect config sync or similar either way.

I think that makes sense in terms of upgrade path being a 'nice to have'. Longer term I'd expect us to be moving the validation out of the form handling though.

Committed/pushed to 8.1.x, thanks!

This adds a protected method to a base class, which theoretically could conflict with an extending class in core or contrib.

Due to this, moving to 8.0.x for backport - we could underscore prefix the protected method and mark it @internal to get around the potential breakage. Or we can not backport and just let this be fixed in 8.1.x.

Actually I'm going to mark this fixed - if someone wants to backport, then please change status.