The preview screen when working with views does not process a single value of '0'. Since arguments/context filters could have a value of zero, this is problematic.
the steps to reproduce:
1) enable the 'show the query' setting in views
2) create a standard content view
3) add a context filter for a numeric field - such as node nid
4) type the number 1 into the the "preview with contextual filters" box then click update
at this point, note that the query has the expected "where node.nid=1"
5) type the number 0 into the "preview with contextual filters" box, then click update
now note that the entire condition is gone, when it should say "where node.nid=0"
This is a result of the following code in viewPreviewForm.php which uses empty() to test the input:
if (!empty($form_state['values']['view_args'])) {
$args = explode('/', $form_state['values']['view_args']);
}
Empty, unfortunately doesn't mean "empty", it means without value, so the values of 0 and "0" are considered empty. A simple resolution is to add a test for "$form_state['values']['view_args']==0".
| Comment | File | Size | Author |
|---|---|---|---|
| #42 | patch_applied_successfully.png | 48.9 KB | Munavijayalakshmi |
| #40 | after.png | 85.53 KB | Munavijayalakshmi |
| #40 | before.png | 85.34 KB | Munavijayalakshmi |
| #39 | 2313073-39.patch | 1.99 KB | smustgrave |
| #39 | interdiff-25-39.txt | 813 bytes | smustgrave |
Comments
Comment #1
netw3rker commentedHere's a patch for this.
Comment #3
tim.plunkettComment #8
skylord commentedRerolled for 8.3.x
Comment #9
jwkovell commentedUsing Drupal 8.3.7
Patch applied cleanly and fixed the issue for me.
Comment #10
jwkovell commentedAlso applies and works on Drupal 8.5.x-dev.
I'm not familiar with the automated testing system or how to read the results. The failures seem... unrelated to the patch?
I'm hesitant to switch to "RTBC" unless someone more familiar with the testing system can confirm those errors aren't a problem. Switching to "Needs (further) Review" instead.
Comment #11
lendudeThe fails in #8 look related since they are working with arguments, so this fix is not right (didn't dive into why).
@jkovell if you are in doubt about whether or not a fail is related, try applying the patch to a local Drupal 8 checkout and running the same tests that failed on the testbot locally.
Also, this needs tests.
Comment #19
smustgrave commentedCould the test be like this?
Comment #20
lendude($form_state->getValue('view_args') !== NULL && $form_state->getValue('view_args') == 0))could just be$form_state->getValue('view_args') === 0right?I get it, but strictly speaking an unrelated change
Comment #21
mrinalini9 commentedUpdated patch #19 by addressing #20, please review it.
Comment #23
smustgrave commentedSorry about the $edit = [] change just was annoying to look at.
So don't think === 0 can be used. If you test the patch in #21 manually it doesn't work. (So the test cases worked!)
Adding back 20.1
Comment #24
lendudeIf
=== 0doesn't work, but== 0does, then we seem to be depending on loose typing, which is a bad idea. So this probably gets passed as a string, so it should be=== '0', does that make it pass?Comment #25
smustgrave commentedThat works too
Comment #26
smustgrave commentedComment #27
abhijith s commentedApplied patch #25 and the query is showing correctly after applying this patch.Attaching screen recordings for reference.
Comment #28
smustgrave commentedMoving to RTBC based on comment #27
Comment #29
alexpottI think this should be
if($form_state->hasValue('view_args')) {- simpler logic and more correct.Comment #30
asad_ahmed commentedComment #31
asad_ahmed commentedMade changes as per #29
Comment #32
asad_ahmed commentedPlease ignore the above interdiff, I have mistakenly mentioned 24 as a old patch number which is 25 actually, uploading the right one.
Comment #33
asad_ahmed commentedComment #34
asad_ahmed commentedFixed the above patches and made changes as per #29
Comment #35
asad_ahmed commentedComment #37
alexpottAh interesting... the form API's ::hasValue and isValueEmpty just seem dangerous...
I think the most elegant and logically correct formulation is then:
Comment #38
Ratan Priya commentedComment #39
smustgrave commentedComment #40
Munavijayalakshmi commentedPatch #39 resolved the issue.
Comment #41
Munavijayalakshmi commentedPatch #39 applied successfully in 9.5.x and resolved the issue.
Comment #42
Munavijayalakshmi commentedOn branch 9.5.x.
Comment #43
alexpott@Munavijayalakshmi thanks for the before and after screenshots proving the patch works - that is useful. The screenshot of the patch being applied in #42 is not.
Committed and pushed 43ea903a40 to 10.1.x and b5faa5d0e9 to 10.0.x and 453e372351 to 9.5.x and ac62c5e5e6 to 9.4.x. Thanks!