Comments on the Property Accessors RFC.
Notes on bringing this RCF up to date with changes in PHP 7, plus a number of changes to simplify this feature and bring accessors into better alignment with regular properties.
The following is not a new RFC, but propsed changes and comments on the previous, rejected RFC - it assumes you've read the original RFC.
The conventions set forth in the examples are too C#-esque: capitalized property-names are extremely un-orthodox and foreign to PHP. We should demonstrate using conventions that more closely resemble de-facto standard PHP code. (Proposed changes to behavior of auto-backing properties also help make the previous convention obsolete, below.)
The get/set syntax is outdated with the introduction of return-types in PHP7. Proposed new syntax:
class TimePeriod {
private $_seconds;
public int $hours {
get { return $this->_seconds / 3600; }
set { $this->_seconds = $value * 3600; }
}
}This example demonstrates two significant changes:
-
The type-hint has moved into the property declaration to avoid duplicating it. PHP7 supports type-hints for return-types as well as argument type-hints. (the getter return-type and setter argument type are the same type, thus no point in repeating them.)
-
The setter implicitly provides the
$valueargument. (since setters always take precisely one argument, there is no point in declaring this - also, see #1.)
This syntax is consistent with this proposal for property type-hints, which should be considered, either in advance or together with accessors, because type-hinted properties would be consistent with type-hinted accessors. Also, consistently with the mentioned proposal, the type-hint is optional. (More justifications for the property type-hints proposal will be listed in the following.)
Accessor methods in the current RFC permit inconsistent visibility of accessor pairs, such as:
class Number {
private $_value;
public int $value {
get { return $this->_value; }
protected set { $this->_value = $value; }
}
}This is problematic, because it is inconsistent with regular properties, which, to the public, have one visibility modifier, rather than individual ones for reading and writing.
Visibility modifiers therefore should not be allowed for individual accessors of the same property, and the above example can be refactored as follows:
class Number {
private $_value;
public int $value {
get { return $this->_value; }
}
protected setValue(int $value) {
$this->_value = $value;
}
}This is simpler and easier to understand - the property isn't sometimes read-only (to the public) and at other times read/write (internally in the class) but has consistent visibility regardless of where it's accessed from, consistent with how regular properties work.
Also, if setting the value is an internal concern of the class, implementing that functionality as an accessor is pretty uninteresting, as it doesn't affect the consumers of that class, at all - to the public, it's a read-only accessor, and to the class itself, it doesn't much matter what it is.
For that matter, in the case of validations, the protected mutator-method can probably be left out, and then class can safely set the internal backing field - validation is often only required when a public consumer interacts with a model object.
Finally, removal of accessors
creates a situation where both public and internal visiblity of get/set-accessor methods are affected,
potentially inconsistently. (for example, an overriden property with public get and protected set
accessors is inconsistently overriden by public $var which makes both accessors public.)
With regards to guarded properties, based on an example from the current RFC, consider the following:
class TimePeriod {
public $hours {
get { return $this->hours ?: "not specified"; }
set { $this->hours = $value; }
}
}The implication is that $hours is both an auto-implemented backing field and an accessor, at the same time - this is
confusing and inconsistent with how auto-backing fields work in other languages. It implies that accessing $this->hours
from within the class behaves inconsistently from accessing e.g. $time->hours from outside the class, which is confusing and
inconsistent with how regular properties work. This raises questions about, at least, initialization and serialization, and
probably others.
In C#, an auto-generated backing field is anonymous and inaccessible to code - it's value can only be reached by means of reflection. The reason is that, in C#, backing fields are inaccessible to the public - this differs from PHP, where regular properties (the closest analog to C# fields) may be accessible to the public; but since fields and properties are not the same thing at all in C#, whereas regular properties and guarded properties in PHP invariably will be the same underlying thing, we can't really reference C# for this behavior.
The point is that auto-implemented properties in C# are not a convenience feature - it's a necessary feature, because a
backing-field is necessary to store the value, and fields are something different from properties. The typical use-case in
C# is something along the lines of public $hours { get; set; }, which isn't a convenient way to avoid the need to
explicitly declare the backing field, but rather is a means of saying "we don't care how this gets backed right now, but
we may want to introduce an implementation in the future".
In the case of PHP, the same is not true, and this feature therefore has no particular use, and shouldn't exist.
More to the point, a declaration such as public $time { get; set; } is equivalent to public $time and therefore
has no use. As argued earlier, inconsistent visibility of accessors shouldn't exist, and doesn't justify the existence
of this feature either.
The remaining argument in favor of this feature, is auto-implementing either get or set while providing an
implementation of the other - this isn't possible in C#, where the auto-generated backing-field is inaccessible,
and would be a feature strictly for convenience, with no functional justification. I therefore propose to remove it.
Allowing inconsistent declarations like public $foo { get; final set; } is problematic, in the same sense that
allowing inconsistent visibility of accessor methods is problematic - if either one or the other has been declared
final, how do you deal with accessor removal? Also, this is inconsistent with regular properties, where the notion
of final doesn't even exist.
By disallowing inconsistent final declarations, final means the property itself (as well as it's accessors) is
final, and can't be overridden or removed.
The current RFC proposes a means of returning references from getters which in most real cases isn't going to be possible - usually, the return value from a getter will be a computed value, otherwise you would likely be using a regular public property. (this point would be further reinforced by the implementation of the aforementioned property type-hints RFC - it eliminates the trivial case, where accessors are used merely as a means of getting a type-check on write.)
This feature is unnecessary and likely inconsistent with the behavior of today's run-time magic accessors - for example:
class Foo {
public function __get($name) {
return array(1,2,3);
}
}
$foo = new Foo();
$foo->bar[4] = 4; // E_NOTICEThe latter line leads to an E_NOTICE with the message Indirect modification of overloaded property, which is essentially
the case and issue with accessors.
A more natural behavior would be to invoke both the getter and setter - for example:
class Foo {
private $_bar;
public array $bar {
get { return $this->_bar; }
set { $this->_bar = $value; }
}
}
$foo = new Foo();
$foo->bar[4] = 4; // first get, then setThis is far more useful - a common reason to use accessors, is to validate the input value; in this example, you could for example validate the number of elements in the array after modification. By returning a reference, the whole point of guarding the property in the first place, is lost - the property value is now completely open to modification from the outside. (which, if that's what you wanted, you could still easily do, by adding a method that returns a reference, to the property, explicitly, and independently of the accessor.)
In short, that's not what accessors are for, and this feature shouldn't exist.
Pre/post increment/decrement, negation and string concatenation operators, consistent with array modification as outlined above, should cause both getter and setter to invoke - this is as per the original proposal, but is now consistent with array setting and appending values to arrays.
In terms of reflection, accessors are not properties - they are methods invoked with property-syntax. Backing fields are properties, but as proposed here, these are declared independently of accessors.
Reporting accessors as properties would cause problems with, for example, existing custom serializers, data mappers and dependency injection libraries, which would enumerate accessors and treat them as regular properties. For example, a serialization library would discover both the regular backing field properties as well as their accessors, which would cause them to serialize first the raw value and then the value returned by the getter, often leading to serialization of the same value twice.
Instead, ReflectionClass should have an added getAccessors() method, in which accessors are reported explicitly
and independently of regular properties. A new class, ReflectionAccessor would report these members using a structure
similar to that proposed as changes to
ReflectionProperty in the original RFC.
For completeness, ReflectionClass should support getAccessor($name), consistent with e.g. getMethod() and getProperty().
Note that reflection of accessor methods as methods (via ReflectionClass::getMethods()) was also considered, and likely is
not a good idea - while, arguably, accessors are methods, they are not methods in the traditional sense, just as accessor-based
properties are not properties in the traditional sense.
The proposed internal $prop->get accessor-name is misleading, where -> seems to indicate property or method resolution,
which is not what this is. Instead, I propose get $prop, which would result in more legible error messages, such as
Fatal error: Call to protected accessor Test::get $foo.
TODO: Should we support doc-block reflection on get/set/isset/unset declarations? This is unaddressed in the current RFC.
The above comments use the terminology set forth in the original RFC.
In a new RFC, the terminology would change from "guarded properties" to "accessors", since, with the described changes, accessors are no longer "guarding" regular properties - rather, accessors are dynamically computed properties, which do not physically exist, but of course may be backed by one or more physical properties.
The current terminology refers to the actual get, set, isset and unset methods as "accessors" - these would be
referred to as "accessor methods".
The proposed changes to reflection are consistent with the new terminology.
The current RFC does not directly address the issue of serialization - for example, it doesn't specify whether accessor
methods of guarded properties will be invoked by unserialize().
With accessors (as proposed here) being an entirely separate concept from properties, issue with serialize() and
unserialize() are a given - because accessors are strictly methods and (as explained in notes on reflection above)
are not methods "guarding" properties, this doesn't affect serialization, at all, whether via serialize() or the
JsonSerializable interface, or custom serialization of properties via reflection.