Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.How to reproduce:
* Create a view with 2 arguments
* Pass arguments [NULL, foo]
Expected: Arg2 filters the view
Experienced: View is not filtered, Arg2 and following are silently ignored
Setting major as this breaks a feature with no workaround.
\Drupal\views\ViewExecutable::_buildArguments // see comments
/**
* Builds all the arguments.
*
* @return bool
* TRUE if the arguments were built successfully, FALSE otherwise.
*/
protected function _buildArguments() {
// Initially, we want to build sorts and fields. This can change, though,
// if we get a summary view.
if (empty($this->argument)) {
return TRUE;
}
// build arguments.
$position = -1;
$substitutions = array();
$status = TRUE;
// Get the title.
$title = $this->display_handler->getOption('title');
// Iterate through each argument and process.
foreach ($this->argument as $id => $arg) {
$position++;
$argument = $this->argument[$id];
if ($argument->broken()) {
continue; // =================== Good, following args not ignored
}
$argument->setRelationship();
$arg = isset($this->args[$position]) ? $this->args[$position] : NULL;
$argument->position = $position;
if (isset($arg) || $argument->hasDefaultArgument()) {
if (!isset($arg)) {
$arg = $argument->getDefaultArgument();
// make sure default args get put back.
if (isset($arg)) {
$this->args[$position] = $arg;
}
// remember that this argument was computed, not passed on the URL.
$argument->is_default = TRUE;
}
// Set the argument, which will also validate that the argument can be set.
if (!$argument->setArgument($arg)) {
$status = $argument->validateFail($arg);
break; // =================== Bad, following args ARE ignored
}
if ($argument->isException()) {
$arg_title = $argument->exceptionTitle();
}
else {
$arg_title = $argument->getTitle();
$argument->query($this->display_handler->useGroupBy());
}
// Add this argument's substitution
$substitutions["{{ arguments.$id }}"] = $arg_title;
$substitutions["{{ raw_arguments.$id }}"] = strip_tags(Html::decodeEntities($arg));
// Test to see if we should use this argument's title
if (!empty($argument->options['title_enable']) && !empty($argument->options['title'])) {
$title = $argument->options['title'];
}
}
else {
// determine default condition and handle.
$status = $argument->defaultAction();
break; // =================== Bad, following args ARE ignored
}
// Be safe with references and loops:
unset($argument);
}
// set the title in the build info.
if (!empty($title)) {
$this->build_info['title'] = $title;
}
// Store the arguments for later use.
$this->build_info['substitutions'] = $substitutions;
return $status;
}
| Comment | File | Size | Author |
|---|---|---|---|
| #26 | 2819908-26.patch | 920 bytes | _utsavsharma |
| #26 | interdiff_25-26.txt | 489 bytes | _utsavsharma |
| #25 | 2819908-16.patch | 909 bytes | SimeonKesmev |
| #14 | DeepinScreenshot_select-area_20190813133609.png | 13.09 KB | weynhamz |
| #14 | DeepinScreenshot_select-area_20190813133544.png | 20.44 KB | weynhamz |
Comments
Comment #2
geek-merlinTrivial patch flying in that should fix this. Needs tests.
One might argue this changes a behavior so might break existing sites.
OTOH one might argue that any site relying on such an insane behavior is broken in the first place.
Choose one.
Comment #3
LendudeWon't this be a problem when the first validation fails and the second passes, that the end result is a 'pass' and should be a 'fail'? Didn't dive too deeply into the logic, so I could be wrong, but this sounds iffy security wise.
Comment #4
geek-merlin#3: Ups right, i changed too much. We only need the other "continue".
Comment #5
miteshmapComment #6
miteshmapUpdated according to #3.
Comment #7
miteshmapComment #8
tim_djLooks good and solved my problem
Comment #9
LendudeMoving back to 'needs work' for the tests.
Comment #14
weynhamzCame across same problem recently.
Views module seems has some builtin logic on this, see below comments on
defaultActionmethod.Also,
From what I see, we should only break the loop if the defaultAction returns False.
See attached patch.
Comment #15
Lendude#14 makes sense, lets see what the bot thinks, but this still needs tests regardless
Comment #16
rensingh99 CreditAttribution: rensingh99 at Red Crackle commentedHi,
I have created a view of article content type with two contextual filters "Content: ID" and "Content: Authored by".
Below is an output screenshot before applying the patch with both views arguments.
Below is an output screenshot before applying the patch with the first argument null.
Below is an output screenshot after applying the patch with the first argument null.
I am not getting any different results after applying the patch. Am I doing anything wrong in my testing?
Thanks,
Ren
Comment #23
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
Did not test for the issue
Was previously tagged for tests which still needs to happen.
Comment #24
SimeonKesmev CreditAttribution: SimeonKesmev commentedThe provided patches only handle cases where the provided argument is missing or has a default value. The exact same thing needs to happen where value is provided, but does not validate.
I will also create a test at some point, but for now just adding the correct patch.
Comment #25
SimeonKesmev CreditAttribution: SimeonKesmev commentedThe patch I've uploaded was based on the core repo, which is not usable.
Comment #26
_utsavsharma CreditAttribution: _utsavsharma at OpenSense Labs for DrupalFit commentedTried to fix minor spacing issue in #25.