Double exposed filter field when trying to use "q" as identifier

The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Attached patch against Drupal 10.1.x

Patch #19 is not applied for Drupal 10 so Interdiff file is not added.

Checking patch core/modules/views/src/Plugin/views/filter/FilterPluginBase.php...
error: while searching for:
*/
abstract class FilterPluginBase extends HandlerBase implements CacheableDependencyInterface {

/**
* Contains the actual value of the field,either configured in the views ui
* or entered in the exposed filters.

error: patch failed: core/modules/views/src/Plugin/views/filter/FilterPluginBase.php:45
error: core/modules/views/src/Plugin/views/filter/FilterPluginBase.php: patch does not apply
Checking patch core/modules/views/tests/src/Functional/Plugin/ExposedFormTest.php...
Hunk #2 succeeded at 162 (offset 3 lines).

Fix cc Issue #23

#23 had no interdiff provided but can see the size of the patch almost doubled so going to say there are out of scope changes unless they can be explained.

Also it doesn't seem to fix the views that currently could experience this issue. Should we had an update path?

Thanks to everyone here for working on fixing this. I just got hit by this bug, and spent much time finding the reason why ( 🐛 Setting Filter Criteria >> Filter Identifier to q in a View adds extra input field Closed: duplicate ).

It would be an awesome improvement, and save lots of time for those poor souls who follow the norm on the internet, and update the "Filter identifier" to "q" ... something so simple, yet with big consequences.

It would be awesome if we could get this moving again, committed and released--perhaps in Drupal 10.2, with luck?

From my perspective we should stop the accident as soon as possible, so perhaps adding an update path could be a follow up issue?

Here is a re-roll for Drupal 10.1, and a re-roll interdiff → .

So double checked with @catch and we typically don't move those upgrade paths to follow ups unless the issue is critical which then one is not. just fyi.

But I'm back tracking my previous statement and since this just never worked I doubt any view was using this. If anyone disagrees then yes an upgrade path would be needed but can't think of why someone would use this if it didn't work.

So going to mark it.

The last submitted patch, 28: 2944421-28.patch, failed testing. View results →

Seems unrelated

The last submitted patch, 28: 2944421-28.patch, failed testing. View results →

Unrelated

Thanks @smustgrave and @Lendude, you're right. I ran the test again, and it passed. I added it to 🌱 [meta] Known intermittent, random, and environment-specific test failures Active .

I guess Version should be still 10.1, right?

Doing triage on the RTBC queue.

I started by following the steps to reproduce and was able to reproduce the problem. I then applied the patch and verified it works.

This issue is changing the User Interface and there should have been a usability review. And for that screenshots of the changes should be in the Issue Summary. I have read through the comments and the last code review was 5 years ago. And I don't see any comments that the test has been reviewed at all. Can anyone confirm that they checked to code?

I then looked closer at the new description, which I found very hard to read. The last sentence starts with a quoted word which was jarring, given that the previous sentence had the word list at the end.

This will appear in the URL after the ? to identify this filter. Cannot be blank. Only letters, digits and the dot ("."), hyphen ("-"), underscore ("_"), and tilde ("~") characters are allowed. "value", "q", "destination", "_format", "_wrapper_format", "token" are reserved words and cannot be used.

I then looked in core for other similar strings. This is the on used for file upload.
Separate extensions with a comma or space. Each extension can contain alphanumeric characters, '.', and '_', and should start and end with an alphanumeric character.

As you can see that latter one uses single quotes to identify characters instead of parenthesis and double quotes as in the first one. And the error message in this patch is very different that the one you get when entering an invalid file extension. The new text definitely needs a Usability review so that we have consistency in the UI.

I then looked at the patch. I now see that the error message is in the same style of others in Views. Not that I agree, but I understand.

+++ b/core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
@@ -45,6 +45,23 @@
+   * This list contains strings that could cause clashes with other site
+   * operations when used as a filter identifier.

This doesn't seem to add any useful information. I can see it is a list of strings. And the 'other clashes' aren't defined so it leaves me wondering. I suggest that this is not needed.

PHPStorm is also showing that there are duplicate blocks of code in FilterPluginBase. Unless there is an issue for that, this needs a follow up.

I am setting this to Needs Work but I intend to consult with the other committers.

I apologize for taking so long to get back to this issue. I checked with the other committers and they agreed it is OK to commit this.

I've updated credit and I made some screenshots.

Thanks everyone and again, sorry for the delay.

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