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 Join class in QueryBuilder #3830

Merged
merged 1 commit into from
Jan 18, 2020
Merged

Conversation

BenMorel
Copy link
Contributor

Q A
Type improvement
BC Break yes
Fixed issues none

Summary

This is a subset of #3829, that only replaces the associative array for the JOIN part with a proper class.
Note that this change alone doesn't solve the phpstan issue.

lib/Doctrine/DBAL/Query/QueryPartJoin.php Outdated Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryPartJoin.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 Show resolved Hide resolved
lib/Doctrine/DBAL/Query/QueryBuilder.php Outdated Show resolved Hide resolved
@morozov
Copy link
Member

morozov commented Jan 18, 2020

Code-wise looks good! Do you want to try and remove the whitelisted PHPStan error, then squash everything?

@BenMorel
Copy link
Contributor Author

Do you want to try and remove the whitelisted PHPStan error

Already tried it, still the same error.

then squash everything?

Squashed!

@BenMorel BenMorel changed the title Proper class for 'join' SQL part Introduce Join class in QueryBuilder Jan 18, 2020
@morozov morozov added this to the 4.0.0 milestone Jan 18, 2020
@morozov
Copy link
Member

morozov commented Jan 18, 2020

BC Break: yes

Not sure where it is. Should it be documented in upgrade notes then?

Already tried it, still the same error.

This is surprising. It may make sense to update the reported issue since unlike the earlier implementation, the proposed uses better-defined types.

@BenMorel
Copy link
Contributor Author

Not sure where it is. Should it be documented in upgrade notes then?

It's in getQueryPart(s)(). The returned array is not compatible with the previous one. I'd rather plan to remove these methods altogether, and document it then.

@BenMorel
Copy link
Contributor Author

This is surprising. It may make sense to update the reported issue since unlike the earlier implementation, the proposed uses better-defined types.

Strangely, now it doesn't happen in the sandbox anymore:
https://phpstan.org/r/8dbea40a-8f10-441a-8e83-cdbdceb8bb88

But it definitely does in our codebase. TBH, I'd better avoid wasting more time on this phpstan bug, and invest it in improving this codebase instead. Once all the changes will be merged, the error will be gone, as you can see in #3829 (d8eecdf).

@morozov morozov merged commit c950aab into doctrine:master Jan 18, 2020
@morozov
Copy link
Member

morozov commented Jan 18, 2020

Thanks, @BenMorel!

@BenMorel BenMorel deleted the sql-part-join branch January 18, 2020 23:16
@morozov
Copy link
Member

morozov commented Jan 20, 2020

I'd better avoid wasting more time on this phpstan bug

diff --git a/lib/Doctrine/DBAL/Query/QueryBuilder.php b/lib/Doctrine/DBAL/Query/QueryBuilder.php
index 00eba6b55..f20e466d9 100644
--- a/lib/Doctrine/DBAL/Query/QueryBuilder.php
+++ b/lib/Doctrine/DBAL/Query/QueryBuilder.php
@@ -1298,8 +1298,8 @@ class QueryBuilder
         $sql = '';
 
         if (isset($this->sqlParts['join'][$fromAlias])) {
+            /** @var Join $join */
             foreach ($this->sqlParts['join'][$fromAlias] as $join) {
-                /** @var Join $join */
                 if (array_key_exists($join->alias, $knownAliases)) {
                     throw NonUniqueAlias::new($join->alias, array_keys($knownAliases));
                 }

Not a bug but documented behavior: phpstan/phpstan#466 (comment).

@BenMorel BenMorel mentioned this pull request Jan 20, 2020
@BenMorel
Copy link
Contributor Author

@morozov Well spotted! Fixed in #3835.

@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