Skip to content
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.

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

Fix bug when using Result Cache with Query::toIterable #8495

Merged
merged 2 commits into from Feb 23, 2021

Conversation

Warxcell
Copy link
Contributor

@Warxcell Warxcell commented Feb 21, 2021

Argument 1 passed to Doctrine\ORM\Internal\Hydration\AbstractHydrator::toIterable() must implement interface Doctrine\DBAL\Driver\Statement, instance of Doctrine\DBAL\Cache\ResultCacheStatement given, called in /app/vendor/doctrine/orm/lib/Doctrine/ORM/AbstractQuery.php on line 1010

@Warxcell Warxcell changed the title Fixes Argument 1 passed to Doctrine\ORM\Internal\Hydration\AbstractHy… Fix bug when using Result Cache with toIterable Feb 21, 2021
@Warxcell Warxcell changed the title Fix bug when using Result Cache with toIterable Fix bug when using Result Cache with Query::toIterable Feb 21, 2021
@Warxcell
Copy link
Contributor Author

Error: Method \Doctrine\ORM\Internal\Hydration\AbstractHydrator::gatherRowData() does not have @param annotation for its traversable parameter $id. Error: Method \Doctrine\ORM\Internal\Hydration\AbstractHydrator::gatherRowData() does not have @param annotation for its traversable parameter $nonemptyComponents.

Whyy???

     * @param array<string, string>  &$id                 Dql-Alias => ID-Hash.
     * @param array<string, boolean> &$nonemptyComponents Does this DQL-Alias has at least one non NULL value? 

@Warxcell
Copy link
Contributor Author

Adjusted a bit test, squashed it in 3 commits (add test, fix bug, add typehints).

I hope it's all good now. :)

greg0ire
greg0ire previously approved these changes Feb 21, 2021
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

It looks good to me, although I would squash the failing test and it fix in just one commit: it would not make sense to revert one without also reverting the other. Also, tests should pass on every commit IMO.

@Warxcell
Copy link
Contributor Author

Warxcell commented Feb 22, 2021

It looks good to me, although I would squash the failing test and it fix in just one commit: it would not make sense to revert one without also reverting the other. Also, tests should pass on every commit IMO.

My logic behind this is that somebody could provide better implementation of fix and that would required reverting the fix, but not and the test. :)

@beberlei
Copy link
Member

Isnt adding types a bc break? Hydrators are a regular extension point of the ORM so we need to be extra careful.

@greg0ire
Copy link
Member

@beberlei they all are in an Internal namespace, what about that? Is it not about the BC promise?

@SenseException
Copy link
Member

SenseException commented Feb 22, 2021

I understand and agree that internal classes can have changes that are usually considered a BC break, but all those other type hints and return types are out of scope for the fix and don't add anything to the bugfix.

@greg0ire
Copy link
Member

@Warxcell let's drop that third commit then?

@Warxcell
Copy link
Contributor Author

@Warxcell let's drop that third commit then?

Done. :(

Signed-off-by: Warxcell <warxcell@gmail.com>
@greg0ire greg0ire added the Bug label Feb 22, 2021
@greg0ire greg0ire merged commit ae19f40 into doctrine:2.8.x Feb 23, 2021
@greg0ire greg0ire added this to the 2.8.3 milestone Feb 23, 2021
@greg0ire
Copy link
Member

Thanks @Warxcell !

alexsegura added a commit to coopcycle/coopcycle-web that referenced this pull request Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants