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.

Sign up for GitHub

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

Jump to bottom

#6625 Skip proxy generation for embeddable classes #6626

Closed
wants to merge 3 commits into from
Closed

#6625 Skip proxy generation for embeddable classes #6626

wants to merge 3 commits into from

Conversation

issei-m
Copy link
Contributor

@issei-m issei-m commented Aug 16, 2017

Fix #6625

@jaikdean
Copy link

This works for me when using a final class as an embeddable 👍 Look forward to it being merged and released!

lcobucci
lcobucci suggested changes Aug 16, 2017
View reviewed changes
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.

LGTM just some small nitpicking 😄

tests/Doctrine/Tests/ORM/Proxy/ProxyFactoryTest.php Outdated

$num = $this->proxyFactory->generateProxyClasses([$cm]);

$this->assertEquals(0, $num, "No proxies generated.");
Copy link
Member

Choose a reason for hiding this comment

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

Please use self::assertSame() instead

tests/Doctrine/Tests/ORM/Proxy/ProxyFactoryTest.php Outdated

$num = $this->proxyFactory->generateProxyClasses([$cm]);

$this->assertEquals(0, $num, "No proxies generated.");
Copy link
Member

Choose a reason for hiding this comment

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

Please use self::assertSame() instead

tests/Doctrine/Tests/ORM/Proxy/ProxyFactoryTest.php Outdated
@@ -75,6 +75,26 @@ public function testReferenceProxyDelegatesLoadingToThePersister()
$proxy->getDescription();
}

public function testSkipMappedSuperClassesOnGeneration()
Copy link
Member

Choose a reason for hiding this comment

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

Please add @group 6625 on the docblock

Copy link
Member

Choose a reason for hiding this comment

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

And add : void

Copy link
Contributor Author

@issei-m issei-m Aug 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please add @group 6625 on the docblock

Although this test case is unrelated with #6625 actually? I just found that test case is missing, thus I added. :)

Copy link
Member

@lcobucci lcobucci Aug 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

True that, sorry 😉

tests/Doctrine/Tests/ORM/Proxy/ProxyFactoryTest.php Outdated
$this->assertEquals(0, $num, "No proxies generated.");
}

public function testSkipEmbeddableClassesOnGeneration()
Copy link
Member

Choose a reason for hiding this comment

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

Please add @group 6625 on the docblock

Copy link
Member

Choose a reason for hiding this comment

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

And add : void

@lcobucci lcobucci added this to the 2.6.0 milestone Aug 16, 2017
@lcobucci lcobucci self-assigned this Aug 16, 2017
@lcobucci lcobucci changed the title Skip embeddable classes proxy generation Skip proxy generation for embeddable classes Aug 16, 2017
@issei-m
Copy link
Contributor Author

issei-m commented Aug 16, 2017

@lcobucci Fixed!

@issei-m
Copy link
Contributor Author

issei-m commented Aug 16, 2017 •
edited
Loading

@lcobucci btw, should this change be shipped to 2.5 branch too, right? The bug seems appeared in 2.5.7.

@Ocramius
Copy link
Member

@issei-m yes, I'm currently doing that.

@Ocramius Ocramius changed the title Skip proxy generation for embeddable classes #6625 Skip proxy generation for embeddable classes Aug 16, 2017
@Ocramius Ocramius modified the milestones: 2.5.9, 2.6.0 Aug 16, 2017
@Ocramius Ocramius assigned Ocramius and unassigned lcobucci Aug 16, 2017
Ocramius added a commit that referenced this pull request Aug 16, 2017
@Ocramius
#6626 #6625 minor CS fixes (removed useless assignments)
71218b6
@Ocramius Ocramius closed this in 9005c5a Aug 16, 2017
Ocramius added a commit that referenced this pull request Aug 16, 2017
@Ocramius
#6626 #6625 minor CS fixes (removed useless assignments)
8f77210
Ocramius added a commit that referenced this pull request Aug 16, 2017
@Ocramius
#6625 #6626 removing PHP 5.3 incompatibilities (required for backport)
ee22be2
Ocramius added a commit that referenced this pull request Aug 16, 2017
@Ocramius
Merge branch 'fix/#6626-skip-proxy-generation-for-embeddable-classes-…
6184343 
…2.5' into 2.5

Backport #6626
Backport #6625
@Ocramius
Copy link
Member

Rebased and manually merged, thanks @issei-m!

master: 9005c5a
2.5: 6184343

@Ocramius Ocramius mentioned this pull request Aug 16, 2017
Closed
@issei-m issei-m deleted the skip-embeddable-proxy-generation branch August 16, 2017 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcobucci lcobucci lcobucci approved these changes

Assignees

@Ocramius Ocramius

Labels
Bug Regression
Projects
None yet
Milestone
2.5.9
Development

Successfully merging this pull request may close these issues.
4 participants
@issei-m @jaikdean @Ocramius @lcobucci