View's build fails when an unrelated form on the same page has validation errors

Thank you for providing such helpful information and a patch, so set it to needs review.

Do you currently understand why this behavior is happening, because just by thinking about FAPI etc.
I don't really see what could cause this behavior. Your patch seems to fix the symptoms not the real reason, but yeah sometimes you can't choose.

This patch makes sense to me.

Yeah this makes sense now! It's interesting that you are the first one who see that problem.
Thanks for the code, committed and pushed to 7.x-3.x

Yet another reason why global state can be problematic.

Here are patches for d8 which also includes tests.

+++ b/core/modules/views/lib/Drupal/views/Tests/Plugin/ExposedFormTest.phpundefined
@@ -51,7 +51,7 @@ protected function setUp() {
+  public function _testResetButton() {

Some tests are disabled ;)

Ups right.

Also rerolled against recent changes.

This time without fails.

#9: drupal-1863020-9.patch queued for re-testing.

Just a rerole.

#12: drupal-1863020-12.patch queued for re-testing.

.

I will try to reroll.

Ok i am "unassigning" from that issues. The reroll is to heavy for me cause of the long time.

Rerolled #12 and run test.

fixing tag. sorry for noise.

18: drupal-1863020-18.patch queued for re-testing.

Rerolling patch from #18

working on a reroll

Reroll from #24 (my bad on the patch name)

Renamed reroll from patch #24

.

28: drupal-1863020-28.patch queued for re-testing.

re-rolling the patch

Thanks for the re-roll, and the reminder this issue existed :)

It needed a bit of fixing to bring it up to date, mainly:

- Added a little more to the test assertions
- Created a FormInterface form that has the form error
- Use the #view element to render it page
- Converted the page to a route and controller class

I also rolled the callbacks etc.. into the views_test_data module instead of a new views_test_form module.

Sorry - with the correct interdiff.

I also think this is major. This will cause some pretty unexpected behaviour for people.

Needs a re-roll. Before that though, it might not hurt to validate this is still an issue since it has been a little while since things were worked on here. I'm pretty sure this also needs a backport to D7.

I also faced this issue, and came up with the following patch.

First of all re-rolling with just the tests from #48.

Patch was very out of date, slightly better re-roll.

Test actually runs properly now, but doesn't fail for me either via simpletest or manually testing with the test module.

OK test should now fail and updated version of that patch that makes that test pass.

Didn't check other views tests though so it could break something else.

The FormState:hasAnyErrors() check in HEAD is completely wrong, and there's no way to get the errors without a proper FormState object, so had to add an optional $form_state parameter to ExposedFormPluginBase::renderForm().

My previous comment was about another issue. Sorry for the noise.

Discussed with @xjm, @tim.plunkett and @dawehner - we decided that this issue is a major issue which you are able to configure a view through the UI that renders the site unusable.

+++ b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php
@@ -126,10 +126,12 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    */
-  public function renderExposedForm($block = FALSE) {
+  public function renderExposedForm($block = FALSE, FormStateInterface $form_state = NULL) {
+    if (!isset($form_state)) {
+      $form_state = new FormState();
+    }

It would be nice to document why we need this second form_state and how this fixes problems.

I filed a duplicate issue, #2841413, and submitted a patch there. Comparing my patch to #52, my patch may be a little more straightforward, as it does not require a second form_state. Instead, it checks for form_state errors in the renderExposedForm() method and sets the "abort" flag there if it encounters an error. In the ViewExecutable build() method, this allows the removal of the global check, FormState::hasAnyErrors.

My conjecture is that FormState::hasAnyErrors is the wrong method to call, because it is a global check for errors on any form on the page. My patch eliminates the use of that method.

I've created a new patch combining the tests from #52 with my patch. I attempted an interdiff, but interdiff wouldn't work, I guess because the differences are too great. Instead, I have also uploaded a version of my patch without the tests, which is pretty simple.

I look forward to everyone's feedback. Thanks!

Oops, I didn't roll the patch in #60 correctly: I forgot to add the new files from the tests in #52, which is why the patch failed testing.

Here's a rerolled patch with all the tests.

@paulmckibben fix looks good to me, makes sense and simplifies things and still passes the tests.

Bit of nitpicking:

1. +++ b/core/modules/views/src/Plugin/views/exposed_form/ExposedFormPluginBase.php
   @@ -133,6 +133,10 @@ public function renderExposedForm($block = FALSE) {
   +    $errors = $form_state->getErrors();
   +    if (!empty($errors)) {
   

   This could use a comment as to why we are doing it at that point during the execution.

2. +++ b/core/modules/views/src/Tests/Plugin/ExposedFormTest.php
   @@ -398,4 +400,24 @@ protected function getExpectedExposedFormId(ViewExecutable $view) {
   +
   

   extra white space line can be taken out

3. +++ b/core/modules/views/tests/modules/views_test_data/src/Controller/ViewsTestDataController.php
   @@ -0,0 +1,32 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\views_test_data\Controller\ViewsTestDataController.
   + */
   

   this can be taken out, no longer needed per coding standards.

4. +++ b/core/modules/views/tests/modules/views_test_data/src/Form/ViewsTestDataErrorForm.php
   @@ -0,0 +1,52 @@
   +/**
   + * @file
   + * Contains \Drupal\views_test_data\Form\ViewsTestDataErrorForm.
   + */
   

   this can be taken out too.

5. +++ b/core/modules/views/tests/modules/views_test_data/src/Form/ViewsTestDataErrorForm.php
   @@ -0,0 +1,52 @@
   + * Simple form page callback to test the view element.
   

   Seems like a copy/paste left over, needs to be updated to something relevant to the current use case.

@Lendude, thanks for the review.

I've implemented your feedback and have uploaded a new patch and interdiff.

We have tests, we have a fix, all is green.

Putting this back in RTBC. I reran tests and they passed. Not sure what happened in #67.

Committed and pushed 4fc3756 to 8.4.x and 25d7f11 to 8.3.x. Thanks!

Leaving at patch to be ported so that the D7 views issue can be opened - once it is this issue should be closed.

diff --git a/core/modules/views/src/ViewExecutable.php b/core/modules/views/src/ViewExecutable.php
index 3b614a0..991f6c3 100644
--- a/core/modules/views/src/ViewExecutable.php
+++ b/core/modules/views/src/ViewExecutable.php
@@ -5,7 +5,6 @@
 use Drupal\Component\Utility\Html;
 use Drupal\Component\Utility\Tags;
 use Drupal\Core\DependencyInjection\DependencySerializationTrait;
-use Drupal\Core\Form\FormState;
 use Drupal\Core\Routing\RouteProviderInterface;
 use Drupal\Core\Session\AccountInterface;
 use Drupal\views\Plugin\views\display\DisplayRouterInterface;

Removed unused use on commit - nice that these two things are decoupled!

This also broke HEAD...
https://www.drupal.org/pift-ci-job/597199

This fails on 8.3.x because of #2561115: Rollback: Use 'Filter' for exposed filter button text in new views and new installations (instead of 'Apply').

Updated the patch to use the new label.

+1 for the fix. Too bad we work on multiple things at the same time

Second time lucky!

Committed and pushed bfa73c9 to 8.4.x and b4512ef to 8.3.x. Thanks!

Leaving at patch to be ported so that the D7 views issue can be opened - once it is this issue should be closed.

I fixed the unused use again... see #70

I'm marking this fixed today because I'm triaging the patch-to-be-ported queue. There's been no view 7.x issue created in the past 2 months and the people who worked on this should be credited.

Opened #2877370: View's build fails when an unrelated form on the same page has validation errors for views d7 backport