Views exposed form action incorrect when view also has a page display

I can see how this might confuse the routing system. This is a form within a view within an entity, right?

The simply workaround that comes to mind would be to add an eva display option to force the view override_path to the current URL (meaning add a conditional here: https://github.com/ahebrank/eva/blob/8.x-1.x/src/Plugin/views/display/Ev...). I'm assuming this block isn't executing in your case because the page display has set override_path already?

OK, thanks for clarifying. Sounds like this is best left as an upstream issue.

This fix makes sense to me, setting to needs review to trigger the testbot, but this needs test, so needs to go to needs work.

We got a passing fix, nice!
Setting back to needs work for tests.

@jurgenhaas I agree a full integration test for the scenario you describe would be a little elaborate. So keep it simple and focus on the change being introduced in core. I would look into a kernel test that just loads the View, sets $view->override_path and then renders the View. Then check if the rendered HTML contains the correct action.

That should cover the change being introduced here. If you want to do a full integration test for the scenario you describe, that might be better to put into EVA, since there such a scenario might be much more relevant.

I did a quick look but couldn't find a test for \Drupal\views\Form\ViewsForm you can use as a starting point for this unfortunately.

Bit of a nudge, config errors can be tricky to figure out.

This test covers the change to \Drupal\views\Form\ViewsExposedForm, so we still need one for the change to \Drupal\views\Form\ViewsForm right?

Also, it's a good idea to add an interdiff when you post a patch, makes reviewing much easier. Look here for a how-to.

\Drupal\views\Form\ViewsForm is used whenever a View field contains a form element (a handler that implements viewsForm).

Maybe take a look at what \Drupal\views\Tests\ViewsFormMultipleTest is doing. And look at \Drupal\views_test_data\Plugin\views\field\FieldFormButtonTest for an example of a handler implementing this.

Shameless plug: check the PR in #2834812: Add a views handler for a per-variation add to cart button for a real life example of its use.

Patch in #25 assumes views is in modules/views rather than core/modules/views. Modifying the paths in the patch fixed the problem for me.

Another scenario that seems to exhibit this issue is with attached displays to block displays.

For example, I have a block view display that renders a list of content related to the currently displayed page. This block includes bulk operations for the related content. It also includes a display attachment via the views_data_export module for exporting a CSV of the related content.

I'm still tracing through the code to understand what is happening, but it does appear that in this case there was an intentional decision in DisplayPluginBase.php to set the form action to the URL of an attached view when the display is a block.

Related ticket in the views_data_export module issue queue: https://www.drupal.org/node/2881076

In my case, I had a view whose main display is a block (with an exposed form), then I added a second display that is a REST Export. The block doesn't have its own route, so when it was just the block in that view, it correctly used the current route for the action of the form. Once I added the REST Export display, which does have its own route, the block started using that route instead of the current route where the block was placed.

This is a result of the technique used to find a valid route: the getLinkDisplay() method is used to find a display within the view that actually does have its own route. The Block display plugin does not override the default logic of this method, which simply returns the first display that has a route. When the block was the only display in the view, this would return NULL and the form action would default to the current route. Once I added another display which had a route, that display was being returned and used for the form action.

I believe this behavior is incorrect, because the Block display plugin exposes an option to choose a linked display, and by default, the value is "None":

Therefore, defaulting to any other random display that the view contains to use as the linked display when determining the action value for the block's exposed form is a bug IMO. The attached patch makes sure the user-selected linked display is used by a block, which may be "None." The "Custom URL" option is not handled, as I don't personally have a need and it seemed more involved, but I would expect that option to be honored by the block display plugin as well.

This problem occurs for exposed forms in entity attachments too. Don't think the patch applies to dev any more, let's see...

Same patch as #33 ... go testbot ...

The embed display type has the same problem. I cant add a patch right now.

Here is a new patch for both block and embed displays.

re-roll wrong path in the last patch.

one last time

Re-roll of #42 for 9.1.x

Choose wrong core version to test with.

@carsoncho Patch #42 applied successfully on 9.1.x, So there is no re-roll required, also the re-roll you provided in #48 is incorrect according to the #42, For the reference I have added test on #42, Please have a look.

Also there is custom command failed issue because comment added after statement.

Try to fixed Cc Issue #48, #49 && Attached patch against Drupal 10.1.x

Cc Issue Fixed #54

added updated patch try to fix CCF #54

#42 Works well for D9. Thanks!

FYI patch #42 still works, tested on Drupal 10.1.6
Patches posted later doesn't work - tested with View with exposed filters using views_embed_view(), where is attached Data export display (module views_data_export, reported in issue 3166881 of this module).

With patch #56 applied to Drupal 10.1.6 I get this error in watchdog when submitting (pressing submit/apply button) with an embedded view with AJAX enabled and with exposed filters:

access denied	Warning	Path: /views/ajax?_wrapper_format=drupal_ajax&title=&field_event_date_value=2023-12-04&field_event_date_end_value=&field_event_tags_target_id=&moderation_state=All&view_name=group_dashboar

And it seems that the exposed filters have no affect on the results.

EDIT: This was not caused by the patch. It was caused by restricting access to the view by permission. An unrelated issue apparently.

Additionally when resetting the view using the reset button, despite having AJAX enabled for the view, the entire page is reloaded and I am taken to the top of the page. This is less than ideal.

EDIT: This is the behavior of the reset button regardless of whether the patch is applied or not. Still not ideal.

I also had this issue and solved it with the patch from #42. I didn't try any of the other patches based on #60
I am on views_data_export 1.2 on drupal 9.5.11.

Patch #42 is working for us on Drupal 10.1.

Still needs tests. Also issue summary should follow the standard issue template please.

Hiding problematic patches, since #42 is still what people are using.

Everything should be converted to a merge request as well.

Opened a merge request with the patch in #42.

The patch on #56( 2840283-56.patch) fails to apply on Drupal version 10.4.1

As part of fixing the test failures I realise this change is going to be a bit problematic for BC. I.e users may have blocks or embeds that they want to automatically link to the page display which would no longer be the case after this change, and at worst could throw a 500 with You cannot create a URL to a display without routes. which was one of failures in ArugmentDefaultTest

There is another scenario that needs to be considered: when setting the action path, if the view was refreshed via AJAX, then the path is set to <current>, which translates to "view/ajax...". Submitting the form then fails.
I've added a new MR (MR !13479) that extends the existing work, adding a new method to determine the action's URL taking AJAX into account.

#2844823: Views exposed form action incorrect for embedded views' displays with other displays with paths has been marked as possible duplicate and may also contain additional relevant information

I’ve hidden MR !8053 to avoid a bit of confusion and/but opened a new MR !14412, to preserve some of the work and testing I’d already done in #2844823: Views exposed form action incorrect for embedded views' displays with other displays with paths.

That testing was done manually on a real project (VBO + exposed filters on a block display with a data_export display), which helped validate a few edge cases.

I didn’t want to step on the work in MR !13479. The approach is very similar, so I’m comparing against it and folding in only what helps reduce the diff and close any remaining gaps.

@joelpittet thanks for the tidy up, does your MR address the issues raised in #75?

MR !14412 is ready for review.

This builds on a lot of hard work that’s already gone into this issue across multiple MRs. My intent here is not to step on toes, but to try to move things a bit closer to a finish line on a long-running and understandably tricky issue. Aside from a small class property name change, I did not merge the other MRs directly, though they very much informed this approach.

I added a post_update hook to preserve BC while removing the guessing behaviour, to avoid unexpected side effects. This will affect my own project(s), but I believe the end result is more predictable behaviour overall.

If this isn’t the right direction, or if other MRs are closer to the mark, I’m completely fine with this one being closed. I’m not attached to it (EDIT: well a little bit attached 😉 but I can deal!). My hope is that, even if this MR isn’t taken, some of the ideas here are useful and help move the issue forward! ➡️

This likely needs a CR. I’m happy to draft one if this approach looks acceptable (though I don't want to create one if I am off base).

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

The review bot went after the other MR. I just fixed the PHPCS issue it reported to keep it from moving back to NW. Please review #86

I think we just close the old MR at this point?

Left a question on the MR but it also needs a rebase.

Also can probably start the CR.

Thanks!

CR added https://www.drupal.org/node/3579301
The reroll was a bit tricky but I think I got it. Responded to MR comments.

MR!14412 is failing to apply for me now on Drupal core 11.3.5. Not sure if this is as a result of the rebase?

Thanks @joelpittet responded to the comment with a small change request to use constructor promotion. Less lines.

I have added constructor property promotion for the new property.

@d.fisher re #95 that is very unlucky but also very possible – we are applying it against the main branch which might have changes that don’t gel well with Drupal 11.3.5.

It seems like we are on the homestretch for this issue (knock on wood) but you can create a patch out of the older commits with git diff.​​​​​​​​​​​​​​​​
(Hot tip, exclude tests from the diff/patch will yield less conflicts)

Yes I suspect it will be some difference that has crept in between the tagged release and the main branch which is unfortunate but always something that can happen. I was just testing using the MR and adding .patch to the end. As it happens we've just switched to AJAX so this is no longer an immediate issue on the site I was testing it on but good to know this one is moving along! Thank you for all the hard work going on in here!

Bump for attention 🕺

Post removed; wrong issue.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

The post update hook removals caused a reroll.

The Needs Review Queue Bot tested this issue. The merge request has merge conflicts and cannot be merged. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Back to Needs Review, the views.post_update.php keeps getting refactored for a clean-up of sorts.

Re-roll the patch to be working with drupal 10.3.x

Left comment on the test.

Addressed feedback, thanks for keeping tabs 📑 @smustgrave

Believe this one is ready! Thanks for the quick turnaround

I don't see any review of the change record here. I am setting this to Needs review for that. I have read it and want to point out that the change is to be first.

The remaining tasks stated that the the BC concerns in #75 are still be done. Is that true?

I have updated credit.

I was going to review the CR but realized I wrote it... so need another taker?

#75 BC concerns shouldn't be of concern with the latest version. See commits addressing backfill in https://www.drupal.org/project/drupal/issues/2840283#mr14412-note659507

Reviewed the CR and it looks good to me.

I wasn't able to finish reviewing the whole MR, but I thought I'd note a couple things I saw so far.

There is still an issue when Better Exposed Filters is used (or any other custom code trying to figure out if the display has an URL. One can argue that the Better Exposed Filters should address it instead, but it looks like it is really Views issue - sort of broken contract or promise.

In BEF, the Reset button callback calls ViewsExecutable::hasUrl(), line 1066:

    if ($this->view->hasUrl()) {
      $form_state->setRedirectUrl($this->view->getUrl());
    }
    else {
      // Fallback if the view does not have a URL (embedded or block views?)
      $form_state->setRedirect('<current>');
    }

Essentially, looks like it tries to decide whether to display the current page or use a link provided by the view display. I see it as a way to accommodate embeds where view display has not URL and is rather displayed as part or whatever page it is embed on. By the code here, it likely assumes that if the current display has no URL, then false will be returned.

The ViewsExecutable::hasUrl() however does something different - it finds a URL if it is set on one at least one of the displays, even if that is not the default display. The doc comment says:

Determines whether you can link to the view or a particular display.

Some displays (e.g. block displays) do not have their own route, but may
optionally provide a link to another display that does have a route.

"...but may optionally provide a link" suggests it should rather return FALSE if the current display has no URL assigned to it. But in reality the ViewsExecutable::hasUrl() calls DisplayPluginBase::getRoutedDisplay(), which then purposefully finds a routed display and returns that display back to caller.

This needs to be addressed as well.

Sounds like NW may be the spot?

Actually got bit by this one again this week.

I’m one of the maintainers of bef and wonder if we need to add the check for DisplayRouterInterface too.

On BEF side - I added a ticket and posted a patch that worked for me. I am not deeply familiar with the affected Views and BEF functionalities, but definitely looks like hasUrl() / getUrl() return misleading results.

The CR says:

> If you have a block or embed display whose exposed filter or bulk form worked correctly only because it happened to pick up a routed display's URL automatically, you should configure the Link display option on that display explicitly.

but AFAICT the MR has an update that changes views so the previously automatic choice is now set explicity.

We should still tell people to check the views that get updated to make sure they're ok though, but the wording needs to be changed.

@joachim - This confuses me... Where on the Embed display there is a link option? I am just curious...

@euk RE #123 sorry if I’m misunderstanding the confusion. The Embed display has a Link display option under Pager, as do most unrouted display types (to be fair, it feels hidden in the UI):

Are you asking where this is configured when the view is embedded programmatically, or is the option not visible in your Views UI?

@joachim RE #122 yes, I can update the CR, the current solution is trying to not break people's built up reliance on this "guessing".

This gist (and intent) of the current approach is:

• During the update, resolve the previously guessed display and preserve it as explicit link_display configuration.
• Going forward, stop guessing for new configuration and require the intended target to be explicit.

I need to re-read the MR to make sure it is doing that (quite tricky to not break other parts of Views with this kind of change) more 👀 is always appreciated!

@euk RE #115, thanks for the BEF code snippet. I don't want to break things for BEF! Could you link to the relevant BEF source code or issue(if you created one)? I’d like to review see the behaviour it's trying to do.

@smustgrave I see why this looks useful:
if ($view->display_handler instanceof DisplayRouterInterface && $view->hasUrl()) {
Just ensures the current display handler is routed before giving you a URL, the else is where the gremlins are buried but at least that part is super clear what you will get.

I should have not tried to target the views exposed forms originally and went straight to the heart of the issue.

From DisplayPluginBase::getLinkDisplay()

// If unknown, pick the first one.

It's been a while, so I might find out why I didn't... but that comment is truly the itch I am trying to scratch here, but still specifically for forms where I saw the problem.

I think I got all the failed tests, testLayoutBuilderUi and testStylesAndScripts don't fail locally. So I will just re-run the tests, don't see them on #2829040: [meta] Known intermittent, random, and environment-specific test failures... yet.

I think this more broad change will be better for this problem... famous last words.

Added the testStylesAndScripts random failure here because I had the job still up #3614445: [random test failure] CKEditor5MarkupTest::testStylesAndScripts

Ok, the tests are all passing now and I have updated the CR. Thanks everyone for the feedback and patience while I worked through this.

AI disclosure: I used AI to help write the first set of tests, including the new AJAX one, and to help draft the CR. I have read through the resulting CR and, although it may be a bit verbose, I think it gets the point across. I also worked through the remaining test failures to get a better feel for the changes myself.

My (over-)explaining in #124 helped me better articulate the goal and led to a rethink of the solution. I directed the work further down into DisplayPluginBase::getLinkDisplay(), so that we remove the guessing at its source rather than only fixing exposed forms. The post-update performs the old guessing once and saves the result as explicit configuration, which should preserve existing behaviour.

Otherwise, I am setting this back to Needs Review. Thanks again for joining me down the complicated 🐰🕳️

The one thing I would like from future reviewers is to LMK if they foresee side-effects on the BC approach that I currently can't see. Currently happy with this.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Bad review queue bot, I clicked update fork and it merged easy enough...

@joelpittet

Sorry for the late response. As far as the link display option - that is no less misleading/confusing =) The pager works for me, or at least I was not observing any issues with it. I didn't even think about pager to be the way of fixing the Reset button behavior. Thanks for the hint!

I believe you already found the BEF issue I posted =) If not - here it is: https://git.drupalcode.org/project/better_exposed_filters/-/work_items/3...

Will say applying the fix did fix the test I had setup for bef so +1 for me.

@euk, RE #131 not late at all, everything is async 😃 I didn't find that issue yet, thanks for posting it!

Yes, that "Link display" option being in the Pager section is confusing, (don't want to add that placement to the scope of this though! too bike-sheddy), but unusual place for it indeed... My guess is that pagers (if used) always have links so they need a base URL... so most common link related thing... who knows, it came from past iterations of Views.

I tested and commented on https://git.drupalcode.org/project/better_exposed_filters/-/work_items/3... and I believe (even more) that this is the right fix and tagging this as a 'Contributed project blocker'

> Yes, that "Link display" option being in the Pager section is confusing, (don't want to add that placement to the scope of this though! too bike-sheddy),

Agreed that it's not in scope here. Could someone file an issue for it though, as it's definitely a confusing place for it.

The current MR doesn't apply to latest core 11.4.5

@joachim RE #135 filed #3617980: Move 'Link display' setting out of the Pager section

@jrochate
This is what I did to get a working 11.4.x patch:

~/Contrib/drupal/drupal 11.4.x 
❯ curl https://git.drupalcode.org/project/drupal/-/merge_requests/14412.diff | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 32560    0 32560    0     0  23504      0 --:--:--  0:00:01 --:--:-- 23509
error: patch failed: core/modules/views/views.post_update.php:5
error: core/modules/views/views.post_update.php: patch does not apply
error: core/modules/views_ui/tests/modules/views_ui_test/config/install/views.view.test_glossary.yml: No such file or directory

~/Contrib/drupal/drupal 11.4.x 
❯ git clean -fd

❯ git checkout main -- core/modules/views/views.post_update.php core/modules/views_ui/tests/modules/views_ui_test/config/install/views.view.test_glossary.yml

~/Contrib/drupal/drupal 11.4.x* 
❯ curl https://git.drupalcode.org/project/drupal/-/merge_requests/14412.diff | patch -p1

... finagle the core/modules/views/views.post_update.php manually ...

~/Contrib/drupal/drupal 11.4.x* 5s 
❯ git diff 11.4.x > 2840283-views-routable-display-guessing-11.4.x.patch

Please give it a try!

@joelpittet I confirm the the last patch works in the following scenario.

Environment:

Drupal core 11.4.6
Better Exposed Filters 7.1.3
Acquia Site Studio 8.2.7

Scenario: A view with two displays: a page display (routed) and a block display (unrouted). The block display is embedded within a Site Studio component on a page different from the page display's path. BEF is configured with a Reset button on the exposed filters.

Behavior without the patch: Clicking Reset redirects the form to the page display's URL, leaving the current page where the block is embedded.

Behavior with the patch: Clicking Reset stays on the current page and resets the filters correctly, without redirecting to the page display.

I did not test other scenarios (AJAX, custom URLs, multiple displays), but the block-embedded-in-component case works as expected.

@andreynacarrasco thanks for testing, do you feel comfortable setting to RTBC based on #138? We had one back in #113

Latest commits were a re-roll was just conflicts in views.post_update.php