Wow, I'm surprised by all the talk about this RFC this time around. I posted
this numerous times in the past trying to elicit feedback and got little to
none, so I took the time to write it as I thought it should be written. Some
of these things will take considerable effort to fix/correct/change.
I'll try to address everyone's comments/questions in one post:
________________________________
Nikita:
> What concerns me with the current implementation is that it leaks many
> implementation details, in particular the fact that the accessors are
> implemented as *real* __getXYZ methods and automatic implementations also use
> *real* $__XYZ properties.
>
> A few examples:
>
> ## 1 - __getProperty() method directly callable
>
> class Test {
> public $property {
> get { return 123; }
> }
> }
>
> $test = new Test;
> var_dump($test->property); // int(123)
> var_dump($test->__getProperty()); // int(123)
>
I don't particularly see this as a problem (the ability to use it as a getter
or call it as a function), but see my comments at the end. I also don't
particularly see the point of auto implemented getters/setters ( public $h {
get; set; } )
> ## 2 - __getProperty() method exposed via exception
>
> class Test {
> public $throwingProperty {
> get { throw new Exception; }
> }
> }
>
> (new Test)->throwingProperty;
>
> exception 'Exception' in /home/nikic/dev/php-src/t29.php:9 Stack trace:
> #0 /home/nikic/dev/php-src/t29.php(31): Test->__getthrowingProperty()
> #1 {main}
>
I have gone to great lengths to shield the implementation details from users,
this one slipped past me. Nearly all errors that are shown reference a
property rather than a function name and this could also be cleaned up.
> ## 3 - Can directly access $__automaticProperty and even unset it (causing
> notices in the internal code)
>
> class Test {
> public $automaticProperty {
> get; set;
> }
>
> public function getAutomaticProperty() {
> return $this->__automaticProperty;
> }
>
> public function unsetAutomaticProperty() {
> unset($this->__automaticProperty);
> }
> }
>
> $test->automaticProperty = 'foo';
> var_dump($test->getAutomaticProperty());
> $test->unsetAutomaticProperty();
> var_dump($test->automaticProperty);
>
> string(3) "foo"
>
> Notice: Undefined property: Test::$__automaticProperty in
> /home/nikic/dev/php-src/t29.php on line 13 NULL
I'm not even sure that automatic backing fields are even desired, I never felt
the need to have them in C# and the only reason they were included is because
they were a part of Dennis's original proposal. Eliminating them would
eliminate this as an issue.
>
> =====
>
> I feel like this approach to the implementation will be a big can of worms.
> Sure, it works, but it is rather fragile and the enduser ends up dealing with
> internal stuff which he ought not care about. I think it would be better to
> cleanly separate out the accessor implementation. It might require more code
> now, but will be better in the long run.
All three of these issues, could be addressed I believe by not having the
functions and/or properties a part of the ordinary HashTables, or to have flags
set on them. I believe there is already a SHADOW flag type defined which may
be able to be used for this type of functionality.
________________________________
Leigh:
> Further to this, take the following example.
>
> public $_state {
> set { ... }
> }
>
> This automatically generates a "hidden" function __set_state(), which is a
> magic method already (and should be declared static, and take an array as
> it's only parameter)
>
> Another fine example of how automatically generating *real* implementations
> can potentially break things.
This would clearly cause problems, again this could possibly be addressed by
not having the accessor functions be a part of the standard Functions
HashTable, the functions would not even need to have names at that point if
they are not intended to be accessible directly.
> public $property {
> set { $this->property = ($this->property*2)+$value } get; }
>
> How do I reference the property being set from within the function? The way I
> have done it in the example will cause recursion? How can I assign to "self"?
>
> How do I set the default value of $property when the object is created?
> Surely I don't have to reverse the set accessors logic and set the inverse in
> __construct?
Generally speaking, I don't know why you would use an automatic backing getter
with a separate setter, but if you wanted to do that, at present you would
access the automatic backing field by $this->__property.
The above will not cause recursion, it is protected from recursion by the same
mechanism that __get() and __set() are protected. In fact, the above code
would set an actual property named $property on the object which would then
side-step the accessor entirely (true properties take precedence over
accessors, though they may only be "set" from within the setter of the
accessor. This may be a bit confusing, but I specifically wrote it this way
for the purpose of lazy-loading.
For example:
public $objList {
get { return $this->objList = new myList(); }
set { $this->objList = $value; }
}
The first access of the $objList property getter would create the object and
attempt to set its-self which would be passed to the setter, the setter (now
guarded) will directly set the property on the object, further calls to
$objList would retrieve the already created object (bypassing the accessor).
To get out of that situation, you would simply unset($objList) and the accessor
would take over again.
Please keep in mind that this is only possible from within the setter of the
property, nothing outside the setter of the property can create a real property
named the same as the accessor, except the setter of the accessor.
________________________________
Nathan:
> This would be much less confusing as it follows other PHP standards for
> creating functions and such. I know C# has a similar syntax to what is
> proposed, but we are not developing for C# we are developing for PHP which
> has its own syntax rules that differ from C#'s and my vote is to follow PHP's
> already in existent syntax format.
I think having the function keyword there is superfluous, it adds more code to
be read, while accessors have the powers of a function, they are quite
different from a function. You would probably never have a 35 line getter, for
example.
________________________________
Rasmus:
> My only remaining comment is on the read-only and write-only keywords...
> this seems really superfluous and strange to me - the syntax (using a
> hyphenated keyword) and the feature itself, is way off the grid as compared
> to other languages.
There is an important distinction with the read-only and write-only keywords
and they are analogous to the final keyword. The final keyword prevents any
subclasses from over-riding something declared as final. Final is supported
for a property as well. The difference here is that a read-only property is
semi-final. Subclasses may redefine a get declared as read-only but may not
declare a setter. Personally I am not a fan of the concepts of final,
read-only or write-only. They were included only because I thought the
original RFC for which I was implementing had been ratified and the feature was
desired.
________________________________
Johannes:
> About the items in regards to reflection in there:
>
> The RFC states
> ReflectionClass::getMethods() will not return accessor functions
> (hides implementation detail).
> Up until now reflection is leaky and is telling the truth. We should either
> keep that or completely clean up reflection. (mind also
> get_class_methods() and such)
I have not done anything with get_class_methods() but I certainly could.
Probably a better solution would be to have get_class_methods() use Reflection
to do its work, so that there is not duplicate code in the core?
I see no reason to continue leaking new code, if someone wants to head up an
effort to make the rest of Reflection not leak implementation details then
great, but to purposely leak implementation details for new features just
doesn't make sense to me.
> The RFC also introduces a new class ReflectionPropertyAccessor and has
> methods returning ReflectionProperty and ReflectionPropertyAccessor.
> there should either be a common base class or interface (see i.e.
> ReflectionFunctionAbstract)
>
I'm not certain there is a point to having a new base class, I would need to
look at the code but I would bet that ReflectionPropertyAccessor is a sub-class
of ReflectionProperty. In any case, we have the instanceof operator which can
be used to distinguish between the results, no?
> What will ReflectionPropertyAccessor->getGetter()->getName() return? I guess
> reflection will be leaky there? (see first item)
I wrote the reflection additions very early on so I would need to look at the
code, but I am pretty certain that the above would return the name of the
property, not the function.
________________________________
Jazzer:
> I think Leigh brings up some important flaws to in the current RFC. What
> Leigh is asking for does not appear to be possible, and in my opinion, it
> should be.
I'm really not quite clear on what Leigh was going for with the code she
indicated, I think she was trying to demonstrate infinite recursion which is
not possible due to guards.
> I also agree with Rasmus, to a certain extent.
> By putting only a getter/setter, the developer essentially sets the property
> as read or write only. At first glance, there is no need for the read-only or
> write-only keyword. Except... subclassing may or may not be an issue.
>
> By setting a property to read-only, it ensures that 'set' can never be set by
> a subclass. Unless I redefine the entire property... or is that not even
> possible? The RFC isn't very clear but it appears that there is no way to
> redefine the entire property, as if you define it in the subclass with only
> get, then it will take set, isset, and unset from the parent class, correct?
> Though, that can be outright stopped by making the property final. But then
> there is no point in having read/write-only.
>
I tried to be clear about this at the top of the section on read-only and
write-only keywords... There are three possible states:
/* read-only by virtue of the fact that there is no setter, any sub-class may
define a setter and/or redefine the getter */
public $Hours {
get { ... }
}
/* final, no sub-class may redefine any aspect of this accessor */
public final $Hours {
get { ... }
}
/* Explicitly read-only, sub-classes may redefine the getter but may not define
a setter */
public read-only $Hours {
get { ... }
}
As I mentioned earlier, I'm not a fan of the concept of final, read-only or
write-only, they were only included because the original RFC indicated that was
what the community had decided on.
> From what I can tell, read-only becomes useful if I extend the class and want
> to partially modify the property.
> Example:
> class A {
> public $seconds = 3600;
>
> public $hours {
> get() { return $this->seconds / 3600 };
> }
> }
>
> class B extends A {
> public $hours { // Maintains 'get' from class A
> set($value) { $this->seconds = $value; }
> }
> }
>
> ^There's no way to stop the developer from doing that without read-only.
That is correct
>
> Also, if the property is public, what if an outside class tries to do this:
> class A {
> public $seconds = 3600;
>
> public $hours {
> get() { return $this->seconds / 3600 };
> }
> }
>
> $object = new A();
> $object->hours = 100;
> What happens then?
A fatal error occurs as there is no setter for hours. It is a read only
property by virtue of the fact that it has no setter. A sub-class could define
a setter for $hours if it so desired (due to the lack of a final or read-only
keyword in the declaration).
>
> And similarly, how do we set a public property as a property accessor, hmm?
> class A {
> public $hours = 1;
> }
>
> $seconds = 20;
>
> $object = new A();
> $object->hours = { // Does this work?
> get() { return $seconds; }
> };
I'm not quite sure what you mean here, are you asking if you can dynamically
define a property? The answer is no, though I think that would be cool it is
likely well beyond my ability to accomplish being that this is my first
contribution to the php project.
________________________________
David:
> class MyClass{
>
> public $property = 5 //?can we do this?
> {
> set($value){
> if(!is_int($value)){
> throw new InvalidArgumentException('value of wrong type');
> }
> $this->__property = $value;
> }
> }
> }
>
> Or would that have to happen in the constructor?
>
> class MyClass{
>
> public $property
> {
> set($value){
> if(!is_int($value)){
> throw new InvalidArgumentException('value of wrong type');
> }
> $this->__property = $value;
> }
> }
>
> public function __construct(){
> $this->property = 5;
> }
> }
I think there is some wide-spread confusion over what an accessor is. An
accessor does not store its own value, there is no "memory space" allocated for
an accessor unless you use the automatic implementation ( public $h { get; set;
} ) but if you're going to do that, you may as well just reduce it to ( private
$__h ) which is all the previous "accessor" would have done for you.
Accessors, in my own use, are nearly always used for convenience aliases with
conversions ( $Hours calculated based on a real stored $Seconds value) or as an
alias to access a value from another class, they are generally not used to
simply store or retrieve values, that's what a property is for, an accessor is
for executing code with the syntactic sugar of making it seem like it's a
variable.
________________________________
In closing for all of this, I really wish that this type of discourse could
have happened long ago when I was requesting it, because many, many hours have
been put into the current incarnation. I would like to put together a "vote
grid" or something on all that has been discussed here and come to a consensus
before I go and re-do any further work. Does the php community have any voting
facilities or is it still done via the wiki pages? How should I decide who
gets a vote and who doesn't? Should I even be handling the voting myself?
