Fixes #5162 #5855

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
4 participants
@samsonasik
Contributor

samsonasik commented Feb 22, 2014

Fixes #5162

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney Mar 3, 2014

Owner

@ralphschindler Can you update with a milestone, please?

Owner

weierophinney commented Mar 3, 2014

@ralphschindler Can you update with a milestone, please?

tests/ZendTest/Db/Sql/SelectTest.php
+ $sqlStr46 = '( SELECT "foo".* FROM "foo" WHERE a = b ) UNION ALL ( SELECT "bar".* FROM "bar" WHERE c = d ) ORDER BY "a" DESC';
+ $internalTests46 = array(
+ 'processCombine' => array('UNION ALL', 'SELECT "bar".* FROM "bar" WHERE c = d')
+ );

This comment has been minimized.

Show comment Hide comment
@samsonasik

samsonasik Mar 5, 2014

Contributor

@ralphschindler $select46 <- this test case will conflict with test case on #5643 , if #5643 will going to be merge after it, please let me know and I will update the test case or if any suggestion to make no conflict ( new function or something)

@samsonasik

samsonasik Mar 5, 2014

Contributor

@ralphschindler $select46 <- this test case will conflict with test case on #5643 , if #5643 will going to be merge after it, please let me know and I will update the test case or if any suggestion to make no conflict ( new function or something)

This comment has been minimized.

Show comment Hide comment
@ralphschindler

ralphschindler Mar 7, 2014

Member

That's not what I would expect the output to be. If you're doing this:

$select46->combine($select46b, Select::COMBINE_UNION, 'ALL')->order(array('a DESC'));

I am expecting that you're applying an order to $select46, which is the first SELECT. So I'd be expecting, by reading your code, you are attempting to produce this:

( SELECT "foo".* FROM "foo" WHERE a = b ORDER BY "a" DESC ) UNION ALL ( SELECT "bar".* FROM "bar" WHERE c = d )
@ralphschindler

ralphschindler Mar 7, 2014

Member

That's not what I would expect the output to be. If you're doing this:

$select46->combine($select46b, Select::COMBINE_UNION, 'ALL')->order(array('a DESC'));

I am expecting that you're applying an order to $select46, which is the first SELECT. So I'd be expecting, by reading your code, you are attempting to produce this:

( SELECT "foo".* FROM "foo" WHERE a = b ORDER BY "a" DESC ) UNION ALL ( SELECT "bar".* FROM "bar" WHERE c = d )
@samsonasik

This comment has been minimized.

Show comment Hide comment
@samsonasik

samsonasik Mar 8, 2014

Contributor

@ralphschindler I think you're right. I revert the change to Zend\Db\Sql\Select.php and only leave the updated test as your suggestion at #5162 for it. I think the test case is still needed to prove it ;).

Thank you.

Contributor

samsonasik commented Mar 8, 2014

@ralphschindler I think you're right. I revert the change to Zend\Db\Sql\Select.php and only leave the updated test as your suggestion at #5162 for it. I think the test case is still needed to prove it ;).

Thank you.

@ralphschindler ralphschindler added this to the 2.3.0 milestone Mar 11, 2014

@ralphschindler

This comment has been minimized.

Show comment Hide comment
@ralphschindler

ralphschindler Mar 11, 2014

Member

Can you rebase develop (with yesterdays changes to Select.php) and point this PR to develop for me? If you can do that soon, I can get this in without having to use a mergetool ;)

Member

ralphschindler commented Mar 11, 2014

Can you rebase develop (with yesterdays changes to Select.php) and point this PR to develop for me? If you can do that soon, I can get this in without having to use a mergetool ;)

@ralphschindler

This comment has been minimized.

Show comment Hide comment
@ralphschindler

ralphschindler Mar 11, 2014

Member

I think this would be test 48 now

Member

ralphschindler commented Mar 11, 2014

I think this would be test 48 now

ralphschindler added a commit that referenced this pull request Mar 11, 2014

Merge #5855 to develop
Fixed CS
Merge branch 'combineunionorder' of git://github.com/samsonasik/zf2 into hotfix/5855

* 'combineunionorder' of git://github.com/samsonasik/zf2:
  fix T_OBJECT_OPERATOR on php 5.3
  fix array syntax for php 5.3
  undo change on Select.php, and leave the test case
  Fixes #5162

Conflicts:
	tests/ZendTest/Db/Sql/SelectTest.php

ralphschindler added a commit that referenced this pull request Mar 11, 2014

Closes #5855
Merge branch 'hotfix/5855' into develop

* hotfix/5855:
  fix T_OBJECT_OPERATOR on php 5.3
  fix array syntax for php 5.3
  undo change on Select.php, and leave the test case
  Fixes #5162
@ralphschindler

This comment has been minimized.

Show comment Hide comment
@ralphschindler

ralphschindler Mar 11, 2014

Member

Merged to develop (the merging was actually not hard ;) )

Member

ralphschindler commented Mar 11, 2014

Merged to develop (the merging was actually not hard ;) )

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