Posted by Chadwick Wood on October 2, 2009 at 1:22am
11 followers
| Project: | Views PHP Filter |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
It seems that if you write a filter that returns an empty array, the filter is ignored. Is this a feature or a bug? I have a PHP filter, and when it matches at least one node, in the Query preview on the View edit page I see something like:
WHERE node.nid IN (16,32)
... but when it matches no nodes, there is no WHERE clause. I would think that the view should have no matches in that case, but instead, the filter lets everything through.
Comments
#1
No, you are correct. If nothing is returned, then there should be no WHERE clause and everything should be allowed through, but if an empty array is passed, there should be no matches. I thought that worked correctly, but if it doesn't, I think I must have forgotten to check for that case when I did the refactoring a while back.
Will be happy to accept a patch for this, or will otherwise take a look at this when I have a little time -- probably this coming weekend; I don't think it should be too hard a fix. Thank you.
#2
I've noticed this too.
I have the following code:
<?php
$nodes = array();
[custom php checking here...]
dsm($nodes);
return $nodes;
?>
The output of
dsm($nodes);is... (Array, 0 elements), yet I still get results in my view...A patch would be great! :)
#3
I am also having this problem and would greatly appreciate a patch.
#4
#5
Patch doesn't cover the empty case with NOR.
The proper way to do this, frankly, would be to handle the empty set in medias res. An empty set should be treated as IS/IS NOT NULL in the SQL.
#6
How's this?
I'm not really sure if this code is what you wanted, I just based it off the previous patch and your comments here.
A quick test of a view previously displaying all nodes when the result was empty now shows nothing (as it should be).
#7
That's closer to what I meant. I just wanted it done at the most appropriate place in the code. I'm sorry I've been so nitpicky about it -- I finally just sucked it up and took a look myself. Really, it made the most sense to just get rid of the empty check in the initial if.
Just committed some changes that should address that problem. Once the dev server rolls a new build (should be later today), give that new dev version a try and see if it addresses the problem.
Thank you for your work on this, and again, sorry I was being so picky about where to make the patch.
#8
No problem, maintainer knows best ;)
#9
New dev version is up. Let me know how it works. In the meanwhile I'll start porting to D5.
#10
Hi,
i have the same issue and latest devel does not solve it. If the array is empty, filter is ignored. Anybody knows a workaround ? I love the views php filter :)
#11
Nobody ? I tried it myself but can´t figure out how to do it...
#12
Here is a little workaround: just add a line to your filter like if(count($options) == 0) {$options = '1'; } where options would be your array and 1 would be the id off a default node and you should be fine.
Actually, this has the merrit that you can enter a default value like "Be the first" or whatever. Hope that helps :)
#13
OK, re-patched. Should work this time. No, really, I mean it. :)
Newest version is in CVS; should roll into the dev build in the next automated build cycle.
#14
Backported to 5.
#15
Automatically closed -- issue fixed for 2 weeks with no activity.
#16
The latest dev-Version doesn't contain the fixed views_handler_filter_node_nid_php.inc, but although I replaced it with the last one from the CVS, still all nodes are shown when an empty array is returned. Until that I use the following code (a slight modification of john_the_second's proposal):
if(count($nids) == 0) {$nids = '-1'; }
Raphael
#17
For some reason the latest release (6.x-1.x-dev) is back from Dec 2009... Are you able to update the dev release to include these fixes?
Update: I also manually replaced views_handler_filter_node_nid_php.inc with the one from CVS and it doesn't appear to have fixed the problem... As jakewalk said, an empty array still returns all results.
#18
Ok, I think I found the problem. The attached patch fixes the check for NULL from '==' to '==='.
Working for me...
#19
Hi
applying the patch at #18 results in a mysql warning since an incorrect
where node.nid IN ()is issued. The problem is the empty parenthesis i think .
Here is an improvement of the #18 bwpanda patch. That takes care of the wrong mysql and adds the AND operator to the thing. In fact running with the OR operator an empty result set in the snippet should allow everything else which (without other prior restriction) is pretty much every node on the site.
But people expect this to function primarily as an AND which is defining a subset of nodes.
So i've added the AND patch and set it as default behavior.
also added a typecast to array since the following code
return here_is_an_array_returning_function()seems to evaluate to a
$result===NULLif not typecast.In my opinion the OR behavior shouldn't add a WHERE clause to the result but instead should add a UNION SELECT with the new values. This is subject probably for a new patch ?
#20
This is excellent -- this patch makes the module much more useful for me (I think "AND" is pretty key functionality).
One suggestion (something that tripped me up temporarily) -- when the PHP filter returns an empty array of nodes, the "WHERE" clause from this module doesn't get evaluated -- i.e., an empty array always causes the condition to pass. This is worked around easily enough by checking if the array is empty and returning false if it is, but I think it might make sense for an empty array to be the same as false, since it means no nodes were found matching the condition(s).
#21
+1 to the patch from #19. It works great.
#22
Is there any chance of the changes in the patch from #19 being incorporated into future updates to the dev version of the module? I updated to the most recent dev version, but then had to reapply the changes (manually, since the patch now fails).
#23
What version did you try for patching? I've just tried with the 6.x-1.x-dev , issued November and it worked. The patch didn't fail, not even a warning.
Anyway there's been no reply from the maintainers on this up to now...
Did you also try the OR functionality.?
If you take a look at the code in the original version the OR is a broken AND -hence the title of this tracker. This is why people don't bother with the AND, they use the OR as AND.
if ($this->operator == 'OR') {$this->query->add_where($this->options['group'], "node.nid IN (%s)", implode(",", $result));
}
In my patch is still an AND, just that i've added an empty test - quite pointless as it is right now since this function is broken...didn't have the time to look into it.
if ($this->operator == 'OR') {if(empty($result)) $this->query->add_where($this->options['group'], "true=true"); // nothing was added
else $this->query->add_where($this->options['group'], "node.nid IN (%s)", implode(",", $result));
}
An OR shouldn't add a WHERE but a UNION SELECT. I don't think the views classes support this operation so the or code should add some more rows in case something is output by the php filter.
Right now the filters that work as they should are the AND and the NAND(after applying the patch)
Perhaps this should be moved to another thread dealing with these issues? I think it's a fault in the design here with these broken booleans. Support would be welcome to make this work properly. So it's either the OR is dropped or patched. I'm marking this as 'needs more work'
#24
Filters are always AND'ed together in Views -- that has nothing to do with this module. The text of the module says OR to make the life of those using the numerical node IDs easier -- they don't want different node IDs ANDed, obviously. In retrospect, I see how the semantics I picked could confuse others.
A "filter" with a UNION SELECT would not be a filter. It is also obvious from the code that I'm not changing the standard filter functionality to try to OR a filter in. (As well as from everyone's use, I imagine; it behaves as expected with the singular exception of the blank/null case
The patch in 19 therefore makes a cosmetic fix that would probably best done by not using logical symbols altogether, frankly.
And the reason the patch failed for some is, I imagine, because some folks caught on to what happened here to get this problem reopened in the first place: the commit that I made back in #13 went to HEAD, not to DRUPAL-6--1. Which is why it's not in the dev roll. Pulling HEAD from CVS works fine.
I'm about to commit a cleaner version anyway; I'll change the module option text to make more sense. (And I'll push this one back to 6.x-1.x as well). Thank you.
#25
OK, committed (and backported to 6.x-dev this time; they should show up as soon as the cron job runs, which should be within 12 hours). If this continues, please open another issue -- the patches above are not likely to work with all the code changes made.