$dataList->limit(null, $offset); doesn’t work

[kinglozzer]

$list = SomeDataObject::get(); // Let's say 5 items are in the list
$list = $list->limit(null, 3); // Should return 2 items, actually returns 5.

We essentially need to uncomment this assertion and make it pass.

[kinglozzer]

Had a quick look at this, insta-confused. I’ve only looked at master.

I think we’re dealing with a few issues:

1. How we store ->limit() parameters in SQLSelect::setLimit() (a non-numeric limit like null currently means offset is discarded)
2. How DBQueryBuilder::buildLimitFragment() handles non-numeric/falsey limits if an offset is present (it currently doesn’t handle this situation, as SQLSelect::setLimit() means that can never happen)
3. SQLSelect::count() has its own logic for limits that will also need updating.

[kinglozzer]

@tractorcow Nudge nudge, could you take a look at this and see if you think it’s fixable as the ORM is kinda your baby? :)

I’ve just had a slightly more detailed look. The trouble is that you can’t have an offset without a limit in MySQL/SQLite (not sure about PGSQL, docs are a little vague and I don’t have it set up anywhere atm). There are ways around it - LIMIT 18446744073709551610 in MySQL, LIMIT -1 in SQLite3 - so could we add a method to SS_Database called something like getBoundlessLimit that returns an SQL fragment suitable for the current connection?

[tractorcow]

It possibly can be done, but we'd need to check all the various connectors to see if it's possible.

[kinglozzer]

Okay, connectors/possible SQL fragments are:

• MySQL - LIMIT 18446744073709551615 - MySQL docs
• SQLite3 - LIMIT -1 - SQLite docs
• PostgreSQL - either omit limit, or LIMIT ALL - PostgreSQL docs
• MSSQL - not sure about this one as MSSQLQueryBuilder uses ROW_NUMBER() etc and docs on that are a bit thin

Have I missed any connectors? When I get time I’ll set each of them up and give it a shot :)

[tractorcow]

Nice job. ;)

[tractorcow]

In 3.2, check out the DBQueryBuilder class. This is the bit we'll have to modify to generate the appropriate syntax.

[kinglozzer]

@tractorcow Thanks for taking the time to look at this :). Progress so far:

• framework - kinglozzer@8d7b5f1
• SQLite3 - kinglozzer/silverstripe-sqlite3@abcff53
• PostgreSQL - kinglozzer/silverstripe-postgresql@4a9ed25

Unit tests are passing for MySQL, SQLite3 and PostgreSQL. I might add a few different assertions to the tests, as ->count() isn’t very reliable for testing that limit/offset are actually applied: it removes the limit here and instead relies on the stored query parameters.

Would it better to move the MySQL-specific stuff I added to DBQueryBuilder into a new MySQLQueryBuilder class, or is it okay where it is?

Also, how on earth do you test on SQLServer?! I’ve never encountered it before, all I can find online suggests setting up an Azure server to run it on! I don’t think my Macbook will be much use for this :(

[sminnee]

Progress so far looks good.

[kinglozzer]

Closed via 37d6c82