Skip to content
This repository has been archived by the owner on Jan 29, 2020. It is now read-only.

Di Runtime Compiler Definition #18

Closed
GeeH opened this issue Jun 28, 2016 · 1 comment
Closed

Di Runtime Compiler Definition #18

GeeH opened this issue Jun 28, 2016 · 1 comment

Comments

@GeeH
Copy link
Contributor

GeeH commented Jun 28, 2016

This issue has been moved from the zendframework repository as part of the bug migration program as outlined here - http://framework.zend.com/blog/2016-04-11-issue-closures.html


Original Issue: https://api.github.com/repos/zendframework/zendframework/issues/6748
User: @nickpeirson
Created On: 2014-10-10T17:49:45Z
Updated At: 2015-03-10T07:03:50Z
Body
Seems that the current CompilerDefinition class has not been maintained with changes that have been introduced in the RuntimeDefinition. This results in different class definitions generated by the CompilerDefinition than those generated by the RuntimeDefinition. To resolve this I have created RuntimeCompilerDefinition which extends RuntimeDefinition and adds the functionality of CompilerDefinition. It has been designed to be used as a drop in replacement for the CompilerDefinition, and as such should be usable any where that was previously been used.


Comment

User: @Ocramius
Created On: 2014-10-10T17:58:42Z
Updated At: 2014-10-10T17:58:42Z
Body
@nickpeirson are there any tests for the newly introduced code?


Comment

User: @nickpeirson
Created On: 2014-10-10T18:03:47Z
Updated At: 2014-10-10T18:03:47Z
Body
Not currently.
Thinking about it, would you be happy replacing the current CompilerDefinition with this class? I can't see any reason not to as the current CompilerDefinition produces broken definitions in some cases. If so, I'll move it over to replacing it and update the tests for coverage of the newly introduced methods.
If there's a reason to leave it where it is in this PR, then I'll add the tests against it as it stands.


Comment

User: @weierophinney
Created On: 2015-02-19T19:57:08Z
Updated At: 2015-02-19T19:57:08Z
Body
@nickpeirson GO FOR IT!

If you can do this by 9 March 2015, we can include it in 2.4. :)


Comment

User: @nickpeirson
Created On: 2015-02-20T08:45:27Z
Updated At: 2015-02-20T08:45:27Z
Body
Done. I did have to make one tweak to maintain BC, which I'm not sure about.

In summary, there's a test to see if CompilerDefinition::hasMethodParameters() returns false when the class doesn't exist. However in RuntimeDefinition::hasMethodParameters(), which is where the implementation now comes from, throws a ReflectionException in this case. I've overridden the method in CompilerDefinition to catch the exception and return false, which maintains BC, but my preferred solution would be to change the test and be consistent with RuntimeDefinition.

Maintaining BC seemed to be the path of least resistance, but I'm happy to switch it over if there's a consensus on the small BC break.


Comment

User: @nickpeirson
Created On: 2015-02-20T08:49:35Z
Updated At: 2015-02-20T08:49:35Z
Body
Travis is showing failed, but it looks like the same failures as on develop, but shout if I missed something that I need to fix.


@weierophinney
Copy link
Member

This is fixed with #6, and will release with v3.0.

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

No branches or pull requests

2 participants