Error Using NOT IN () with Arrays for Conditions (Issues #796 & #821) #422

Merged
merged 1 commit into from Jan 18, 2012

Conversation

Projects
None yet
4 participants
Contributor

scottharwell commented Jan 18, 2012

I think that this issue could be solved with the check I added in this commit. It extends the check for and array with 1 value and a string key, and looks for NOT at the end of the key. It passed all of the core unit tests. I will also add a unit test to case #796 since I can't attach them here. I can't think of any other times where this could fail, so I am submitting for integration.

Owner

lorenzo commented Jan 18, 2012

How are you doing the NOT IN ?

Contributor

scottharwell commented Jan 18, 2012

Just submitted attached my test case to Issue #796. http://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/796-using-not-in-for-conditions#ticket-796-4

Basically, if you had an array at the end of a NOT condition with only 1 value, then a SQL error is generated:

$result = $this->Article->find(
        'all',
        array(
            'conditions' => array(
                'Article.id NOT' => array(1)
            )
        )
    );

This is an attempt to prevent the error when using NOT.

Contributor

scottharwell commented Jan 18, 2012

I didn't mean to close the request. Meant to hit comment but hit close and comment.

scottharwell reopened this Jan 18, 2012

Owner

lorenzo commented Jan 18, 2012

The correct way in CakePHP is array('NOT' => array('Article.id' => array(1)), if we are going to support NOT as an operator, we need to do the same for all type of values, not just arrays

Contributor

scottharwell commented Jan 18, 2012

I understand. Per the ticket, I interpreted a solution focused on arrays. I'm happy to broaden the scope and resubmit.

Contributor

scottharwell commented Jan 18, 2012

One more thought...just out of curiosity. Cake currently allows for this syntax when building most queries. In fact, it even works properly as I have listed above except when you have only one element in the array. The issue, per the tickets that I referenced, only happens when there is one element -- but works fine when there are more than one in the array. This fix doesn't change the handling of arrays as you state, but rather fixes the improper SQL that is generated from a 1 element array. Given that this syntax works in those other cases, is it an unintended "feature" and/or workaround or is the plan to build in support to use NOT as I have above (and other users have such as those that reported those tickets).

Owner

markstory commented Jan 18, 2012

It seems a bit lame that the correct query is generated for more than one element, but not for an array with a single element. I think the fix makes sense in that context. While there is an existing better way, this option is currently possibly broken when combined with user input, and we can't really remove the Model.field NOT behavior at this time.

lorenzo reopened this Jan 18, 2012

@lorenzo lorenzo added a commit that referenced this pull request Jan 18, 2012

@lorenzo lorenzo Merge pull request #422 from scottharwell/2.1
Fixing possible SQL error when using 'field NOT' => array(1)
6211be2

@lorenzo lorenzo merged commit 6211be2 into cakephp:2.1 Jan 18, 2012

Owner

lorenzo commented Jan 18, 2012

Thanks @scottharwell :)

Member

ADmad commented Feb 4, 2012

@scottharwell Could you please also add test cases for this.

Contributor

scottharwell commented Feb 5, 2012

@ADmad I added the test to the Light House ticket. Are you asking to add it to this pull request too?

Member

ADmad commented Feb 5, 2012

Yes please a pull request would be nice, and you can just modify the existing test cases files, no need to create new files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment