Skip to content

Fix for omition of the glue on WHERE after the first call in query builder #956

Closed
wants to merge 13 commits into from

7 participants

@Naouak
Naouak commented Mar 3, 2012

As discussed on the mailing list, here is a first fix for the where of the query builder.
It should permit usage of AND and OR in a cleanier way :
$q->where("a = 1")->where("a = 2","OR")->where("b = 1","AND");

will generate :
WHERE a = 1 OR a = 2 AND b = 1

before this patch it would have generated :
WHERE a = 1 AND a = 2 AND b = 1

UPDATE :
Nested is now also available:
$q->where(array("a = 1","a=2"),"OR");
will generate:
WHERE (a = 1 OR a=2)

$q->$q->where(array("a = 1","a=2",array("b=1","b=2")),"OR");
will generate:
WHERE (a=1 OR a=2 OR (b=1 OR b=2))

and finally to do nesting with multiple glue:
$q->$q->where(array("a = 1","a=2",array("b=1",array("b=2","glue"=>" AND ")),"OR");
will generate:
WHERE (a=1 OR a=2 OR (b=1 AND b=2))

See added test to understand how it works.

I also fixed some test on JDatabaseQueryElementTest that were not working on windows because of the difference of end of line characters between systems.

@joomla-jenkins

Unit testing complete. There were 1 failures and 0 errors from 2197 tests and 11594 assertions.
Checkstyle analysis reported 163 warnings and 10 errors.

@chdemko
chdemko commented Mar 3, 2012

You have to correct some coding style:

FILE: ...hdemko/Code/php/joomla/platform/git/libraries/joomla/database/query.php
--------------------------------------------------------------------------------
FOUND 10 ERROR(S) AFFECTING 7 LINE(S)
--------------------------------------------------------------------------------
   68 | ERROR | No space found after comma in function call
   80 | ERROR | The comments for parameters $elements (1) and $glue (2) do not
      |       | align
   80 | ERROR | Expected 2 spaces after the longest type
   80 | ERROR | Expected 2 spaces after the longest variable name
   88 | ERROR | Expected "if (...)\n...{...}\n...else\n"; found
      |       | "if(...)\n...{...}\n...else\n"
   97 | ERROR | No space found after comma in function call
  102 | ERROR | Please put a space between the // and the start of comment
      |       | text; found "//Won't add the glue on the first element"
  103 | ERROR | Expected "if (...)\n...{...}\n...else\n"; found
      |       | "if(...)\n...{...}\n...else\n"
  103 | ERROR | The use of function sizeof() is forbidden; use count() instead
 1479 | ERROR | No space found after comma in function call
--------------------------------------------------------------------------------
@AmyStephen

Above, it is mentioned:

$q->where("a = 1")->where("a = 2","OR")->where("b = 1","AND");

generates:

WHERE a = 1 OR a = 2 AND b = 1

My concern is that the results produce an ambiguous where clause. It's not clear if the developer intended:

A. WHERE (a = 1 OR a = 2) AND b = 1 ?
or
B. WHERE a = 1 OR (a = 2 AND b = 1) ?

Each are very different queries.

Right now, there is a way to specify connectors in an intentional manner within the same where clause.

A. $q->where("((a = 1 OR a = 2) AND b = 1)");
or
B. $q->where("(a = 1 OR (a = 2 AND b = 1)");

Unless I am missing something, I think Joomla has a better approach for developers to use right now. Maybe I am not understanding, though.

@chdemko
chdemko commented Mar 3, 2012

The error in the tests is:

1) JDatabaseQueryElementTest::test__Construct with data set "array-element" (array('FROM', array('field1', 'field2'), ','), array('FROM', array('field1', 'field2'), ','))
Line 173 elements should be set
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     0 => 'field1'
-    1 => 'field2'
+    1 => ','
+    2 => 'field2'
 )

tests/suite/joomla/database/JDatabaseQueryElementTest.php:174
@Naouak
Naouak commented Mar 3, 2012

Code style and test unit error should be fixed now.

AmyStephen:
With the old version you weren't even able to set a query with multiple AND and OR. The where in your query would have been either with only AND or OR.

I have some code to do nested AND and OR but it's kinda complicated to use. You can have a look at it there :
https://github.com/Naouak/joomla-platform/blob/feature/nested-where/libraries/joomla/database/query.php

I didn't tested it yet but it should give you the possibility to define any nesting of your query.

Naouak added some commits Mar 3, 2012
@Naouak Naouak Fixing test for windows
Test were using "\n" for assertion and function used PHP_EOL for line ending.
On windows every test failed because of that.
6cca79b
@Naouak Naouak Adding support for nesting into where function.
Usage should be :
$q->where(
array(
"a=1",
"b=1",
array(
"c=1",
array("c=2","glue"=>"OR")
),
"AND");

Should generate :
WHERE (a=1 AND b=1 AND (c=1 OR c=2))
e534af4
@Naouak Naouak Fixing forgotten assignation in the constructor.
Modifying where function according to new constructor parameters.
Updating Doc Block.
051ecd7
@Naouak Naouak Adding a test case for nesting of elements in a JDatabaseQueryElement eba6e69
@Naouak Naouak Merge branch 'master' into feature/nested-where fa8b92c
@Naouak
Naouak commented Mar 3, 2012

UPDATE :
Nested is now also available:
$q->where(array("a = 1","a=2"),"OR");
will generate:
WHERE (a = 1 OR a=2)

$q->$q->where(array("a = 1","a=2",array("b=1","b=2")),"OR");
will generate:
WHERE (a=1 OR a=2 OR (b=1 OR b=2))

and finally to do nesting with multiple glue:
$q->$q->where(array("a = 1","a=2",array("b=1",array("b=2","glue"=>" AND ")),"OR");
will generate:
WHERE (a=1 OR a=2 OR (b=1 AND b=2))

See added test to understand how it works.

I also fixed some test on JDatabaseQueryElementTest that were not working on windows because of the difference of end of line characters between systems.

@AmyStephen

@Naouak said: "AmyStephen:
With the old version you weren't even able to set a query with multiple AND and OR. The where in your query would have been either with only AND or OR."

Here's how I do a complex where statement with JDatabaseQuery:

$db = JFactory::getDbo();
$query = $db->getQuery(true);

$query->select($db->qn('id'));
$query->select($db->qn('title'));
$query->from($db->qn('#__categories'));
$query->where('(id > 6 OR (id < 4 AND title = "Uncategorised"))');

echo $query->__toString();

$db->setQuery($query);
$results = $db->loadObjectList();

That produces this query statement (and you can see both the OR and the AND are present):

SELECT id,title FROM #__categories WHERE (id > 6 OR (id < 4 AND title = "Uncategorised"))

And, it returns the following expected results (plain Joomla 2.5 installation with no sample data):

array(3) {
[0]=>
object(stdClass)#134 (2) {
["id"]=>
string(1) "2"
["title"]=>
string(13) "Uncategorised"
}
[1]=>
object(stdClass)#116 (2) {
["id"]=>
string(1) "3"
["title"]=>
string(13) "Uncategorised"
}
[2]=>
object(stdClass)#156 (2) {
["id"]=>
string(1) "7"
["title"]=>
string(13) "Uncategorised"
}
}

Give it a try and see if that works for you.

@Naouak
Naouak commented Mar 3, 2012

@AmyStephen The aim of my pull request is to avoid having to write directly the content of the query. As you may know, SQL engine don't always works the same and the aim of that piece of code is to be able to do the same thing with different SQL engine.

If I extrapolate your answer, it's like saying "you could write : $db->setQuery("SELECT * FROM #__table WHERE a = 1 AND (b =1 AND c = 1)"); ".
If you wan't to be sure to be able to support the more SQL engine possible without having to write queries for each, you have to get an abstract layer like the one I added.

@AmyStephen

No, the other parts of the query do not need to be written together and my example doesn't do that, nor does it require it.

When it comes to the WHERE clause and combining OR's and AND's (and NOT's), then it is logic, not JDatabaseQuery, that requires the statement be expressed as a whole in order to produce expected results.

Your first patch produces ambiguous results, so I would be disappointed to see that introduced to core.

Your second patch attempts to address that problem but, in my thinking, presents a far more complex approach to developing the query that many developers will find difficult:

$q->where(array("a = 1","a=2",array("b=1",array("b=2","glue"=>" AND ")),"OR");

Whereas the existing method is pretty straightforward:

$q->where('(a=1 OR a=2 OR (b=1 AND b=2))');

Anyway, that's just my 2 cents on this.

Maybe what is missing is the ability to define parenthesis for grouping statements? That could be handy.

Thanks for working on this. Please don't be discouraged by my comments. It's just how I see it. No big deal. :)

@AmyStephen

@gpongelll - if I am understanding correctly, my response is #956 (comment)

@Naouak
Naouak commented Mar 4, 2012

@AmyStephen
The function before my patch is : where($elements,$glue)
where elements is a string or an array of string and glue the piece that will be before the string and between elements of the array. The glue parameter was only taken into account on the first call of the where function.
If you for example put OR as a glue on your first call, then the where parameters would be only composed of "OR" and you couldn't change that.
My pull request was to correct that behavior which seems to be a bug to me.

Then you came and said it was ambiguous because of missing parameters. So I proposed an update of my patch that permit to define nested parameters so parenthesis would be addable.

Old way of writing the code is still available as my code is fully backward compatible (that was my aim) but in my opinion, it's not a clean way to do it as it's like using an ORM for nothing.

@AmyStephen

@Naouak said "If you for example put OR as a glue on your first call, then the where parameters would be only composed of "OR" and you couldn't change that."

That makes sense to me since predictable results can be produced by always using an AND or always using an OR in a set of WHERE clauses where there is no parenthesis.

Do this IF a or b or c or d.

Do this IF a and b and c and d.

Both are clear. It's when you vary the AND and the OR that you must provide a grouping mechanism for the where clauses.

This is just logic.

@gpongelli

@AmyStephen
You're right about ambiguous result, but look at my test file to know how I've handled these case.

To have
WHERE (a = 1 OR a = 2) AND b = 1
the code is
$q->where('(a = 1 OR a = 2)')->where('b = 1');

The other case is handled with
$q->where('a = 1')->where('(a = 2 AND b = 1)', 'OR');

Easy to understand, simple to do, without use of array that give harder call and harder handling.

Eng. Gabriele Pongelli

@joomla-jenkins

Build triggered by changes to the head.

Unit testing complete. There were 2 failures and 1 errors from 2201 tests and 11594 assertions.
Checkstyle analysis reported 165 warnings and 12 errors.

@joomla-jenkins

Build triggered by changes to the head.

Unit testing complete. There were 2 failures and 1 errors from 2201 tests and 11594 assertions.
Checkstyle analysis reported 165 warnings and 12 errors.

@joomla-jenkins

Build triggered by changes to the head.

Unit testing complete. There were 0 failures and 0 errors from 2201 tests and 11599 assertions.
Checkstyle analysis reported 165 warnings and 12 errors.

@eddieajau eddieajau commented on the diff Mar 4, 2012
...s/suite/joomla/database/JDatabaseQueryElementTest.php
@@ -127,25 +127,25 @@ public function dataTestToString()
'FROM',
'table1',
',',
- "\nFROM table1"
+ PHP_EOL . "FROM table1"
@eddieajau
eddieajau added a note Mar 4, 2012

Could you put the all the PHP_EOL changes into a separate pull? I think that will fix up some problems we are having with windows testing in general and I'd like to get those in while your pull is being reviewed. Thanks!

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

I like where this conversation is heading. Here's my "thinking out loud".

I think we should consider the "glue" to apply to how "this piece" glues to the "last piece", not the glue for all pieces. There is a risk that could introduce a B/C issue if someone had accidentally mixed glues (but it never worked because only the first one was honored). So if that was the case the following query:

$q->where('a=1')
    ->where('b=2')
    ->where('c=3', 'OR')
    ->where('d=4')

could render as:
WHERE a=1 AND b=2 OR c=3 AND d=4

The next step could be that we add a third argument to enclose an array in ( )'s, for example:

$q->where('a=1')
    ->where('b=2')
    ->where(array('c=3', 'd=4'), 'OR', true)

might render as:
WHERE a=1 AND b=2 OR (c=3 ?? d=4)

The obvious problem their is what should ?? be - AND or OR. Ok, we could add a fourth argument (sigh):

$q->where('a=1')
    ->where('b=2')
    ->where(array('c=3', 'd=4'), 'OR', true, 'AND')

might render as:
WHERE a=1 AND b=2 OR (c=3 AND d=4)

Getting messy so let's start over. So, what if we had some new gluer methods called and and or that supported multiple arguments, and did something funky like this:

$q->where(
    $q->and(
        'a=1',
        'b=2',
        $q->or(array('c=3', 'd=4'), 'e=5')
    )
);

rendering to:
WHERE a=1 AND b=2 AND (c=3 OR d=4) OR e=5

the special rule being that an array is always enclosed in ( ). The above should actually be equivalent to:

$q->where('a=1')
    ->where('b=2')
    ->where( 
        $q->or(array('c=3', 'd=4'), 'e=5')
    );

or, if we changed the "glue rule" as I mentioned initially (which is probably a good thing to do anyway), we could have:

$q->where('a=1')
    ->where('b=2')
    ->where( 
        $q->or(array('c=3', 'd=4'))
    );
    ->where('e=5', 'OR');

Just throwing it out there.

@elinw elinw commented on the diff Mar 5, 2012
libraries/joomla/database/query.php
}
else
{
- $this->elements = array_merge($this->elements, array($elements));
+ //Won't add the glue on the first element of the list or of a nesting
@elinw
elinw added a note Mar 5, 2012

Please put a space after the //.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@elinw elinw commented on the diff Mar 5, 2012
libraries/joomla/database/query.php
- $this->append($elements);
+ $this->append($elements,$glue,$nested);
@elinw
elinw added a note Mar 5, 2012

You need spaces after commas in a bunch of places.

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

rendering to:
WHERE a=1 AND b=2 AND (c=3 OR d=4) OR e=5

Andrew - after staring at it awhile, it dawned on me that the result you shared in your example is also an ambiguous where statement.

If bringing this improvement is a goal, though, maybe we should look at what others are doing? I have not looked very closely at the class, probably should do that.

In all honesty, I'm not unsatisfied with the current approach. JDatabaseQuery is very easy to work with and part of it's beauty is that is should be easier for even frontend folks to pick up. Hate to see it get overly complex, either.

Thanks for looking into this (and for building it.)

@eddieajau

Do you mean to say the "result" is ambiguous to MySQL, or the result it's different from what you'd expect given the code example?

Whatever the case, you can construct your entire string by hand if you want - it's all opt in for dev's that see a need for it. But I've come across cases where building nested OR's with implodes and explodes of arrays is just painful to do, not to mention unreadable.

@AmyStephen

No, just simply that in the example shared it's not possible to know for certain what the developer intended:

WHERE a=1 AND b=2 AND (c=3 OR d=4) OR e=5

I suppose MySQL would interpret the results as this (I think, didn't test):

WHERE (a=1 AND b=2 AND (c=3 OR d=4)) OR e=5

Maybe that's good enough? I suppose the tool doesn't have to prevent ambiguous SQL? I might be overthinking this. They can build bad joins - ask for columns that don't exist, and so on.

Anyway, I use JDatabaseQuery all the time. Rarely have I had to break out into native SQL. It's useful because it's simple and powerful. I would just be careful not to lose that.

@eddieajau

Operator precedence is well defined otherwise the whole shebang would fall apart ;)

http://dev.mysql.com/doc/refman/5.0/en/operator-precedence.html

@AmyStephen

Of course. Good night.

@elinw
elinw commented Apr 26, 2012

Andrew about the parentheses comment ...I was working with WHERE and I was wondering why it doesn't support where() the way some other elements to .. i.e.
in some elementdng () to the end of it will automatically wrap the clause in parentheses.
So
$q->where()(array('a=1',b=2'),'OR')
would be
(a=1 OR b=2)

@AmyStephen

At the very least, what Elin is proposing should produce predictable and accurate results.

Personally, I think $q->where('(a = 1 and b = 2)'); is far more intuitive, but maybe there is a use case for the array approach in very complex queries.

@elinw
elinw commented Apr 26, 2012

The arrays are basically if you are using OR as the glue or if you have multiple clauses they save code. If you use AND as the glue you can leave out array since it defaults to AND. i.e if you are using AND normally you would just write
$q->where('a=2','b=3');

at least if you are trying to take advantage of jdatabasequery and not just use it as a wrapper for strings. Of course the cool think is you can indeed stay old school and have the string input.

Anyway I think the element() picking up that is supposed to wrap is a really great thing, it made it possible to do multiple unions with ) UNION ( as the glue but for whatever reason it does not seem to work for where.

@AmyStephen

@elinw -

Your comment makes me wonder if maybe you have identified a new issue.

This issue is intended to allow a developer to change the glue between WHERE clauses, thus combining OR's and AND's.

The challenge we were discussing was how to do so in a predictable way.

After reading your second post, it now sounds like maybe you are discussing another issue related to the WHERE clause - but not related to changing the glue. If so, it's probably a different issue.

@elinw
elinw commented Apr 27, 2012

I was commenting on Andrew's suggestion (note the "Andrew about the parentheses comment .." beginning of my comment specifically addresses his comment and him no one else) about extending the proposed solution.

@elinw
elinw commented Oct 9, 2012

@Naouak This has been sitting unmergable for a while. Do you want to rebase?

@eddieajau

I'm going to close this pull request a) because it's not mergable, and b) because I don't think there is a clear consensus on how this problem should be solved. I suggest, to move the idea behind the pull request forward, a thread is opened on the platform mailing list to see if we can reach an agreement on how to do AND and OR nesting/grouping and then write the code from there.

@eddieajau eddieajau closed this Oct 9, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.