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 join when recreation of query from parts. #1295
Conversation
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DDC-3554 We use Jira to track the state of pull requests and the versions they got |
@@ -704,6 +704,10 @@ public function add($dqlPartName, $dqlPart, $append = false) | |||
} | |||
|
|||
$dqlPart = $newDqlPart; | |||
|
|||
if (!$append) { | |||
$isMultiple = false; |
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 naming is a bit confusing: can you find a new variable name?
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.
Can you be a bit more specific about which part is confusing exactly?
Especially since i'm only using existing variables...
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.
@NoUseFreak the unclear part is why a join
would mean $isMultiple = true
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.
@Ocramius Do you propose a different solution that will make it more clear to understand?
Like this:
$isMultiple = is_array($this->_dqlParts[$dqlPartName]) && $dqlPartName != 'join';
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.
@NoUseFreak that's much better/understandable IMO.
… errors initialize() is called sometimes, even when the following code doesn't need the targetPlatform property. Specifically, in AbstractClassMetadataFactory::getAllMetadata(). But as of DBAL 2.5.0, calling Connection::getDatabasePlatform() will make a connection to the database, which means that sometimes it may fail (e.g. you haven't configured your database yet). As a result, calling a method like AbstractClassMetadataFactory::getAllMetadata() - which does not need the targetPlatform - will fail, because determining the targetPlatform requires a connection, which fails. This avoids that - we only get the targetPlatform *when* we need it, which are cases where we're doing things that do indeed need a connection.
I moved the check from the test to the project as users should not know about any logic when adding parts. |
Merged after a rebase at b523896, thanks! |
No description provided.