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

Fixing #5887 - lazy loading of one-to-one relationship with custom id object #6274

Merged
merged 1 commit into from
Apr 30, 2017
Merged

Fixing #5887 - lazy loading of one-to-one relationship with custom id object #6274

merged 1 commit into from
Apr 30, 2017

Conversation

necsord
Copy link
Contributor

@necsord necsord commented Feb 3, 2017

Fixing case when lazy loading of entity in one-to-one relationship on the side without foreign key
did not return the entity because of custom type not being detected and properly formatted before
applying to database query.

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.

@necsord thanks for the PR but unfortunately it doesn't solve the issue.

Your test is only passing because you're setting the parameter type on the query (->setParameter('id', $customerId, Issue5887CustomIdObjectType::NAME)).

* @Entity
* @Table(name="issue5887_cart")
*/
class Issue5887Cart
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 class to the test file (like we have here).


namespace Doctrine\Tests\Models\Issue5887;

class Issue5887CustomIdObject
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 class to the test file (like we have here).

use Doctrine\DBAL\Platforms\AbstractPlatform;
use Doctrine\DBAL\Types\Type;

class Issue5887CustomIdObjectType extends Type
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 class to the test file (like we have here).

Copy link
Member

Choose a reason for hiding this comment

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

It can also inherit StringType instead so you don't need to implement the method getSQLDeclaration().

* @Entity
* @Table(name="issue5887_customer")
*/
class Issue5887Customer
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 class to the test file (like we have here).

@@ -16,7 +16,7 @@ static public function dataValidateModelSets()
{
$modelSets = [];
foreach (self::$_modelSets as $modelSet => $classes) {
if ($modelSet == "customtype") {
if (in_array($modelSet, ["customtype", "issue5887"])) {
Copy link
Member

Choose a reason for hiding this comment

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

Not needed for inlined classes

@@ -302,6 +302,10 @@
Models\Issue5989\Issue5989Employee::class,
Models\Issue5989\Issue5989Manager::class,
],
'issue5887' => [
Copy link
Member

Choose a reason for hiding this comment

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

Not needed for inlined classes

@@ -576,6 +580,11 @@ protected function tearDown()
$conn->executeUpdate('DELETE FROM issue5989_managers');
}

if (isset($this->_usedModelSets['issue5887'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Not needed for inlined classes

@necsord
Copy link
Contributor Author

necsord commented Feb 23, 2017

@lcobucci Thanks for looking into this. I've applied changes regarding your comments and simplified test case to test only the most basic case I could come up with.

The problem is not with the first query for Customer which as you pointed out uses type, it's with the second one for Cart initiated in \Doctrine\ORM\UnitOfWork::createEntity:

// Inverse side of x-to-one can never be lazy
$class->reflFields[$field]->setValue($entity, $this->getEntityPersister($assoc['targetEntity'])->loadOneToOneEntity($assoc, $entity));

If you remove the applied change in BasicEntityPersister the test will fail.

Fixing case when lazy loading of entity in one-to-one relationship on the side without foreign key
did not return eht entity because of custom type not being detected and properly formatted before
applying to database query.

Closes #5887
@lcobucci lcobucci added this to the 2.6.0 milestone Apr 30, 2017
@lcobucci
Copy link
Member

@necsord I've just rebased your branch to sync with master and squash the commits. Patch LGTM just waiting for tests to pass and I'll merge it.

Thanks for your contribution 😉

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

3 participants