Joomla! Issue Tracker | Joomla! CMS #10853 - Remove double LIMIT keywords in Pdomysql driver

Fixed in Code Base
5 Sep 2016
Medium
Build: staging
# 10853
Diff
asika32764:patch-9

Success

Success continuous-integration/travis-ci/pr The Travis CI build passed Details
Success JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed. Details

User tests: Successful: Unsuccessful:

asika32764
17 Jun 2016

Summary of Changes

This is a blog page in mysqli driver

This is blog page in pdomysql driver

Since Pdo driver has process limit in setQuery(), we don't need to process it twice.

See https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/database/driver/pdo.php#L714

Testing Instructions

Open any Joomla page with pagination or rows limit in debug mode. You will see double LIMIT in SQL debug console if driver is pdomysql, after this patch, the LIMIT keyword will only appear one.

ea66db8 17 Jun 2016

Get rid of double LIMIT for DB query

asika32764 - open - 17 Jun 2016

asika32764 - change - 17 Jun 2016

Status

New

⇒

Pending

joomla-cms-bot - change - 17 Jun 2016

Labels

Added: ?

asika32764 - change - 17 Jun 2016

The description was changed

asika32764 - change - 17 Jun 2016

The description was changed

brianteeman - change - 17 Jun 2016

Category

⇒

SQL

mbabker - comment - 17 Jun 2016

Something about this feels like a major code smell to me. JDatabaseDriverPdo::setQuery() ignores the limit and offset values that can be set in both the database driver and the query object and relies on values passed when you call setQuery(). Plus, this change doesn't account for the fact that casting a query object to string would still run proessLimit() if values are set in the query object.

Something definitely needs to be fixed, but at a first glance, this doesn't feel like the "right" fix.

asika32764 - comment - 19 Jun 2016

There is a case will make LIMIT appear 3 times. If someone try to override limit by setQuery() with a query (object or string) which has set limit value. However this issue has exists a long time before this PR. see:

try
{
    $db->setQuery($db->getQuery(true)->select('*')->from('#__content')->setLimit(20, 10), 100, 50)->execute();
}
catch (\Exception $e)
{
    echo @end($db->getLog());
}

SELECT *
FROM fan_content LIMIT 10, 20 LIMIT 100, 50 LIMIT 100, 50

This PR can only remove the last 2 LIMIT which set by setQuery(), but the first one which set from query object is still not overridable.

Here is a simple test of this PR:

$db = JFactory::getDbo();

// set string by setQuery()
$db->setQuery('SELECT * FROM #__content', 10, 20)->execute();

echo @end($db->getLog());
echo '<br>';

// set query object by setQuery()
$db->setQuery($db->getQuery(true)->select('*')->from('#__content'), 10, 20)->execute();

echo @end($db->getLog());
echo '<br>';

// set query object with limit
$db->setQuery($db->getQuery(true)->select('*')->from('#__content')->setLimit(20, 10))->execute();

echo @end($db->getLog());
echo '<br>';

// set query object and setQuery() both with limit
try
{
    $db->setQuery($db->getQuery(true)->select('*')->from('#__content')->setLimit(20, 10), 100, 50)->execute();
}
catch (\Exception $e)
{
    echo @end($db->getLog());
    echo '<br>';
}

Before patch

SELECT * FROM fan_content LIMIT 10, 20 LIMIT 10, 20
SELECT * FROM fan_content LIMIT 10, 20 LIMIT 10, 20
SELECT * FROM fan_content LIMIT 10, 20
SELECT * FROM fan_content LIMIT 10, 20 LIMIT 100, 50 LIMIT 100, 50

After patch

SELECT * FROM fan_content LIMIT 10, 20
SELECT * FROM fan_content LIMIT 10, 20
SELECT * FROM fan_content LIMIT 10, 20
SELECT * FROM fan_content LIMIT 10, 20 LIMIT 100, 50

mbabker - comment - 19 Jun 2016

This is still an improvement but something in my gut tells me there's still something being handled incorrectly. I just can't figure out a way to deal with it.

asika32764 - comment - 19 Jun 2016

I also think so, maybe we need unittest to make sure everything works correctly, this PR just a begining.

But I need more suggestion about how to continue work for this fix.

hardiktailored - test_item - 13 Jul 2016 - Tested successfully

hardiktailored - comment - 13 Jul 2016

I have tested this item ✅ successfully on ea66db8

I have tested it and patch removes the double LIMIT keyword from the SQL when using MySQL PDO driver.

_{This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/10853.}

hardiktailored - comment - 13 Jul 2016

Before Patch: