fix hasPublicMethod implementations for abstract functions in ReflectionService #301

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@kmohrf

kmohrf commented Oct 10, 2013

Since php >=5.4.8 the method is_callable returns false for
abstract functions. As the documentation for the
hasPublicMethod method states that it is sufficient
for the class to have a public method with that name it’s
not necessary to check whether the method is abstract or not.

The mentioned method is currently used in the doctrine2-orm
project where it helps with the validation of lifecycle callbacks.
As lifecycle callbacks must be callable but considering the fact
that abstract functions can only be used in abstract classes it
should be OK to rely on php for the assumption that the method
is in fact not abstract when it is called because php would be
unable to instantiate abstract classes.

this fixes DDC-2708.

there are currently no tests in this pull request. i’d volunteer for writing one or two (one for common, one for orm) but i would like to talk to someone first :).

Konrad Mohrfeldt
fix hasPublicMethod implementations for abstract functions in Reflect…
…ionService

Since php >=5.4.8 the method is_callable returns false for
abstract functions. As the documentation for the
hasPublicMethod method states that it is sufficient
for the class to have a public method with that name it’s
not necessary to check whether the method is abstract or not.

The mentioned method is currently used in the doctrine2-orm
project where it helps with the validation of lifecycle callbacks.
As lifecycle callbacks must be callable but considering the fact
that abstract functions can only be used in abstract classes it
should be OK to rely on php for the assumption that the method
is in fact not abstract when it is called because php would be
unable to instantiate abstract classes.

this fixes DDC-2708.
@doctrinebot

This comment has been minimized.

Show comment
Hide comment
@doctrinebot

doctrinebot Oct 10, 2013

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-219

We use Jira to track the state of pull requests and the versions they got
included in.

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DCOM-219

We use Jira to track the state of pull requests and the versions they got
included in.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Oct 10, 2013

Member

The implementation in StaticReflectionService looks wrong to me as it assumes that the class exists (whereas this service is meant to be used during code generation). However, I think the current implementation suffers the same issue.

Member

stof commented Oct 10, 2013

The implementation in StaticReflectionService looks wrong to me as it assumes that the class exists (whereas this service is meant to be used during code generation). However, I think the current implementation suffers the same issue.

@kmohrf

This comment has been minimized.

Show comment
Hide comment
@kmohrf

kmohrf Oct 10, 2013

I have to admit that I didn’t study where the StaticReflectionService is used. The doc comment also states "PHP Runtime Reflection Service" for both implementations. It’s still possible to catch the exception from the ReflectionClass constructor though.

kmohrf commented Oct 10, 2013

I have to admit that I didn’t study where the StaticReflectionService is used. The doc comment also states "PHP Runtime Reflection Service" for both implementations. It’s still possible to catch the exception from the ReflectionClass constructor though.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Oct 17, 2013

Member

@kmohrf what is the use case this PR tries to fix? Can you write a test case for it?

Member

Ocramius commented Oct 17, 2013

@kmohrf what is the use case this PR tries to fix? Can you write a test case for it?

@kmohrf

This comment has been minimized.

Show comment
Hide comment
@kmohrf

kmohrf Oct 21, 2013

Hey, sorry for the delay. I was on vacation :).

@Ocramius Uhm, yeah I have one and it’s actually the reason why I stumbled upon this.
I have a project where I have different kind of entities that need to be approved by a number of people. As the information on the approval is not relevant to the entity (for instance a product) itself it’s separated in a Release entity. But as the approval process differs from entity to entity there have to be specific Release implementations for each entity. So there is an abstract class that implements the general logic behind the process but leaves everything else to the specific implementations. That abstract class is defined as mapped superclass in doctrine as there is a defined way when an approval can be assumed or is just dependent on other entities. That’s handled through lifecycle PreUpdate calls on the abstract release entity. and right now it’s failing for newer PHP versions.

