Skip to content

ProxyGenerator: Disallow creating proxies for abstract and final classes #771

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

Merged
merged 1 commit into from
Feb 4, 2017

Conversation

Majkl578
Copy link
Contributor

@Majkl578 Majkl578 commented Feb 3, 2017

Fixes #770
Related to symfony/symfony#21509

This could theoretically be a BC break, but I can't imagine any such scenario.

@Ocramius Ocramius added this to the 2.8.0 milestone Feb 4, 2017
@Ocramius Ocramius self-assigned this Feb 4, 2017
@@ -260,6 +260,8 @@ public function setProxyClassTemplate($proxyClassTemplate)
*/
public function generateProxyClass(ClassMetadata $class, $fileName = false)
{
$this->verifyClassValidnessForProxy($class);
Copy link
Member

Choose a reason for hiding this comment

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

verifyClassCanBeProxied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I admit I was out of fantasy. 😇 Renamed.

@@ -306,6 +308,17 @@ public function generateProxyClass(ClassMetadata $class, $fileName = false)
rename($tmpFileName, $fileName);
}

private function verifyClassValidnessForProxy(ClassMetadata $class)
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 relevant @throws annotations in the private/public API of the class being affected by the change

@Ocramius Ocramius removed their assignment Feb 4, 2017
@Majkl578 Majkl578 force-pushed the proxy-disallow-abstact-final branch from dc27414 to c116421 Compare February 4, 2017 20:26
@Ocramius Ocramius self-assigned this Feb 4, 2017
@Ocramius Ocramius merged commit 196ca71 into doctrine:master Feb 4, 2017
@Ocramius
Copy link
Member

Ocramius commented Feb 4, 2017

@Majkl578 awesome, thanks!

@theofidry
Copy link

👌

@pamil
Copy link

pamil commented Aug 14, 2017

Unfortunately, this breaks Sylius (already fixed in dev-master) - looks like we don't really use an entity which was final, so it has started to break after an update to 2.8. If anyone bumped onto this issue as well, after using this workaround https://github.com/Sylius/SyliusElasticSearchPlugin/pull/55/files#diff-b5d0ee8c97c7abd7e3fa29b9a27d1780R41 it works just fine :)

@bendavies
Copy link

bendavies commented Aug 15, 2017

Yes, this breaks using https://github.com/moneyphp/money/ as an embeddable.

@Ocramius
Copy link
Member

Related: doctrine/orm#6625

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

Successfully merging this pull request may close these issues.

5 participants