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

Introduce properties for SQL parts in QueryBuilder #3836

Merged
merged 1 commit into from
Jan 25, 2020

Conversation

BenMorel
Copy link
Contributor

@BenMorel BenMorel commented Jan 21, 2020

Q A
Type improvement
BC Break yes
Fixed issues none

Summary

This supersedes #3829. Now that #3830 and #3833 have introduced From and Join objects, it's time to replace the $sqlParts associative array with an explicit object, namely QueryParts.

Update: the SQL parts are now part of QueryBuilder itself.

@BenMorel
Copy link
Contributor Author

@morozov Do you know what's wrong with phpcs? How can I early exit here?

@morozov
Copy link
Member

morozov commented Jan 22, 2020

Do you know what's wrong with phpcs? How can I early exit here?

Please try phpcbf:

--- a/lib/Doctrine/DBAL/Query/QueryParts.php
+++ b/lib/Doctrine/DBAL/Query/QueryParts.php
@@ -67,8 +67,10 @@ class QueryParts
             $this->where = clone $this->where;
         }
 
-        if ($this->having !== null) {
-            $this->having = clone $this->having;
+        if ($this->having === null) {
+            return;
         }
+
+        $this->having = clone $this->having;
     }
 }

See slevomat/coding-standard#371 for the reference.

@BenMorel
Copy link
Contributor Author

Please try phpcbf: (...)

Kills readability in this specific case, if you ask me. Should I proceed anyway?

@morozov
Copy link
Member

morozov commented Jan 22, 2020

Kills readability in this specific case, if you ask me. Should I proceed anyway?

I agree (hence the issue). However, I'd prefer to format the code according to the standard instead of having to maintain an exception in phpcs.xml.

@BenMorel
Copy link
Contributor Author

I agree (hence the issue). However, I'd prefer to format the code according to the standard instead of having to maintain an exception in phpcs.xml.

Fixed in 00b4f89.

@BenMorel
Copy link
Contributor Author

I agree (hence the issue). However, I'd prefer to format the code according to the standard instead of having to maintain an exception in phpcs.xml.

Fixed in 00b4f89.

And obsoleted by 42c6062. Problem solved.

@BenMorel BenMorel changed the title Introduce QueryParts object in QueryBuilder Introduce properties for SQL parts in QueryBuilder Jan 22, 2020
@BenMorel
Copy link
Contributor Author

@morozov All good for me. Let me know if you see anything else!

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

All good for me. Let me know if you see anything else!

Apart from the nitpicks, looks good. Love the removal of the add() implementation.

lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
@BenMorel
Copy link
Contributor Author

@morozov All done! Ready for your review.

lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
@BenMorel
Copy link
Contributor Author

@morozov Ready for review, once again.

Copy link
Member

@morozov morozov left a comment

Choose a reason for hiding this comment

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

Apart from the upgrade notes, looks good. Let's get it merged.

UPGRADE.md Outdated Show resolved Hide resolved
@BenMorel
Copy link
Contributor Author

Squashed!

@morozov morozov self-assigned this Jan 24, 2020
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
tests/Doctrine/Tests/DBAL/Query/QueryBuilderTest.php Outdated Show resolved Hide resolved
@morozov
Copy link
Member

morozov commented Jan 24, 2020

@BenMorel after getting this done, if you’re still in the mood for static analysis puzzles like this, you may want to look at #3799. There was some progress done in the past but it seems stuck.

@BenMorel
Copy link
Contributor Author

Please use $this->expectException() instead.

Ugh. All assertions are static, and this method isn't. How can this even work??

Anyway, all fixed, and squashed again.

@morozov morozov merged commit 7f4ef4a into doctrine:master Jan 25, 2020
@morozov
Copy link
Member

morozov commented Jan 25, 2020

Thanks, @BenMorel! All good this time.

@morozov morozov mentioned this pull request Apr 7, 2020
4 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants