Problem

There is hardcoded LIMIT and OFFSET query elements in database storage (xnttsql) submodule. It makes impossible to use other SQL statements than mysql/mariadb/posgressql compatible.

Solution

Change hardcoded LIMIT/OFFSET into queryRange() mathod which was earlier forced by CrossSchemaConnectionInterface but it was kicked out in time, but still exists in database drivers, in Connection classes of mysql and pqsql

\Drupal\mysql\Driver\Database\mysql\Connection::queryRange
\Drupal\pgsql\Driver\Database\pgsql\Connection::queryRange

also exists in contrib sqlsrv driver which support is needed for me into External Entities and required Cross Schema should allow me to make my own plugin for SQl Server connections.

\Drupal\sqlsrv\Driver\Database\sqlsrv\Connection::queryRange

It is still active for few years issue in core which wants to deprecate query/queryRange methods into Select class.

CommentFileSizeAuthor
#2 Use_queryRange_instead_of_hardcoded_LIMIT_OFFSET-1.0.patch1.55 KBsebaz

Issue fork external_entities-3623530

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

sebaz created an issue. See original summary.

sebaz’s picture

sebaz commented
StatusFileSize
new Use_queryRange_instead_of_hardcoded_LIMIT_OFFSET-1.0.patch1.55 KB
guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented

I'll try to have a look into this next week.

guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented
Assigned: Unassigned » guignonv

I had a look to your patch but it looses the ORDER BY clause. So it requires some more work. I also had a look to the deprecation posts and it brings new issues in the way we manage queries. XNTTSQL allows to use a set of raw SQL queries for each "CRUD" operation because sometimes, things can be complicated and one needs to perform several queries or call stored procedures to achieve some operations. That's why "query()" is/was used. We can not replace it by a "select()" query, it would not work as expected.

But I would like to have XNTTSQL working with standard SQL so what you pointed out is correct: we should not use hard coded OFFSET and LIMIT. I need to think a little bit more to a more reliable solution... I keep you tuned.

sebaz’s picture

sebaz commented

WHERE and ORDER BY clause are part of $query variable which is param for query() and queryRange() methods. Patch should not change ORDER BY if it is configured as part of CRUD queries.

guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented
Status: Active » Needs review

I just committed a fix based on your good suggestions on this issue fork (see above). Could you try it and let me know if it works for you?
Note: it is using XNTT 3.1 but the same patch would work with 3.0 (and I'll port the patch to both).

guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented
Assigned: guignonv » Unassigned
sebaz’s picture

sebaz commented

It looks like it’s working for me. :)

By the way, it was my mistake — in the patch, I removed the $order_clause variable, which is why the ORDER BY clause disappeared.

guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented
Assigned: Unassigned » guignonv

  • guignonv committed 05b09f6e on 3.0.x 
    Issue #3623530: Use queryRange() instead of hardcoded LIMIT...OFFSET

  • guignonv committed 14bb9018 on 3.1.x 
    Issue #3623530: Use queryRange() instead of hardcoded LIMIT...OFFSET
guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented
Status: Needs review » Reviewed & tested by the community
guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented
Assigned: guignonv » Unassigned
Status: Reviewed & tested by the community » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

guignonv’s picture

guignonv
Primary language French
Location Montpellier
commented

Thanks @Sebaz! I'll add the credits for you. ;)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.