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?
