get_class_methods() - hide accessors or re-write to call into Reflection* code? #15

Closed
cpriest opened this Issue Dec 29, 2012 · 14 comments

Projects

None yet

2 participants

@cpriest
Owner
cpriest commented Dec 29, 2012

get_class_methods() will return internal accessor function names which is inconsistent with what updates to Reflection do.

Need to either fix get_class_methods() or possibly make get_class_methods() a stub function which asks the Reflection code what is the situation. This would have the advantage of eliminating duplicate code.

Is Reflection standard and here to stay? Thoughts?

@nikic
Collaborator
nikic commented Jan 1, 2013

I'm really not sure this hiding should occur in the first place. My impression of the discussions was that if we stick the accessors in the function table, then they should be fully exposed. I.e. either all or nothing and not something in the middle with checks all over the place.

@cpriest
Owner
cpriest commented Jan 2, 2013

Well there are two aspects to the hiding. One is hiding their existence from reflection. The other is changing errors so they make sense. I don't really care if the functions are hidden or not, I just don't want them exposed when a property is accessed. See the error messages section of the 1.2 RFC for what I mean. But likewise if we're going to put out error messages that make sense then I think it makes sense to go further. Revealing implementation details to userland programmers just seems to be the lazy way out and serves to do nothing but confuse...

@nikic
Collaborator
nikic commented Jan 2, 2013

I'm not arguing the error messages. Those should reference the properties, not the underlying methods. But I'm not sure it makes any sense to half-hide these methods. After all they do exist and are callable. If they wouldn't be accessible, I'd understand, but as they are fully usable from userland...

@cpriest
Owner
cpriest commented Jan 2, 2013

Alright, so if we go the other way then I'll have to change reflection back to not filter out the accessor functions. Should be simple to do that.

@cpriest
Owner
cpriest commented Jan 2, 2013

Then we can close this ticket, since we don't need to worry about hiding the functions. #16, #14, as well.

@nikic
Collaborator
nikic commented Jan 2, 2013

I'd still implement #14 and #16. Error messages are a different thing than reflection.

@cpriest
Owner
cpriest commented Jan 2, 2013

Wait, that makes no sense to me. Why reveal implementation details in some places and hide them in others?

Either userland developers can handle seeing __getFoo() and knowing that means a getter or they can't, right?

@nikic
Collaborator
nikic commented Jan 2, 2013

I never was a fan of having those methods in the first place, but now that we do have them there has to be some compromise as to how much they are exposed.

As far as I understood (if that's wrong then my comments here are bogus) the __getFoo-style methods are callable by the user, i.e. writing $bar->__getFoo() is possible.

So these methods exist and are usable and thus should be shown in Reflection. Reflection is a rather low-level introspection mechanism and if you ask it to give you all methods, then it should give you all methods. Some applications might need to see the internal accessor methods, others might need to filter them. It's not the job of Reflection to decide this.

Error messages are very different from reflection. They're supposed to help the programmer find an error quickly and showing the underlying implementation won't help this goal. Seeing Foo->__setbar('baz') in a backtrace probably will give you a hint at this being a setter invocation, but you'll probably have to lookup this implementation detail unless you already knew it. Something like Foo->bar->set('baz') is more clear (though that's just a sample syntax, maybe there's some better variant).

@cpriest
Owner
cpriest commented Jan 2, 2013

I guess I can see the distinction...

@cpriest
Owner
cpriest commented Jan 3, 2013

You know, in re-reading this. I guess this is why I made Reflection the way it is. I don't consider the __getFoo() to be a method, they are accessors, more than just a method. Meaning that Reflection should reflect that, by way of seeing those accessors through getProperties() and not through getMethods(). Know what I mean?

If I call getMethods() and I see an entry for __getFoo() I have no way to distinguish that between a method someone defined named __getFoo() or a real accessor.

A user defining __getFoo() will not cause $o->Foo to call __getFoo().

@nikic
Collaborator
nikic commented Jan 8, 2013

Resolved by 6ac5dd5, tested by 74c2262.

The accessors are not part of the methods table, they will not be shown by get_class_methods (or reflection).

@nikic nikic closed this Jan 8, 2013
@cpriest cpriest reopened this Jan 8, 2013
@cpriest
Owner
cpriest commented Jan 8, 2013

Re-opened as a flag to update the RFC

@nikic
Collaborator
nikic commented Jan 9, 2013

Added a small note in the RFC:

 These functions are not directly callable by the user, e.g. doing something like $this->{'$foo->get'} will not work. 

Is that sufficient?

@cpriest
Owner
cpriest commented Jan 9, 2013

I'd be curious what Stas thinks but I think that simply omitting any mention of the ability to call it directly makes the most sense. It would confuse the people who have already been thinking of it in terms of being able to call $o->__getFoo() but in the end, omitting the mention would imply that there was never a way to call it directly.

What do you think? It's again an engine implementation detail and so should be in the implementation details document as opposed to the main RFC. Thoughts?

@cpriest cpriest closed this Jan 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment