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

Fix quoting issues related to ticket #6402 #6404

Closed
wants to merge 3 commits into from
Closed

Fix quoting issues related to ticket #6402 #6404

wants to merge 3 commits into from

Conversation

lemartin
Copy link
Contributor

Apply quoting strategy to foreign key columns of one-to-one relation so
that correct select statements are generated. And use unquoted column
names in result mapping instead of quoted ones, as consumers (namely
IdentifierFlattener) expect unquoted column names.

Apply quoting strategy to foreign key columns of one-to-one relation so
that correct select statements are generated. And use unquoted column
names in result mapping instead of quoted ones, as consumers (namely
IdentifierFlattener) expect unquoted column names.
@lemartin
Copy link
Contributor Author

Is there any way to rerun the Travis CI build? The mariadb problem looks to me like a transient error.

@lcobucci
Copy link
Member

lcobucci commented Apr 19, 2017

@lemartin restarted, I'll also review your PR in a few hours 😉

Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

After restarting the job twice it passed (quite odd tbh).
Besides of these small things it LGTM, any comment @Ocramius

<?php


namespace Doctrine\Tests\ORM\Functional;
Copy link
Member

Choose a reason for hiding this comment

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

Please move this test to Doctrine\Tests\ORM\Functional\Ticket

use Doctrine\Tests\Models\Quote\User;
use Doctrine\Tests\OrmFunctionalTestCase;

class QuoteTest extends OrmFunctionalTestCase
Copy link
Member

Choose a reason for hiding this comment

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

Please refer to the ticket on the class name (e.g. GH6402Test) also adding the PHPUnit's @group 6402 to the class docblock


return $address->id;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Add a line break here plz

Copy link
Contributor

@phansys phansys left a comment

Choose a reason for hiding this comment

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

Status: Needs work.

@@ -1327,7 +1327,7 @@ protected function getSelectColumnAssociationSQL($field, $assoc, ClassMetadata $
$resultColumnName = $this->getSQLColumnAlias($joinColumn['name']);
$type = PersisterHelper::getTypeOfColumn($joinColumn['referencedColumnName'], $targetClass, $this->em);

$this->currentPersisterContext->rsm->addMetaResult($alias, $resultColumnName, $quotedColumn, $isIdentifier, $type);
$this->currentPersisterContext->rsm->addMetaResult($alias, $resultColumnName, $joinColumn["name"], $isIdentifier, $type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, use single quotes.

@lemartin
Copy link
Contributor Author

Thanks for the reviews, sorry for spamming Travis.

@lcobucci lcobucci self-assigned this Apr 30, 2017
@lcobucci lcobucci added the Bug label Apr 30, 2017
@lcobucci lcobucci added this to the 2.6.0 milestone Apr 30, 2017
@lcobucci
Copy link
Member

@lemartin manually merged on 7c6c5d8 talking with @Ocramius we decided to keep in v2.6.x only since it depends on the cleanup made by @guilhermeblanco ages ago.

@Ocramius
Copy link
Member

Ocramius commented May 2, 2017

Handled in #6416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants