Do not skip processing all arguments if one "fails"

diff --git a/tests/views_argument_skip_processing.test b/tests/views_argument_skip_processing.test
index bf3ab4d..4cb4717 100644
--- a/tests/views_argument_skip_processing.test
+++ b/tests/views_argument_skip_processing.test
@@ -2,60 +2,36 @@
 
 /**
  * @file
- * Definition of ViewsArgumentValidatorTest.
+ * Definition of ViewsArgumentSkipProcessingTest.
  */
 
 /**
- * Tests Views argument validators.
+ * Tests Views argument skip processing option.
  */
-class ViewsArgumentValidatorTest extends ViewsSqlTest {
+class ViewsArgumentSkipProcessingTest extends ViewsSqlTest {

error: tests/views_argument_skip_processing.test: does not exist in index

Something is wrong here.

The patch in #6 no longer applies to 7.x-3.x-dev; here's a reroll that also fixes a typo and adds a tiny bit of clarification to the help text for the new checkbox.

Before the patch, a Views content pane with multiple contextual filters displayed no results if the first context was not available and the second context was applied. After the patch, it works as expected: the 2nd filter is applied correctly and the results are filtered.

The main tests won't pass until CTools 1.14 is released due to a PHP 5.3 incompatibility, so lets see how it works with PHP 5.6.

CTools 7.x-1.14 was just released, so I triggered a re-test of #7 - it now runs green with PHP 5.3.

Hm, I mixed up the re-roll on the patch in #7. The added checkbox option ended up in the wrong place. See new patch attached.

This bug is related to the too-many-results issue reported after the initial fix to #1863166: Bad arguments when first optional context argument is unavailable. Between this patch and the CTools patch forthcoming on that issue, both sides of the problem should be fixed.

Some code review:

1. +++ b/includes/view.inc
   @@ -859,6 +859,9 @@ class view extends views_db_object {
              $status = $argument->validate_fail($arg);
   +          if (!$argument->skip_processing_arguments_on_fail()) {
   +            continue;
   +          }
              break;
   

   I don't totally understand all the code in the method (it's huge and complex) but I'm wondering if the new if (..) should come before setting $status? My worry is that it will set to a failed status, which might not get reset on the next iteration. There's a lot of conditionals in there, and it looks like it it'd be possible for it to not get reset? It'd be great if someone who knows this code better could chime in on that.

2. +++ b/includes/view.inc
   @@ -900,8 +903,11 @@ class view extends views_db_object {
            $status = $argument->default_action();
   +        if (!$argument->skip_processing_arguments_on_fail()) {
   +          continue;
   +        }
            break;
   

   Same here.

Also, I wonder how D8 handles this? Does it have the same behavior? And, if so, what's the policy with regard to Views and adding features to in D7: do they need to be added in D8 core first?

Thanks for the review @dsnopek. I walked through this function and the places it's called from and wasn't sure exactly what the upshot of it all was.

As far as I can tell, in the original unpatched code, $status is never set inside the loop except in cases where the loop then immediately breaks. So we start out with $status=TRUE and have two places in the loop where it might be set to FALSE. These are:

• If the argument or a default is available, but can't be set;
• If the argument or a default is not available.

In both cases, $status is set based on validation of the default action. I can't tell whether these checks might return TRUE, or if they only ever return FALSE. So the loop will break at the first $status=FALSE, regardless, and it might also break on a $status=TRUE.

The patch adds the possibility that the loop continues to be evaluated even if $status is set to FALSE. There are three things that could be done to $status: Leave it as is, which is most likely FALSE (current approach). Set it explicitly to TRUE. Or set it explicitly to FALSE.

I've tested this in a case where I had three optional contexts and only the middle one was available, and it did the right thing. This implies that letting the $status remain as is is OK. I think additional testing on what happens if the last argument is unavailable, depending on the checkbox, will be helpful.

Interesting.. So, I did a little research on what methods like validate_fail() and default_action() are meant to do. They are to determine if the argument is configured to have the whole view fail if the argument fails. They return TRUE, if the View should still be rendered, and FALSE if not.

So, I'm now really thinking $status shouldn't be set if we're doing a continue. Otherwise, you could have an earlier argument set $status to FALSE, and have that carry through to the return value, but only if all other arguments succeed. Or, an earlier argument could set it to FALSE, to have that FALSE discarded if the following argument fails but resets $status to TRUE. Both of those outcomes are really unexpected from the perspective of the admin building the View.

Maybe if $status is set to FALSE, then we don't continue? That seems like the most expectable outcome: if you configure a View to fail if an argument fails, then that should happen regardless of what the subsequent arguments do.

I'll update the patch in a little bit...

Here's a patch to illustrate what I was thinking. I haven't actually tested it, but there's a test for the issue this patch is trying to fix, so, I'm looking forward to seeing what the testbot says.

Thanks @dsnopek, your reasoning makes sense. Your patch works to fix the issue in the situations I'm testing (works the same as the original and #11).

Rerolled patch #16 to apply to latest dev.

I've been unable to get this test to run locally, so here's a patch that may work. We'll see what the testbot says!

A dumb type-o! Let's try that again.