1. What database driver (PDO or MySQLi)?
2. Can you var_dump((string) $query); and share that?
3. Since we lacked it, I added an explicit test case for the query object's setLimit method, the CI build shows the tests passing after that commit so the right query is being built by the query object with MySQLi at least.

DatabaseQuery::setLimit() is OK.
DatabaseDriver::setQuery() is guilty. To force it to work you can do $db->setQuery($query, null);

As a temporary solution, for your example, instead of $query->setLImit(1) you can use $db->setQuery($query, 0, 1);

@mbabker tested on both drivers

Dumping the query displays it but without the LIMIT

@csthomas yeah I know $db->setQuery($query, 0, 1); works (as mentioned above), however the setLimit() method should be fixed :)

Well it's hard to fix setLimit() when I added a unit test case and it does exactly what the test expects (which to me proves the method ain't broke). So without some better test cases around $db->setQuery() and $query->setLimit() basically right now I'm seeing a "I need to chase a ghost" issue.

A quick way to fix can be:

diff --git a/libraries/vendor/joomla/database/src/DatabaseDriver.php b/libraries/vendor/joomla/database/src/DatabaseDriver.php
index f4acacba26..ec607336df 100644
--- a/libraries/vendor/joomla/database/src/DatabaseDriver.php
+++ b/libraries/vendor/joomla/database/src/DatabaseDriver.php
@@ -1723,7 +1723,7 @@ abstract class DatabaseDriver implements DatabaseInterface, DispatcherAwareInter
         * @since   1.0
         * @throws  \InvalidArgumentException
         */
-       public function setQuery($query, $offset = 0, $limit = 0)
+       public function setQuery($query, $offset = null, $limit = null)
        {
                $this->connect();

Why? The function signature is no different between the CMS 3.x code and the Framework code (either branch). So if that's your fix that means there is some kind of behavioral difference that needs to be accounted for, I'm not about to make these params nullable out of the blue.

Changes were made inside the setQuery () method, so someone will need to correct it there.

Our code is garbage.

1. The current code in CMS 3.x shouldn't be resetting the values of passed in parameters, it's doing that.
2. The current code in the Framework 2.x branch shouldn't be able to work with null values, our API does not document this support and allowing nulls is wrong here.
3. We're at the point where the limit/offset params can be removed from setQuery() since all of the query objects in the Framework now support the limitable interface and to be frank this should've been the design from day one versus leaving the driver to try and fix that.

I absolutely think this fix is garbage, but you basically need the logic of the CMS method copied to the Framework and we need to deprecate the limit/offset params on the driver API and require them used on the query builder.

Yes.

Off topic: I would like to add some day a code that can generate a sql like SELECT * FROM table LIMIT 1 FOR UPDATE for all database drivers.

Maybe it would be nice to split setLimit() into limit() and offset()?

I’m not totally opposed to that except it feels like change for the sake of
change, there isn’t any real gain. With all the limit query manipulation
finally in the query builder deprecating those params in setQuery just
finishes something started years ago.

On Fri, Jun 22, 2018 at 3:24 PM Tomasz Narloch notifications@github.com
wrote:

Maybe it would be nice to split setLimit() into limit() and offset()?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#20840 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoV93rz85jVhsIUmP5g74CLHJaXQkks5t_VKFgaJpZM4U0HcK
.

--

• Michael Please pardon any errors, this message was sent from my iPhone.

Fixed via 46bcd0e

perfect, thank you @mbabker