Omitting $start in entityQuery->range() does not return full result set

I have tested the query and if $length value is set in range(NULL,1), so query will return 1 record only, so i have updated the doc comment for this and added a MR, please review.

The entity query backend for SQL -- as it does for many things -- passes range unaltered to the database query. It does so for many things. While we certainly had the intention to make the system work for any storage backend, they were not implemented at the same time: note the time gap between #1801726: EntityFieldQuery v2 → and #1846454: Add Entity query to Config entities → . Also note how hard it was to query config -- the current implementation didn't appear until #50 (!) in the latter. No wonder small details like range NULL simply was overlooked. A test for "// Add a range to a query without a start parameter" was added but, alas, this used a 0 start instead of a NULL start.

$start NULL then got documented many, many years later in #2711739: Missing summary/param/return docs for several QueryInterface methods → by copy-pasting the doxygen from Database/Query/SelectInterface::range() to Entity/Query/QueryInterface::range() however as this issue shows, not all backends implements range() this way. Indeed, in core we have a KeyValueStore and a Config backend and both does array_slice($result, $this->range['start'], $this->range['length'], TRUE); completely ignoring the fact that $this->range['start'] can be NULL. I am quite surprised more modern PHP doesn't complain a NULL is passed where an int is expected, it sure does like to complain when a NULL is passed in lieu of a string. It's possible it'll start complaining indeed in the future so if the issue ends up with allowing NULL for a start then this definitely needs to be fixed by a simple type cast.

There are multiple ways forward. Deciding is up to you. I am just an old ghost haunting the issue queues. And it's entirely possible there are yet more ways forward I missed. As this issue shows what we all know too well: I certainly can overlook the obvious.

1. Keep the behavior as is and change the doxygen to say: $start NULL has undefined behavior. This is the most BC friendly way. The only code change here would be to the array_slice() calls: add int typecasts.
2. Keep the doxygen and change QueryBase to remove $this->range if NULL is passed in. I think -- but of course all this needs confirmation from someone more knowledgable about current affairs -- this would be an API change and might be tricky to navigate through the BC policy. Once again, I think -- but again please double check -- adhering to BC policies this would take many releases: first passing in NULL would need to be depreciated because the behavior of it changes for non-SQL stored entities and instead of passing NULL to $range there would need to be a new method to remove previously set range. Then, if it is desirable to arrive back to where we are now then in the next release depreciate the new method and remove the NULL depreciation. Then finally remove the new method.

One more thing to note: while currently removing range is perhaps not a common thing to want but maybe with the alter hook finally implemented there might be more use cases for it in the future.

MR is showing 1000s of change so that needs to be addressed.

Can't tell if there is test coverage but there should be.

What is also interesting is that the sister method Select::range() _does_ implement this as documented. So for simplicity it would be good to align these two implementations.

I am also wondering the impact of changing this, I would hazard a guess that there would be comparatively few calls to ->range(NULL, 1) in the wild since this pattern looks a but unnatural. But they will undoubtedly exist, and might happen when passing through data from elsewhere (for example REST API calls might pass 1 parameter only).

I think at least in this issue we should change the documentation to match the existing behaviour, because you can use range(NULL, NULL) to explicitly remove the range anyway.

In slack @chx suggested deprecating the current behaviour when only the first parameter is null, to later throw an exception - maybe we could do that for other range implementations too (in a follow-up) so that people have to use NULL, NULL for that case?

I have checked MR showing 1000+ changes. I think this issue needs a rebase.
please let me know if any other changes required for work on this issue.
Thanks

Issue isn’t mergeable so not ready for review just yet.

I have fixed the mergeable issues, please review.
Thanks

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

All issues and MRs in the core queue should target the 11.x branch. Bug fixes will be backported to the right branch(es) as appropriate. But we always fix everything in "main" first, only we don't have a "main" right now and we're using "11.x" as "main" for now...

Please move the MR target branch back to 11.x, and rebase 3413834-omitting-start-in against 11.x.

Thanks!
-Derek

shalini_jha → changed the visibility of the branch 3413834-omitting-start-in to hidden.

HI , i have added a new MR for this changes against 11.x. please review.

#8 seems to still have an open question

In slack @chx suggested deprecating the current behaviour when only the first parameter is null, to later throw an exception - maybe we could do that for other range implementations too (in a follow-up) so that people have to use NULL, NULL for that case?

Issue summary also still appears to be under discussion.