Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with JDatabaseQuery __toString #6693

Closed
wants to merge 10 commits into from
Closed

Problem with JDatabaseQuery __toString #6693

wants to merge 10 commits into from

Conversation

fruppel
Copy link

@fruppel fruppel commented Apr 7, 2015

There is an issues with the __toString method of the class JDatabaseQuery.

unionAll did not work as expected, ie the string you entered into the unionAll method was never appended to the SQL query

Steps to reproduce the issue

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

echo $query
    ->select('*')
    ->from('#__table_1')
    ->unionAll(
        $db->getQuery(true)
        ->select('*')
        ->from('#__table_2')
    );

Expected result

The code above should output "SELECT * FROM #__table_1 UNION ALL ( SELECT * FROM #__table_2)".

Actual result

At the moment the output is "SELECT * FROM #__table_1".

Additional comments

This PR also fixes a problem with the ordering of UNION and ORDER BY. You can see how to reproduce and test it here: #6789.

unionAll did not work as expected, ie the string you entered into the unionAll method was never appended to the SQL query
@fruppel
Copy link
Author

fruppel commented Apr 7, 2015

It does not seem to be a problem with the tests, but rather with my file. Travis keeps telling me

FILE: ...home/travis/build/joomla/joomla-cms/libraries/joomla/database/query.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
382 | ERROR | Whitespace found at end of line
387 | ERROR | Whitespace found at end of line

I can't find any whitespaces at these lines.
I edited the file completely on Github, may this be the problem? Sorry - it's my first PR. Any help would be appreciated.

@zero-24
Copy link
Contributor

zero-24 commented Apr 7, 2015

@fruppel i have just send you a PR to fix the whitespaces 😄

https://github.com/fruppel/joomla-cms/pull/1

@fruppel
Copy link
Author

fruppel commented Apr 7, 2015

Thank you @zero-24 👍
I think I found the problem with the whitespaces. The Github editor adds tabs on each new (empty) line which I have to remove next time.

@zero-24
Copy link
Contributor

zero-24 commented Apr 7, 2015

I think I found the problem with the whitespaces. The Github editor adds tabs on each new (empty) line which I have to remove next time.

Exact 😄

@fruppel
Copy link
Author

fruppel commented Apr 16, 2015

Changed the order of "ORDER" and "UNION/UNION ALL" to fix #6789

@fruppel fruppel changed the title unionAll is not working as expected Problems with JDatabaseQuery __toString Apr 16, 2015
@locii
Copy link

locii commented Apr 16, 2015

Confirmed that this pull requests fixes the related issue in #6789.

@sovainfo
Copy link
Contributor

Object to applying order to the set. See #3565

The fix objected #3565
@fruppel fruppel changed the title Problems with JDatabaseQuery __toString Problem with JDatabaseQuery __toString Apr 17, 2015
@fruppel
Copy link
Author

fruppel commented Apr 17, 2015

@sovainfo ok I understand the problem. I reverted my last commit

@RonakParmar
Copy link

I have tested this item 🔴 unsuccessfully on 8d6bb9f

Still not fix for #6789.

Already fixed in #4127 PR.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6693.

@csthomas
Copy link
Contributor

csthomas commented Nov 4, 2016

Can we close it as it is fixed at #10817

@roland-d
Copy link
Contributor

roland-d commented Nov 4, 2016

Closed in favor of #10817


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/6693.

@roland-d roland-d closed this Nov 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants