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

Bugfix - cannot override final methods ( #187 ) #189

Merged

Conversation

malukenho
Copy link
Collaborator

No description provided.

$classGenerator->addMethodFromGenerator(new MagicSleep($originalClass, $initializer, $valueHolder));
$classGenerator->addMethodFromGenerator(new MagicWakeup($originalClass));

$sleep = $originalClass->getMethod('__sleep');
Copy link
Owner

Choose a reason for hiding this comment

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

As discussed on hangouts, this kind of complexity is not sustainable on the long term, and should be abstracted into something like ProxyManager\Generator\ClassGeneratorUtils::addMethodIfNotFinal(ReflectionClass $originalClass, ClassGenerator $classGenerator, MethodGenerator $generatedMethod)

* @author Jefersson Nathan <malukenho@phpse.net>
* @license MIT
*/
class ClassGenerator
Copy link
Owner

Choose a reason for hiding this comment

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

final

Copy link
Owner

Choose a reason for hiding this comment

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

Should probably be renamed to ClassGeneratorUtils

- Rename class name `ClassGenerator` to `ClassGeneratorUtils`
- Fix docblock on `addMethodIfNotFinal` method
- Rename test
@malukenho malukenho force-pushed the bugfix/extends-private-magic-methods branch from b4a9410 to 61ec8c4 Compare October 22, 2014 01:07
GeneratorClass $classGenerator,
MethodGenerator $generatedMethod
) {
if (! $originalClass->hasMethod($generatedMethod->getName())) {
Copy link
Owner

Choose a reason for hiding this comment

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

Cache the call to getName in a variable

malukenho added 2 commits October 22, 2014 10:50
- Fix implementation for allow create methods not existents
@malukenho malukenho force-pushed the bugfix/extends-private-magic-methods branch from d38f214 to 54560e3 Compare October 22, 2014 14:17
@Ocramius Ocramius added this to the 1.0.0 milestone Oct 22, 2014
@Ocramius Ocramius self-assigned this Oct 22, 2014
@Ocramius
Copy link
Owner

Linking #187

$methodName = $generatedMethod->getName();

if ($originalClass->hasMethod($methodName)
and $originalClass->getMethod($methodName)->isFinal()) {
Copy link
Owner

Choose a reason for hiding this comment

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

please don't use and: use &&

@malukenho malukenho force-pushed the bugfix/extends-private-magic-methods branch from 8f65492 to 49af12b Compare October 22, 2014 19:48
$classGenerator->addMethodFromGenerator(new MagicUnset($originalClass, $initializer, $init, $publicProperties));
$classGenerator->addMethodFromGenerator(new MagicClone($originalClass, $initializer, $init));
$classGenerator->addMethodFromGenerator(new MagicSleep($originalClass, $initializer, $init));
ClassGeneratorUtils::addMethodIfNotFinal(
Copy link
Owner

Choose a reason for hiding this comment

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

Looking at it again: you can array_map this stuff :-)

@@ -102,6 +102,7 @@ public function getTestedClasses()
$data = array(
array('ProxyManagerTestAsset\\BaseClass'),
array('ProxyManagerTestAsset\\ClassWithMagicMethods'),
array('ProxyManagerTestAsset\\ClassWithFinalMagicMethods'),
Copy link
Owner

Choose a reason for hiding this comment

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

We also need an additional ClassWithFinalMethods

*
* @return void
*/
public static function addMethodIfNotFinal(
Copy link
Owner

Choose a reason for hiding this comment

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

A test dedicated only to ClassGeneratorUtils is missing

{
public function testCantAddAFinalMethod()
{
$classGenerator = $this->getMock('Zend\\Code\\Generator\\ClassGenerator');
Copy link
Owner

Choose a reason for hiding this comment

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

May want to assert $classGenerator->expects($this->never())->method('addMethodFromGenerator');

Ocramius added a commit that referenced this pull request Oct 23, 2014
…ethods

Bugfix on inherit private magic methods
@Ocramius Ocramius merged commit dd6153d into Ocramius:master Oct 23, 2014
@Ocramius Ocramius changed the title Bugfix on inherit private magic methods Bugfix - cannot override final methods ( #187 ) Oct 24, 2014
Ocramius added a commit that referenced this pull request Dec 12, 2014
Total issues resolved: **17**
- [108: Fix windows path length limitations](#108)
- [172: Alternate hotfix for #108 - windows path length limitations](#172)
- [178: Verify type-safety with `self` and `static` type-hints](#178)
- [180: #178 - `self` type safety check](#180)
- [181: Documentation should be released on github pages on the `gh-pages` branch.](#181)
- [182: Documentation on github pages](#182)
- [187: Proxy generation fails if magic methods are marked as final](#187)
- [189: Bugfix - cannot override final methods ( #187 )](#189)
- [190: [WIP] [EXPERIMENTAL] Codegen should not trigger fatals](#190)
- [191: Put link of documentation on README.md and Close #185](#191)
- [193: [WIP] Codegen errors](#193)
- [194: Code-generation fatal error prevention](#194)
- [195: Blogpost about 1.0.0, 2.0.0 and stability frames](#195)
- [196: Define maintainance time-frames (stable/oldstable/etc)](#196)
- [198: Highlighting the code examples](#198)
- [199: Removed unused `ReflectionMethod` import](#199)
- [202: #196 - adding document with expected stability time-frames](#202)
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.

2 participants