8000 Honor INDEX BY construct in Query::toIterable by ajgarlag · Pull Request #9098 · doctrine/orm · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Honor INDEX BY construct in Query::toIterable #9098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 7, 2021
Merged

Honor INDEX BY construct in Query::toIterable #9098

merged 1 commit into from
Oct 7, 2021

Conversation

ajgarlag
Copy link
Contributor
@ajgarlag ajgarlag commented Oct 6, 2021 •
edited
Loading

Fix the problem explained in #8927 as a bug fix.

Alternative implementation as a feature in #9099.

@ajgarlag ajgarlag force-pushed the bugfix-indexed-iterable branch 4 times, most recently from a3de308 to 583de03 Compare October 6, 2021 06:52
@ajgarlag ajgarlag mentioned this pull request Oct 6, 2021
Closed
@ajgarlag ajgarlag marked this pull request as ready for review October 6, 2021 07:43
greg0ire
greg0ire reviewed Oct 6, 2021
View reviewed changes
lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php Outdated
@@ -196,7 +197,7 @@ public function toIterable($stmt, ResultSetMapping $resultSetMapping, array $hin
$this->cleanupAfterRowIteration();

if (count($result) === 1) {
yield end($result);
yield array_keys($result)[count($result) - 1] => end($result);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this works? Is the key for INDEX BY always the last one for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a polyfill for array_last_key. We have to return the last key, because current code is returning last position of the $result array.

But after reading the code again, if count($result) === 1 it could be rewritten as a simple yield from $result.

@ajgarlag ajgarlag force-pushed the bugfix-indexed-iterable branch 4 times, most recently from 459db29 to 6d96f69 Compare October 7, 2021 10:24
@ajgarlag
Copy link
Contributor Author
ajgarlag commented Oct 7, 2021 •
edited by greg0ire
Loading

@greg0ire After reviewing the code again, I think the problem exposed in #8927 is finally a bug.

I've found that the tests use IterableTester to ensure that the values returned by Query::getResult and Query::toIterable are equivalent. So I've written a small test that breaks it with the addition of the INDEX BY construct to a simple query.

orm/tests/Doctrine/Tests/ORM/Functional/QueryIterableTest.php

Line 82 in a7b4d9a

$query = $this->_em->createQuery('SELECT u FROM ' . CmsUser::class . ' u INDEX BY u.username');

After fixing it, I've found another bug: when selecting multiple entities, the results from Query::getResult and Query::toIterable are different, so I'll open another issue to discuss this problem that, once resolved, should be tested combining INDEX BY and multiple entities.

@greg0ire greg0ire added the Bug label Oct 7, 2021
@greg0ire

Copy link
Member
greg0ire commented Oct 7, 2021

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/2.10.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@ajgarlag ajgarlag mentioned this pull request Oct 7, 2021
Closed
@ajgarlag ajgarlag force-pushed the bugfix-indexed-iterable branch from 6d96f69 to 483e09c Compare October 7, 2021 11:02
@ajgarlag
Copy link
Contributor Author
ajgarlag commented Oct 7, 2021

@greg0ire Squashed!!

@greg0ire greg0ire added this to the 2.10.2 milestone Oct 7, 2021
@greg0ire greg0ire merged commit 1ee68eb into doctrine:2.10.x Oct 7, 2021
@greg0ire
Copy link
Member
greg0ire commented Oct 7, 2021

Thanks @ajgarlag !

@github-actions github-actions bot mentioned this pull request Oct 21, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greg0ire greg0ire greg0ire approved these changes

@SenseException SenseException SenseException approved these changes

@derrabus derrabus Awaiting requested review from derrabus

Assignees
No one assigned
Labels
Bug
Projects
None yet
Milestone
2.10.2
Development

Successfully merging this pull request may close these issues.
3 participants
@ajgarlag @greg0ire @SenseException
0