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

Proxy generation fails if magic methods are marked as final. #187

Closed
jmalloc opened this issue Oct 18, 2014 · 9 comments
Closed

Proxy generation fails if magic methods are marked as final. #187

jmalloc opened this issue Oct 18, 2014 · 9 comments
Assignees
Labels
Milestone

Comments

@jmalloc
Copy link

jmalloc commented Oct 18, 2014

The PDO::__wakeup() method is marked as final, but it looks like the final flag of magic methods is not checked as it is for other methods. Hence the code below produces a fatal error:

$factory = new ProxyManager\Factory\LazyLoadingValueHolderFactory();
$factory->createProxy(
    'PDO',
    function () {}
);
Fatal error: Cannot override final method PDO::__wakeup() in ... on line 5
@Ocramius
Copy link
Owner

@jmalloc not sure if I should just deny proxying (as I can't ensure proxy invariants in this case) or silently skip the method. Thoughts?

@Ocramius Ocramius added the bug label Oct 19, 2014
@Ocramius Ocramius added this to the 2.0.0 milestone Oct 19, 2014
@Ocramius Ocramius self-assigned this Oct 19, 2014
@jmalloc
Copy link
Author

jmalloc commented Oct 20, 2014

That's a tough call to make! In regards to __wakeup, specifically, I'd say there's value in allowing the proxies to be created as there's plenty of reason to use a proxy without ever having to serialize/unserialize it (unless this is used internally somewhere?).

@Ocramius
Copy link
Owner

Alright: I'll try fixing this for 1.0.0
On Oct 20, 2014 2:19 AM, "James Harris" notifications@github.com wrote:

That's a tough call to make! In regards to __wakeup, specifically, I'd
say there's value in allowing the proxies to be created as there's plenty
of reason to use a proxy without ever having to serialize/unserialize it
(unless this is used internally somewhere?).


Reply to this email directly or view it on GitHub
#187 (comment)
.

@Ocramius Ocramius modified the milestones: 1.0.0, 2.0.0 Oct 20, 2014
malukenho pushed a commit to malukenho/ProxyManager that referenced this issue Oct 21, 2014
@Ocramius
Copy link
Owner

Solved in #189

@jmalloc
Copy link
Author

jmalloc commented Oct 24, 2014

Awesome, thanks!

Ocramius added a commit that referenced this issue 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)
@githoober
Copy link

Hi @Ocramius,

my question is whether a final method should instantiate a proxy. I have a strong feeling that it should. And because it cannot be overridden a different mechanism should be implemented to make it work.
Any thoughts on that?

@Ocramius
Copy link
Owner

And because it cannot be overridden a different mechanism should be implemented to make it work.

It simply cannot be done. final is final, and that's perfectly ok. If you need to mock/proxy final classes, then you want to proxy their interface instead.

@githoober
Copy link

I agree with an argument about creating a proxy for a final class, but my question is about a regular not final class that can contain some final methods. Since a proxy should be instantiated by any public method, this should also include final public methods. Since overriding is not working in this case, a different way could be researched instead.

@Ocramius
Copy link
Owner

@githoober that depends on the proxy implementation. Right now, we already proxy only methods that can be overridden

In some proxy implementations, the methods need to be all proxied, and therefore it is not possible to proceed with the code generation.

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

No branches or pull requests

3 participants