-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Forward compatibility with DBAL 4 #9730
Conversation
*/ | ||
private $firstResult = null; | ||
private $firstResult = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will require a deprecation of setFirstResult(null). The modifyLimitQuery related changes rhis enables are nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say it's a valid logical next step but not a requirement. From the standpoint of the end result, 0
and null
here are and have always been identical, so this patch doesn't change anything about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a breaking change. Symfony's test suite is now failing because Symfony's ORMQueryBuilderLoader
checks if getFirstResult()
returns null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction: The change on the QueryBuilder
is actually the one that breaks Symfony's DoctrineBridge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What part of the API contract does this change break?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It breaks code that does not treat 0
and null
as equal. One could argue though that such a piece of code has always been broken or fragile at least.
Here's the fix for Symfony: symfony/symfony#46282
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I specifically mean the API contract. If passed null, the component will do this. If passed a zero, it will do that. What has changed? (Thanks for taking care of that, BTW).
Looking at the patch, I'd say you fixed an existing bug. It existed prior to the change in the ORM and now became reproducible via the existing test suite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of $em->createQueryBuilder()->getFirstResult()
has changed from null
to 0
. Do we consider this to be part of the API contract? The doc block says we do:
orm/lib/Doctrine/ORM/QueryBuilder.php
Line 651 in 07ee555
* Returns NULL if {@link setFirstResult} was not applied to this QueryBuilder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, it's a minor BC break. The proper documentation should have said "Returns NULL or 0..." and the clients should have checked for null or zero. Then this change wouldn't have broken anything.
This is a partial backport of #9727. The relevant changes from DBAL 4.0.x are: