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

Paginator: Skip limit subquery if not required #7863

Merged
merged 1 commit into from Nov 16, 2019
Merged

Paginator: Skip limit subquery if not required #7863

merged 1 commit into from Nov 16, 2019

Conversation

Seb33300
Copy link
Contributor

This fixes #7829

@SenseException
Copy link
Member

@lcobucci Are these $query->setMaxResults(1); in the older tests not an indicator for a breaking change that needs to target master?

@Seb33300
Copy link
Contributor Author

Seb33300 commented Oct 15, 2019

I feel more like I fixed issues in older tests.

They are both related to the Paginator and tests was failing because they was expecting the Limit Subquery to be applied, but without adding a LIMIT to the query...
And an empty limit will return all data, so this is why my PR is intended to skip the limit query in this case. So only 2 queries will be executed instead of 3.

The only BC that could happen is if you use the SQLLogger to count how many queries have been executed when using a Paginator without LIMIT, like in the failing test:

$this->connection->expects($this->exactly(3))->method('executeQuery');

@lcobucci
Copy link
Member

lcobucci commented Nov 16, 2019

@Seb33300 there's a small change which is not BC compatible here as highlighted by @SenseException: no length + usage of output walkers. But then again, the output walker is useful when performing the subquery.

@lcobucci lcobucci self-assigned this Nov 16, 2019
@lcobucci lcobucci merged commit 8c47dcb into doctrine:2.7 Nov 16, 2019
@lcobucci
Copy link
Member

@Seb33300 🚢

I marked this with BC Break because behaviour might change a tiny bit. I just forgot to document it in UPGRADE.me, will do it separately. Thanks for your contribution.

lcobucci added a commit that referenced this pull request Nov 16, 2019
@Seb33300 Seb33300 deleted the skip-limit-subquery branch November 29, 2019 07:31
vilartoni added a commit to Emagister/doctrine-orm that referenced this pull request Jan 14, 2020
…gin-master

v2.7.0

[![Build Status](https://travis-ci.org/doctrine/orm.svg?branch=v2.7.0)](https://travis-ci.org/doctrine/orm)

This release solves Symfony 5.0 compatibility issues, some small improvements, and adds
various deprecation notices.

Please read carefully the [upgrade to 2.7
notes](https://github.com/doctrine/orm/blob/2.7/UPGRADE.md#upgrade-to-27) to know more
about the reasons and how to fix the deprecation messages.

---

- Total issues resolved: **1**
- Total pull requests resolved: **15**
- Total contributors: **10**

Deprecation
-----------

 - [7911: Be explicit about which Doctrine package in message](doctrine#7911) thanks to @lcobucci
 - [7909: Add deprecation messages](doctrine#7909) thanks to @lcobucci
 - [7901: Add deprecation warnings for 2.7.x](doctrine#7901) thanks to @lcobucci
 - [7701: Split and deprecate AbstractQuery#useResultCache()](doctrine#7701) thanks to @someniatko

CI
--

 - [7904: Make sure composer files are valid](doctrine#7904) thanks to @greg0ire
 - [7600: &doctrine#91;2.7&doctrine#93; CI: Test against PHP 7.4snapshot instead of nightly (8.0)](doctrine#7600) thanks to @Majkl578

Improvement
-----------

 - [7876: Fix compat of commands with Symfony 5](doctrine#7876) thanks to @nicolas-grekas
 - [7829: Skip Paginator LIMIT subquery and WHERE IN if query do not have LIMIT](doctrine#7829) thanks to @Seb33300
 - [7723: Allow Symfony 5.0](doctrine#7723) thanks to @nicolas-grekas
 - [7710: Prettified arrays in tool command orm:mapping:describe](doctrine#7710) thanks to @rtek
 - [7340: Fix config template for PHPUnit >= 7.2](doctrine#7340) thanks to @guilliamxavier

BC Break,Improvement
--------------------

 - [7863: Paginator: Skip limit subquery if not required](doctrine#7863) thanks to @Seb33300

Documentation
-------------

 - [7382: Update homepage](doctrine#7382) thanks to @Majkl578

Bug
---

 - [7326: Cherry-pick doctrine#7307 to fix remaining usages of deprecated ClassLoader and Inflector from doctrine/common](doctrine#7326) thanks to @nicolas-grekas
 - [7079: Fix getJoinTableName for sqlite with schema attribute](doctrine#7079) thanks to @mairo744

BC Break,Deprecation,Improvement
--------------------------------

 - [6803: Deprecation of EntityManager copy method](doctrine#6803) thanks to @SenseException
lcobucci added a commit to lcobucci/doctrine2 that referenced this pull request Feb 15, 2022
Since v2.7.0 the ORM avoids using extra queries via the paginator
component when maximum results isn't set on the original query. With
that change, this test was not executing the code path that it was
expected to run.

This makes sure we trigger the forcing of custom DBAL types when
hydrating the identifiers, making sure we don't introduce bugs.

More info:
- Forcing DBAL type conversion: doctrine#7905
- Issue on optimisation: doctrine#7829
- PR on optimisation: doctrine#7863
- Minor BC-break docs: https://github.com/doctrine/orm/blob/2.11.x/UPGRADE.md#minor-bc-break-paginator-output-walkers-arent-be-called-anymore-on-sub-queries-for-queries-without-max-results
lcobucci added a commit to lcobucci/doctrine2 that referenced this pull request Feb 15, 2022
Since v2.7.0 the ORM avoids using extra queries via the paginator
component when maximum results isn't set on the original query. With
that change, this test was not executing the code path that it was
expected to run.

This makes sure we trigger the forcing of custom DBAL types when
hydrating the identifiers, making sure we don't introduce bugs.

More info:
- Forcing DBAL type conversion: doctrine#7905
- Issue on optimisation: doctrine#7829
- PR on optimisation: doctrine#7863
- Minor BC-break docs: https://github.com/doctrine/orm/blob/2.11.x/UPGRADE.md#minor-bc-break-paginator-output-walkers-arent-be-called-anymore-on-sub-queries-for-queries-without-max-results
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants