On Fri, Oct 12, 2012 at 7:23 AM, Clint Priest <cpri...@zerocue.com> wrote: > Alright, here is the updated RFC as per discussions for the last few days: > > https://wiki.php.net/rfc/propertygetsetsyntax-as-implemented > > If you could read it over, make sure I have all of your concerns correctly > addressed and we can leave this open for the two week waiting period while I > make the final code changes. > > -Clint
So here comes my round of feedback on the current proposal. But before getting to that: I have collected a bit of data how getter and setter are currently used in Symfony and ZF: https://gist.github.com/3884203 Mainly so I get a better overview of the situation, but might be of interest for other people involved in this discussion too. So, my points on the proposal, in no particular order: 1. Typehints ----------------- The last revision of the proposal added the ability to typehint the set() accessor, so that's a good start. Still I would like to go a bit further on this point. My data says that about 20-30% of all setters are typehinted and it is highly probable that if we had scalar typehints (as I'm sure we will have in the future) that number would be even larger. In particular I would like to have an easy way to typehint if I *don't* want to override any additional behavior. Currently that would require me to write something along the lines of this: protected $_date; public $date { get() { return $this->_date; } set(DateTime $date) { $this->_date = $date; } } This is a rather lot of boilerplate code just to add a typehint. With automatic properties this might be reduced to this: public $date { get(); set(DateTime $date); } This is already a lot nicer, because I don't have to implement the same getting and setting pattern again and again, but I still find that it doesn't do a particularly good job at conveying its intention. Rather, what that code *really* wants to say is just this: public DateTime $date; I think that it would be very beneficial to support this syntax, because I think that a large part of the use of the accessor proposal will just be type checking. But if we indeed want to syntax we should reconsider the get {} set { $value; } syntax again, because as I understand it the new get() {} set($value) {} syntax was mainly introduced to deal with setter typehints. If we chose to allow typehints before the property name, then that wouldn't be necessary anymore. 2. Interfaces ------------------ I already mentioned this before, but I'll reiterate it here again: Currently properties and properties with accessors are handled different with regards to interfaces. In particular, if you have an interface such as follows: interface Extension { public $name { get; } // more stuff } then the following is not a valid implementation of the interface: class FooExtension implements Extension { public $name = 'foo'; // more stuff } This does not make sense to me: $foo->name is a gettable property, as such it satisfies the interface. I don't see a reason why we would need a hard distinction between properties and properties with accessors here. The converse is also true: An interface currently can not define a "plain" property: interface Foo { public $foo; } I would expect this interface definition to be the same as "public $foo { get; set; }". But I'm not sure on this point, as one could argue that it's just a redundant way to say the same thing. 3. Protected backing property ------------------------------------------ The RFC currently contains just a single code example, namely setting a $Seconds property through an $Hours property, which is an example where you want to override both accessors and there is a backing property that already exists "naturally". If you on the other hand you consider cases where you only want to override either the set or the get behavior and there doesn't already exist a backing property, you will end up with a code like this: protected $_isEnabled; public $isEnabled { get() { return $this->_isEnabled; } set($isEnabled) { $this->_isEnabled = (bool) $isEnabled; } } The above code really only adds a bool cast, but still it has to do two things: Add a protected backing property and define a getter which basically matches the default behavior. With automatic properties as they are currently implemented you could write: public $isEnabled { get(); set($isEnabled) { $this->__isEnabled = (bool) $isEnabled; } } This is closer to what I actually want to do, but it relies on using $this->__isEnabled, which is really just an implementation detail. Instead, I would rather want to have access to the "underlying" property: public $isEnabled { get(); set($isEnabled) { $this->isEnabled = (bool) $isEnabled; } } I know that the wording is a bit vague; another way to put it, is that the above inverts the current shadowing behavior: In the current implementation, if there exists a "plain" property and an "accessor" property with the same name, then the plain property shadows the accessor property. As you say, this is to help lazy-loading. I don't really buy the argument about lazy-loading, because I don't think that the small perf difference between going trough an accessor and accessing a plain property is really relevant in that context (especially as lazy-loading would have required a method otherwise). What I'd rather see is the reverse behavior: An accessor property shadows a plain property, so that the plain property is only accessible from within the accessor methods. This would allow you to write code like in the last example above and I think it would also make automatic properties more meaningful (you could store the state in a property with the same name; also nicely integrating with serialization and debugging). I know that you (Clint) want to create a hard distinction between plain properties and accessor properties, but I think that making it more smooth juts integrated better with PHP. I already noticed several people who, after reading the RFC, tried to write code where they access the property from within the accessor. It seems to me that people think it should work. The current behavior where you can shadow the accessor property by assigning a property of the same name seems rather obscure to me. This is the feedback I have for now. Looking forward to your thoughts on the manner. Nikita -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php
