Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Exceptions thrown from an accessor reveal underlying implementation details #14

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

Comments

Projects
None yet
3 participants
Owner

cpriest commented Dec 29, 2012

Todo: Identify what the current output is and what is desired.

I favor masking the internal implementation detail of a get access from being shown as a function call to __getXXXX() to something else, but this may look funky in a backtrace. Need output of what it would currently show and proposals on how to make it look.

Collaborator

nikic commented Jan 3, 2013

Possible implementation in nikic/php-src@7fee8dd. The output for the exception traces is shown in https://github.com/nikic/php-src/blob/7fee8dd675e89ab0e00739d9dd63be2a129bf1eb/Zend/tests/accessors/exception.phpt.

Foo->__setbar('test')
// becomes
Foo->bar->set('test')

I'm not sure what the best syntax here is, imho the above doesn't show well enough that it is a property. Maybe Foo->$bar->set('test')?

The above commit also fixes #16 by adding the property and accessor elements. It does not yet fix debug_print_backtrace or whatever it's called.

Collaborator

nikic commented Jan 3, 2013

One thing I noticed while writing this is that currently the implementation segfaults when an exception is thrown from a getter. I fixed this by returning an uninited zval ptr. Additionally one should probably include the code for checking indirect modifications as it is done for __get (see https://github.com/nikic/php-src/blob/7fee8dd675e89ab0e00739d9dd63be2a129bf1eb/Zend/zend_object_handlers.c#L479). (Or create a common function for calling both, to avoid copying that code)

Owner

cpriest commented Jan 3, 2013

I think that looks great, I'd prefer the Foo->$bar->set('test') format, that makes it pretty clear. Although if Foo is the class, most elsewhere it would be Foo::$bar->set('test').

If we need that additional code referenced on L479 I agree we should centralize it into a common function. Really, before the shadowing the getter code was leveraging all of that same code, I think it could be re-worked a bit to all use the same code. I've created #24 for that task.

Collaborator

nikic commented Jan 5, 2013

Fixed by cb1ac84. They are now displayed as Foo->$bar->set('test') in the backtrace

@nikic nikic closed this Jan 5, 2013

Owner

cpriest commented Jan 5, 2013

Very nice, love it.

I think this is wrong and should not be done.

Collaborator

nikic commented Jan 5, 2013

@smalyshev You are free to elaborate as to why you think so ;)

I elaborated on this like 9000 times already, but here it goes again: we should not write code that its whole purpose is to misrepresent what is happening in the engine. Not only it is an unnecessary complication which needs to be done everywhere where we touch functions names and would inevitably lead to inconsistencies, it also is greatly confusing, as $bar->set('test') means $bar is an object, and method 'set' is being called on it. But that is not what is happening in reality - in reality, the setter is called on entirely different object and $bar is a variable that can be a number or a string or anything and is not the object of the set() call at all.

Collaborator

nikic commented Jan 5, 2013

we should not write code that its whole purpose is to misrepresent what is happening in the engine.

This does not misrepresent what is happening in the engine. It says that a function $bar->set was called and that's exactly what happened.

Not only it is an unnecessary complication which needs to be done everywhere where we touch functions names and would inevitably lead to inconsistencies, it also is greatly confusing, as $bar->set('test') means $bar is an object, and method 'set' is being called on it.

This change removed the need to add checks everywhere the function name is touched. It's now displayed everywhere the same and through the same mechanisms that are used for ordinary methods.

But that is not what is happening in reality - in reality, the setter is called on entirely different object and $bar is a variable that can be a number or a string or anything and is not the object of the set() call at all.

Owner

cpriest commented Jan 5, 2013

I agree, it makes numerous other places where function names are "shown" much more clear and it also makes it more difficult for developers to try and call these functions directly because the spec will not gaurantee those function names at all. In addition, there was the __set_state() issue by declaring a guarded property of _state.

This also obliterates issue #30 which we decided wasn't worth solving anyways. To me, this change is 100% win.

Owner

cpriest commented Jan 5, 2013

I might add that I could see it being named slightly differently though, I do see Stas's points somewhat. In the end I'm okay with what it is (better than before) but also think that something like set:$Bar might be more clear...

" It says that a function $bar->set was called and that's exactly what happened." Wait, so we have now a function named "$bar->set"? This is a new development and should be reflected in the RFC. I also not sure this is a good change - this breaks with __set and actually removes __ prefix from the function name. In any case, this is a big change and should be explicitly noted. Right now, the RFC says: __FUNCTION__ = __getFoo, so which is it?

Owner

cpriest commented Jan 5, 2013

Yes, this is a new development but was proposed a week or so ago, I was surprised to find it implemented as I hadn't heard anything about it.

Regarding that mention of FUNCTION I will update the RFC to indicate that it will return the actual function name, which should not be relied upon because it is an internal engine feature and not to be relied upon, but that it will return a value.

OK, please add to the RFC then what is the actual state of affairs is. And yes, it should include implementation details too, because it's important for the discussion and understanding the feature. It may be not in the user manual, but it should be in the RFC.

Owner

cpriest commented Jan 5, 2013

I plan to, it's an issue listed here but it's the last that will occur prior to vote because @nikic and I are working hard to get this ready for a vote.

Owner

cpriest commented Jan 5, 2013

I have updated the RFC to indicate the situation.

The RFC still does not mention the functions will be called $foo->get and does mention __setHours. Before voting, I think we need to know what we are voting for.

P.S. now that I understand the change I'm not opposed to removing the special cases, quite the contrary. Just to remove the doubt :)

Owner

cpriest commented Jan 6, 2013

I am purposely stating that for the user land developer that detail is not to be relied upon. It's like buying a car, you don't ask what kind of spark plugs come with it, you assume they will work as long as you drive the car using the steering wheel. It's an internal implementation detail that should be irrelevant to the users. It will definitely go into the updated implementation details doc before vote...

Clint Priest
YAHOO/AIM: pkp9774

On Jan 5, 2013, at 5:48 PM, Stanislav Malyshev notifications@github.com wrote:

The RFC still does not mention the functions will be called $foo->get and does mention __setHours. Before voting, I think we need to know what we are voting for.


Reply to this email directly or view it on GitHub.

Hi!

I am purposely stating that for the user land developer that detail is
not to be relied upon. It's like buying a car, you don't ask what kind
of spark plugs come with it, you assume they will work as long as you
drive the car using the steering wheel. It's an internal implementation
detail that should be irrelevant to the users. It will definitely go
into the updated implementation details doc before vote...

I think before voting for such large and complex feature, especially
that we're voting not on having it developed in principle, but having it
actually merged in PHP right now right this moment in this exact form, of course I want to be sure the details are correct!

It's not like buying a car, it's more like buying a house knowing you'd
have to live in it next 10 years and fix it if something breaks there .
You pretty damn well going to check all the plumbing and ensure it's
solid or no deal.

Stanislav Malyshev, Software Architect
SugarCRM: http://www.sugarcrm.com/
(408)454-6900 ext. 227

Collaborator

nikic commented Jan 6, 2013

@smalyshev Stas, we are not voting on this yet. Once we are voting on this, the internals doc will be up to date. But until then the internal implementation might still change quite a bit, so it makes little sense to document its current state :)

OK, fair enough.

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