If my example is to weird (the thing when you work on something for too long) let’s just stick with an abstract base entity that implements an autogenerated id and created and updated datetime fields. that should be pretty common and would be implemented via lifecycle callbacks with a vanilla doctrine setup as well.

apart from all that i think that the method names in the ReflectionService implementations are misleading (because hasPublicMethod really has everything to do with a method being public and nothing with being callable) and that PHP, by fixing one of their own bugs, changed your initial implementation.

If you still want some code just say so :).

cheers konrad

kmohrf commented Oct 21, 2013

Hey, sorry for the delay. I was on vacation :).

@Ocramius Uhm, yeah I have one and it’s actually the reason why I stumbled upon this.
I have a project where I have different kind of entities that need to be approved by a number of people. As the information on the approval is not relevant to the entity (for instance a product) itself it’s separated in a Release entity. But as the approval process differs from entity to entity there have to be specific Release implementations for each entity. So there is an abstract class that implements the general logic behind the process but leaves everything else to the specific implementations. That abstract class is defined as mapped superclass in doctrine as there is a defined way when an approval can be assumed or is just dependent on other entities. That’s handled through lifecycle PreUpdate calls on the abstract release entity. and right now it’s failing for newer PHP versions.

If my example is to weird (the thing when you work on something for too long) let’s just stick with an abstract base entity that implements an autogenerated id and created and updated datetime fields. that should be pretty common and would be implemented via lifecycle callbacks with a vanilla doctrine setup as well.

apart from all that i think that the method names in the ReflectionService implementations are misleading (because hasPublicMethod really has everything to do with a method being public and nothing with being callable) and that PHP, by fixing one of their own bugs, changed your initial implementation.

If you still want some code just say so :).

cheers konrad

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Oct 21, 2013

Member

@kmohrf tests are surely required, yes. The fix is just a detail

Member

Ocramius commented Oct 21, 2013

@kmohrf tests are surely required, yes. The fix is just a detail

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Dec 3, 2013

Member

@kmohrf any news?

Member

Ocramius commented Dec 3, 2013

@kmohrf any news?

@kmohrf

This comment has been minimized.

Show comment
Hide comment
@kmohrf

kmohrf Dec 4, 2013

yes! my job is killing my spare time ;). but regarding the intention of your question: I hope to get it done during the weekend because otherwise it will probably take me until christmas.

in any case: i’ll only write tests for what i’ve added here ignoring the fact that @stof has an issue with the StaticReflectionService in both implementations, if nobody wants to address that.

kmohrf commented Dec 4, 2013

yes! my job is killing my spare time ;). but regarding the intention of your question: I hope to get it done during the weekend because otherwise it will probably take me until christmas.

in any case: i’ll only write tests for what i’ve added here ignoring the fact that @stof has an issue with the StaticReflectionService in both implementations, if nobody wants to address that.

@Ocramius

This comment has been minimized.

Show comment
Hide comment
@Ocramius

Ocramius Dec 4, 2013

Member

@kmohrf I wasn't implying that you're not busy, just wasn't sure if this PR was still in your mind.

Member

Ocramius commented Dec 4, 2013

@kmohrf I wasn't implying that you're not busy, just wasn't sure if this PR was still in your mind.

@kmohrf

This comment has been minimized.

Show comment
Hide comment
@kmohrf

kmohrf Dec 4, 2013

i didn’t get it that way :). and yes it’s still on my mind. so as long as you don’t mind the open pull request I’ll resolve it this year :).

kmohrf commented Dec 4, 2013

i didn’t get it that way :). and yes it’s still on my mind. so as long as you don’t mind the open pull request I’ll resolve it this year :).

@beberlei

This comment has been minimized.

Show comment
Hide comment
@beberlei

beberlei Feb 3, 2014

Member

Fixed in 2cb32a6

Member

beberlei commented Feb 3, 2014

Fixed in 2cb32a6

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