Steps to reproduce:
1) Create a new view against the user table. Continue and Edit.
2) The default user view contains a filter on "User: Active".
3) Get error message as the query contains "(users.status NOT IN ())"

I tried debugging this, but the user status references a boolean handler, and the boolean handler in turn doesn't appear to be able to generate IN-wheres at all, so I don't know how to debug further.

Assigning to dereine as it's a D7 issue.
Happens in -rc1 and -dev.

@dereine If you want, ping me on IRC and give me a hint where I should look, and maybe I can work it out myself :)

CommentFileSizeAuthor
#10 1277518_10_Move-query-logic.patch2.6 KBLetharion
#9 1277518_9_Move-query-logic.patch2.53 KBLetharion
#8 1277518_8_Move-query-logic.patch2.24 KBLetharion
#7 removed_add_less_enthusiastic_query_alter.patch1.1 KBcbeier
#4 add_less_enthusiastic_query_alter.patch1.28 KBmiax

Comments

Letharion’s picture

Letharion commented
Status: Active » Closed (won't fix)

Bah. Third party module running query alters.

Letharion’s picture

Letharion commented
Project: Views (for Drupal 7) » Views Menu Support
Version: 7.x-3.x-dev » 7.x-1.x-dev
Component: user data » Code
Assigned: dawehner » Unassigned
Status: Closed (won't fix) » Active

The above error in Views is caused by a slightly too enthusiastic query alter.

Letharion’s picture

Letharion commented
Title: Filtering on user.status yields incorrect query. » Over-enthusiastic query_alter

If the condition['value'] == 0, this
in_array($condition['value'], $replacements) appears to be true, and the replacement happens.

miax’s picture

miax commented
StatusFileSize
new add_less_enthusiastic_query_alter.patch1.28 KB

I came across this issue also, as Letharion found, but in a different context.
When using views to filter on unpublished nodes the condition['value'] is set to 0 and the replacements happens.
Resulting in that I can not filter a view on unpublished nodes (node.status == 0).

I have created a patch with emattias that seem to fix this issue.

Letharion’s picture

Letharion commented
Status: Active » Fixed
Letharion’s picture

Letharion commented
Status: Fixed » Needs work

That wasn't very clever of me. Will revert the patch, as it generally broke vms.

cbeier’s picture

cbeier commented
StatusFileSize
new removed_add_less_enthusiastic_query_alter.patch1.1 KB

The included patch from #4 was included in the 7.x-1.0-beta1. But the patch broke the vms functionality completely, if the (node) field was not named "vms_field_promo_mlid_direct" or "vms_field_promo_mlid_sub".

I have attached a "patch" that will remove the problematically patch from #4.

Letharion’s picture

Letharion commented
Status: Needs work » Needs review
StatusFileSize
new 1277518_8_Move-query-logic.patch2.24 KB

Yes, my apologies, as I noted in #6, that wasn't my most clever move.

Anyway, I rewrote the relevant logic completely, which should solve both the original problem in a robust way, as well as solve the problem I introduced.

Please try and report back.

Letharion’s picture

Letharion commented
StatusFileSize
new 1277518_9_Move-query-logic.patch2.53 KB
Letharion’s picture

Letharion commented
StatusFileSize
new 1277518_10_Move-query-logic.patch2.6 KB

#9 Introduces better "IS NOT" handling.

Misspelled variable in #9 fixed with this patch.

Letharion’s picture

Letharion commented
Priority: Normal » Critical

Marking critical as the module currently does not properly perform it's main purpose.

knalstaaf’s picture

knalstaaf commented

Just for the sake of clarity: is this fixed in the dev-version? Or does the dev-version require the patch as well?

(I'm using the Submenu tree module for the time being, but I'd like to follow the VMS project because it looks like it provides more flexible solutions.)

Letharion’s picture

Letharion commented

I've been waiting with committing it as I haven't had any feedback. Since it's critical though, and it works for me, I will commit it very soon unless I hear otherwise.

Please give some feedback in case you test it. :)

knalstaaf’s picture

knalstaaf commented

Despite some error notices while patching the dev version it seems to work ok for me.

First I applied the latest patch in the main folder of the module, but it said it couldn't find a certain file (something about line 17 as far as I can remember). Then applied it again on the "handlers" folder, which had a similar notice.

But after that it seems to work...

Letharion’s picture

Letharion commented

Glad the module work well for you :)

With that said: I just tried it, the patch applies cleanly against the latest code. You should get no errors. Maybe you want to read Applying patches for future patch applying.

knalstaaf’s picture

knalstaaf commented

Ok, I patched it using this line:

patch -b < 1277518_10_Move-query-logic.patch

I'm using it for all patches, and it seems to work fine usually. Don't know what the -b stands for though. I should read the "Applying pages" page through.

Edit: (...) and the -b option creates a backup copy of the file before modifying it.

Letharion’s picture

Letharion commented

Most patches are done in -p1 style, and if you don't specify the proper -p level, a lot of patches will fail.

Letharion’s picture

Letharion commented
Status: Needs review » Fixed

Commited

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.