| SitePoint Sponsor |
IMHO, if you really want get()/set() they should be included in an accessor interface (ie: rather than directly in the magic one), perhaps like:Originally Posted by arborint
PHP Code:interface PropertyAccessors extends Properties {
public function get($name);
public function set($name, $value);
public function propertytIsset($name); // ? name
public function propertyUnset($name); // ? name
}
I concur. As opposed to IsSet and UnSet, I'd like has($name[, $name[, ...]]) and remove($name), but perhaps I'm just odd. In any case, you'll have to shorten those two by a fair bit.
not to randomly make comments, but $name is pretty bland, generic, $property is more specific. and $propertyName is even more specific, because that is what is being passed in the signature ((string) $propertyName, (mixed) $value). but $property makes alot of sense because of the way it might be used within the method, by returning the actual property of the class.
i know this sounds picky, and i don't mean to sound rude, but i mean to encourage code that is more readable, because if you start now with the basic things in being generic, those practices will be used elsewhere, where they might not be as readable or easy to follow. someone could easily change it to where they are using $name to call a specific accessor method of a class or just even a method, unless you want specifically want to leave it generic so that someone could do the following.
to me (and i don't know about anyone else) what is shown below is more readable to what is going on and the general idea of what the interface intends to happen.PHP Code:// implements Properties
public function __get($name) {
// hacked way of getting accessor
$name = 'get_'.$name;
return ($this->$name());
}
public function isFoo() { return false; }
public function __get($name) { return $this->$name();}
so being more specific allows the code to be self documenting, which will make less work later for documenting the api and it gives a better idea of what the class and the way it was intended to be used.PHP Code:public function __get($property) { return ($this->$property); }
the php manual is a great thing, but it too could use some improvements. obviously if the status quo was good enough, so many people would not be interested in this thread.
Gah - I go to sleep, and this is what I get...28 new posts.
Yea I know -- that's what I was referring to. Most of the time I use this sort of behavior I usally just use the heirarchy to seperate things -- not for joint retrieval later, but usually just to great a logical scheme for me, the developer.You may be able to emulate uniqueness of key, but you cannot emulate depth and hierarchy that way. What if I want to get back an array of all my levels?
@get/set/__get/__set discussion:
Does all this seem a bit nit-picky, with everyone interjecting their particular opinions regarding a naming scheme? Let's pick the clearest interface name (Property?) and add basic/commonly used property methods (set/get, facaded by __set/__get). Thoughts?
Regards,
Tyler Tompkins
No - please make random comments... Sometimes they bring something up that can help us hash ideas out
To address the specific concern, I understand what you mean, but I would argue that $property is implied, thus $name == $propertyName. I personally favor the $key use as it makes sense in the case of arrays (array == $key => $value is probably a maxim somewhere), but I adopted (arbitrarily and am open to change) $name to keep continuity with the manual.
I agree - but at that point we're talking implementation specifics. If you want to implement Properties and create your own custom accessor methods, then you're more than welcome to. The only thing to keep in mind is that you're attempting to make something that can be interchanged with other parts. If you move away from relying on __get('value') or it's object->value usage, you've stepped away from the standard and moved to relying on the implementation of the object.i know this sounds picky, and i don't mean to sound rude, but i mean to encourage code that is more readable, because if you start now with the basic things in being generic, those practices will be used elsewhere, where they might not be as readable or easy to follow.
.. snip ..
so being more specific allows the code to be self documenting, which will make less work later for documenting the api and it gives a better idea of what the class and the way it was intended to be used.
And without any further talk... here's the last proposed code (available at revision 4 in the subversion repository:
PHP Code:<?php
/**
* Provides an interface for storing and retrieving any number of "properties"
* via PHP's {@link http://us2.php.net/manual/en/language.oop5.magic.php magic
* methods}.
*
* @license http://www.gnu.org/copyleft/lesser.html LGPL
* @author Collectively authored via
* {@link http://www.sitepoint.com/forums/showthread.php?t=298611}.
*/
interface Properties
{
/**
* Retrieves a mixed value from this container by its given <i>$name</i>.
*
* Should throw {@link PropertyNotFoundException} if the equivilent of
* <i>__isset($name) == FALSE</i> is found.
*
* @param string
* @return mixed
*/
public function __get($name);
/**
* Sets the property of a given <i>$name</i> to a given <i>$value</i>.
*
* @param string
* @param mixed
*/
public function __set($name, $value);
/**
* Returns <i>TRUE</i> if a given property is set, <i>FALSE</i> otherwise.
*
* @param string
* @return boolean
*/
public function __isset($name);
/**
* Unsets a property based on the provided <i>$name</i>.
*
* Should perform no action if {@link __isset($name)} returns false.
*
* @param string
*/
public function __unset($name);
}
if (!class_exists('RuntimeException')) {
// To be included until first stable release of PHP 5.1
class RuntimeException extends Exception { }
}
/**
* Should be thrown by {@link Properties::__get()} when an attempt to access an
* unknown property name is made.
*/
class PropertyNotFoundException extends RuntimeException { }
Nothing wrong with that. I can live with that, and without losing any sleep
Should __set be able to throw a PropertyReadOnlyException ?PHP Code:public function __set($name, $value);
Possibly... If it can, I'm in favor of creating a ReadOnlyProperties interface that is blank, but can be used to tag a Properties object as read-only so you have some way to divine that an exception could occur.
Of course, then we get into trying to guess how Properties might be applied. The next step is BlackHoleProperties where you can __set(), but not __get(). Calling __get() on something that doesn't exist is bound to happen and we need to know how to handle it - return null, thrown an exception, die, etc. - but being read-only should be left to the developer.
Along these lines - should __unset() throw an exception similar to __get()? Basically, all retrievals should return __isset() == true or a PropertyNotFoundException is thrown.
Personally I'm inclined to trigger the same errors that PHP would on access failures. If you do go with exceptions, then you may want to consider basing them on a more general exception, say PropertyAccessException. Asides from permission exceptions there are a host of other states that may be important to catch for some uses. For example, you may be allowed to set a property but the container's set operation may fail.
i use this quite frequently as well, but i'm thinking in reguards to a broader audience.I personally favor the $key use as it makes sense in the case of arrays
which is why i like the notion of putting something in like $property cause it sets it's intented use because i'll use the evil ms dot net for example there is a control class that impliments IComponent, IDisposable, IParserAccessor, IDataBindingsAccessor,If you want to implement Properties and create your own custom accessor methods, then you're more than welcome to. The only thing to keep in mind is that you're attempting to make something that can be interchanged with other parts. If you move away from relying on __get('value') or it's object->value usage, you've stepped away from the standard and moved to relying on the implementation of the object.
but most people never look to know this or find it out. so if we design a class implimenting interface properties, and then inherit from that class. Then average joe who is utilizing this with an ide, sits and sees that this class inherits a method __get($name) and then goes creates a hack here and there.
but enough on that, i'm tired, moody and i digress.
There are a couple of different question here: One is when are exceptions used and when are errors triggered.
Another is how to handle these rarer uses. Do we bother everyone with an exception when the spirit of PHP is perhaps looser than that, or do we say that in those cases implement things like isWritable($name) and isReadable($name) as standard extensions. If you can always do an "if ($data->isWritable($name)) {" then fine, but if the error by its nature needs to be handled farther away then an exception. I don't know if anyone is in PHP rushing down the "execption for everything" path.
Christopher
Hi...
No, we are at cross purposes. I thought you wanted read-only whereas in fact you want readable. I still don't find it a very useful interface though...
yours, Marcus
Marcus Baker
Testing: SimpleTest, Cgreen, Fakemail
Other: Phemto dependency injector
Books: PHP in Action, 97 things
I would like to make a couple of suggestion:
- These interfaces should ideally be implementable in php4, and should therefore support both the magic methods and get()/set(); additionally, I would vote for calling the other methods isset() and unset() for the sake of succinctness.
- Properties that are null should return isset() = false, as I believe this follows php's semantics for variables?
- Directly accessing properties is a bad idea (although I'm not sure if anyone suggested this) because we may want to lazy load objects.
- Possibly beyond the scope of this discussion, but... say this interface is being used as part of an ORM implemententation, and we want certain properties to return a collection of values; how would this be implemented? In my personal code, I use Collection objects that implement a simple iterator interface. How do you guys feel about this?
Keep up the good work, I feel a concensus coming on
Hi.
A major, really the major, decision seems to be slipping under the radar again. Whether properties are going to be accessed as either of...
Jeff, with an appeal to a PHP4 performance argument, has swung the weight behind the first version (which I'll call DB:: Data_Object style). This has serious implications. Really calling the look-up paramater $name ot $property is utterly insignificant compared with this.PHP Code:$value = $container->key;
$value = $container[$key];
$value = $container->get('key');
$value = $container->getKey()
I am not against this choice, but I cannot believe it hasn't been debated. Here are some shortcomings:
1) Reflection is knackered.
2) Mistakes will add variables: $container->unknown_key = 'something'.
3) It's difficult to decorate/proxy.
4) It makes use of __get() and there is only one of these. That's one less card for the developer to play later.
5) It can nameclash with existing variables in the object.
Also things are getting complicated again for no good reason. Who says there has to be an unset() method? That may be really hard to implement or incorrect, say for a database row. An implementer may choose to add it, but as you don't need it in every case, it shouldn't be in the interface.
See how abstractions bloat? Insidious things aren't they
yours, Marcus
Marcus Baker
Testing: SimpleTest, Cgreen, Fakemail
Other: Phemto dependency injector
Books: PHP in Action, 97 things
I'm also a proponent of the first style. I'll be terse in my comments so please don't think that I'm trying to pick apart your argument. I always respect your comments.
Reflection is always knackered unless you explicitly declare public properties, you declare public accessors or you provide custom reflection capabilites. In other words, there is no virtual (ie. dynamic) reflection in the engine. The internals folks don't seem to want to add reflection for virtual properties so I think the last choice is acceptable -- provide our own. OTOH, reflection is not the end-all-be-all. There are many implementations of virtual properties where Reflection is not a win (lazy loaded properties come to mind).
That would be either a poor implementation of your container or a container that was happy to have anonymous virtual properties. You can whitelist names (or patterns) and reject everything else.
I disagree. If your container implements intercepter stacks you have complete control of the behaviour and moreso, the process of implementing behaviour. For example, you can do beforeUpdate afterUpdate sort of stuff.
See 3 above. Previously I mentioned routing every magic method through __call which then calls a foreign (assigned) dispatch mechanism. It permits setting magic behaviour at runtime. You can have as much __get as you are willing to allow from your container.
My previous suggestion was: if you use virtual properties, you should not use public members. Protected and Private can be prefixed _.
I can't see how you can do a true property implementation without unset. Say you can't literally unset a backend -- your container might instead keep an internal list and mark the var as unset. The object state has to stay consistent and look consistent from the public API even if that means it has to do internal bookeeping. Perhaps a compromise is to provide some abstract classes with default behaviours, even if they are simply NOP.
All this perhaps suggests that there should possibly be an interface for directing the dynamic mechanisms. I think it also turns the discussion back to that of concrete vs dynamic, perhaps because some seem to care about PHP4 compatability. It could very well be that there is no happy common ground between the concrete and dynamic camps. The features for dynamic mechanisms aren't really capable enough until 5.1 and IMO, if you are going dynamic it is probably because you want to avoid using some of the heavier Javaesque idioms that concrete classes in PHP tend to elicit.
Best Regards.
Valid point. You could leave it out of the Interface, and have the feature as an abstract class method instead, in an abstract class instead.An implementer may choose to add it, but as you don't need it in every case, it shouldn't be in the interface.
That is one way to be sure it is implemented, whilst keeping the Interface slim.
Hi...
Please, please pick apart my argument. The choices at this stage have to be defended and advocated or we won't get very deeply into them. I actually agree with your points (mostly), but...
You can explicitely hand code the class to the interface, or you can code generate. Both of these would preserve reflection. However if you force people to use __get() then it's gone for good. You need reflection to further use a dynamic proxy (AOP, DI, Mocks), so there is a high price attached to this loss in sophisticated environments.
But if you are using this interface as a mixin, then the original behaviour may be contradicted.
True, but this raises the bar. Again, there is a price to pay.
Hack? Since when did the mere presence of an interface force you start doing things like this. This is not going to win hearts and minds.
To unset() implies removal of a property. You cannot do this with a database row. You can set a field to null, but that is not the same thing. As a database row is a very likely use case for this interface I would say you are stuck at this point. This is the one point where I genuinely disagree; this is a contradiction.
It comes down to what we mean by the interface. If you think of it as a blank slate to add and remove slots at will, then you will want unset(). If you view it as a fixed set of slots (DB row or config file) then using an unknown field will throw an exception and unset() is an anathema.
I think this highlights an important difference between something called Properties and something called Hash. I think an interface called Keyed (still my favourite) could apply to both.
That goes against the whole idea of an interface. You effectively renage on the contract and this will confuse the hell out of people. How can you trust any interface if all developers do is stub out the methods they don't want. Code that relies on these methods doing as expected will still fail.
It's an interface. It has nothing to say about whether the implementation is dynamic or static.
yours, Marcus
Marcus Baker
Testing: SimpleTest, Cgreen, Fakemail
Other: Phemto dependency injector
Books: PHP in Action, 97 things
Except PHP says you better be coding real methods in your class if it implements and interface, handling your interface through __call() is verboten
Jason Sweat ZCE - jsweat_php@yahoo.com
Book: PHP Patterns
Good Stuff: SimpleTest PHPUnit FireFox ADOdb YUI
Detestable (adjective): software that isn't testable.
Just thought I would throw in a concrete implementation of Properties. It's a modified version of the Request object from the skeleton thread.
Note 1: PHP 5.0 does not implement __isset or __unset so I had to use a method. And you cannot have a method called isset.PHP Code:<?php
require_once 'Properties.interface.php';
/* ------------------------------------------
* Taken from the Skelton framework
* Basically decides between $_GET and $_POST
* And cleans up slashes etc
*
*/
class ZRequest implements Properties
{
protected $props = array();
protected $post = FALSE;
function __construct()
{
/* Store any command line arguments */
if (isset( $_SERVER['argv'][1])) {
parse_str($_SERVER['argv'][1],$_GET);
}
/* Decide between post or get */
if (!strcasecmp($_SERVER['REQUEST_METHOD'], 'POST')) {
$this->props = $_POST;
$this->post = TRUE;
}
else $this->props = $_GET;
/* Nice to know the script */
$this->props['PHP_SELF'] = $_SERVER['PHP_SELF'];
/* Clean up silly quotes */
if (get_magic_quotes_gpc()) {
$this->removeSlashes($this->props);
}
}
function removeSlashes(&$str)
{
if (is_array($str)) {
foreach ($str as $key => $val) {
if (is_array($val)) $this->removeSlashes($val);
else $str[$key] = stripslashes($val);
}
}
else $str = stripslashes($str);
}
function __isset($name)
{
return isset($this->props[$name]);
}
function __unset($name)
{
throw new PropertyReadOnlyException("Request $name");
}
function __set($name,$value)
{
throw new PropertyReadOnlyException("Request $name");
}
function __get($name)
{
if (!isset($this->props[$name])) {
throw new PropertyNotFoundException("Request $name");
}
return $this->props[$name];
}
function is_set($name) { return isset($this->props[$name]); }
function get($name) { return $this->$name; }
function isPost() { return $this->post; }
}
?>
Note 2: I made the props read only on the theory that one should not update posted variables.
Note 3: I snuck in a get() method so I didn't have to change some of my other code. But $request->key works fine.
Note 4: Might try to specify the format of the Exception message strings.
Marcus, I agree there should be more debate. __get/__set does have consequences and gotchas.
For example, Here is another implementation to look at
PHP Code:<?php
interface Properties {
public function __get($property);
public function __set($property, $value);
public function __isset($property);
public function __unset($property);
}
class AccessorPropertySupport implements Properties {
public function __get($property) {
if (method_exists($this, 'get' . $property)) {
$method = 'get' . $property;
return $this->$method();
} else {
$internalProperty = '_' . $property;
return $this->$internalProperty;
}
}
public function __set($property, $value) {
if (method_exists($this, 'set' . $property)) {
$method = 'set' . $property;
$this->$method($value);
} else {
$internalProperty = '_' . $property;
$this->$internalProperty = $value;
}
}
public function __isset($property) {
if (method_exists($this, 'get' . $property)) {
return TRUE;
} else {
$internalProperty = '_' . $property;
return isset($this->$internalProperty);
}
}
public function __unset($property) {
throw new Exception('Cannot remove properties.');
}
}
class Rectangle extends AccessorPropertySupport {
protected $_width;
protected $_length;
function __construct($width, $length) {
$this->width = $width;
$this->length = $length;
}
function getArea() {
return $this->_width * $this->_length;
// We cannot use the same notation from within the class as
// we do externally to the class because our magic method
// does not get called internally.
// return $this->width * $this->length
// does not work here.
}
}
$rec =& new Rectangle(10, 20);
echo 'width:', $rec->width, "<BR>\n";
echo 'length:', $rec->length, "<BR>\n";
echo 'area:', $rec->area, "<BR>\n";
?>
Whoa... Hang on, didn't I say earlier that we needed to have a non-mutable version because the presence of set()/__set() implies that a property can be set...
Seriously though, I don't see a problem with them being there. If your loading a read-only Properties, throw an exception when a set or unset is attempted. Otherwise, there's two interfaces - the read-only - __get() & __isset() - and the mutable - add __set() & __unset(). Beyond that, it's up to the implementor to say when a given action is allowed. Anyhow, I think the bigger issue is introspection.
Should a Properties/Keyed implementation provide getKeys() which returns an array of key names that are available? In a dynamic setting, this would return basically an array_keys($this->_corral), while a read-only might return the meta-data from the db table that was used to create the object.
Why not just call __isset() directly? It's not as readable, but not much more so than is_set().
This seems to be the second use mentioned this evening (besides my own) for read-only. I keep coming back to that being the simplest form of this.
Good points, lastcraft. I'll try once again but it is certainly getting harder.
You have a good argument here. Perhaps we can overcome some of the difficulty with an interface for reflection (__reflect(), anyone). Yes, more overhead but the engine permits little wiggle room here. OTOH, no one is forced to use this interface for all things.
Its not the presence of the interface but rather the fact that I prefer uniform behaviour over all public properties, virtual or not. As you are well aware, PHP member vars don't get user magic. This is the main reason for my statement.
If unset is verboten for a specific implementation, then the implementation should trigger an error or throw exception, not? So wouldn't you want to implement that and why shouldn't it be part of the interface? After all, you can't prevent the user from calling unset($foo->property). Should we leave it to the engine to act on that? Or as we are implementing virtual properties should we not take it on ourselves to implement the proper behaviour? I may be forced to concede on this point if anyone has a need for using a private or protected __unset.
Wait a second, an interface is a contract to be able to call certain names with certain signatures. They caller won't fail per se as long as you return an expected type -- though they may do the wrong thing if the implementation doesn't carry through the appropriate operation. That's a general issue -- just because you agree to respond to the interface doesn't mean the code really does the right thing (ergo, testing). Yet I didn't say stub out and certainly not for any method; I said provide default behaviours, perhaps NOP. In your example of unsetting a db row, doing the right thing may indeed be to do nothing at all.
That said, I think I should have rather said that the default behaviour should be to trigger errors -- that's actually what I do in my classes for all default magic behaviour.
I would almost be willing to accept this but then I couldn't type hint on the interface. Further, I firmly believe that if we virtualize properties through magic that we have to hook all of the engine's interfaces to them. We can not unprovide through lack of mention what the engine magically exposes.
Best regards.
I disagree with this. I think we have shown quite a few different ways to approach DI without relying on reflection on the DI Thread. Most of the approaches are even PHP4 compatible. As far as AOP goes, it is precisely the magic in _get, _set and _call that finally makes possible in PHP the generation of dynamic proxies and true decorators.
PHP Code:class LoggerDecorator() {
function __construct($decorated){
$this->decorated = $decorated;
}
function set($name, $value){
Logger::log("setting $name to $value");
return $this->decorated->set($name, $value);
}
function _call($methodName, $parameters){
$this->decorated->$methodName($parameters);
}
}
Garcia
So which makes more sense?
Personally, if we can't use it internally, I say the magic methods should be pulled off the table as the primary and put them as sub-classes.PHP Code:return $this->__get('width') * $this->__get('length');
// or
return $this->get('width') * $this->get('length');
// or your internal storage
return $this->_width * $this->_length;
// or mine internal storage
return $this->_corral['width'] * $this->_corral['length'];
Thoughts? This does remove the following behavior:PHP Code:interface Keyed {
public function get($key);
public function has($key);
}
interface MagicKeyed extends Keyed {
public function __get($key); // maps to get($key) or vice-versa
public function __isset($key); // maps to has($key) or vice-versa
}
That's the only draw-back I see. Of course, if you rely on the first behavior, you rely on MagicKeyed.PHP Code:// will work
$keyed->someArray[1]->value->key;
// will not work without some fancy implementation
$keyed->get('someArray[1]')->get('value')->get('key');
@Selkirk: I don't like that __get can't seem to chain (you can call $rec->getArea() and get expected results even if getArea() uses the public property access) but why wouldn't an accessor method reference internals? After all, aren't accessors supposed to expose internal storage? For example, if I implemented getLength() in your example class, it would clearly be wrong for it to access $this->length -- that is what it is supposed to be providing.
Posting Permissions
Bookmarks