Skip to content

Commit

Permalink
Fixed return by reference to allow modification
Browse files Browse the repository at this point in the history
  • Loading branch information
@rndstr
rndstr committed Dec 6, 2010
1 parent 826da36 commit 91512cc
Showing 1 changed file with 2 additions and 2 deletions.
4 changes: 2 additions & 2 deletions src/PHPCR/NodeType/NodeTypeTemplateInterface.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public function setQueryable($queryable);
* @return array A mutable List of PropertyDefinitionTemplate objects.
* @api
*/
public function getPropertyDefinitionTemplates();
public function &getPropertyDefinitionTemplates();

/**
* Returns a mutable List of NodeDefinitionTemplate objects.
Expand All @@ -136,6 +136,6 @@ public function getPropertyDefinitionTemplates();
* @return array A mutable List of NodeDefinitionTemplate objects.
* @api
*/
public function getNodeDefinitionTemplates();
public function &getNodeDefinitionTemplates();

}

7 comments on commit 91512cc

@schmittjoh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I have no idea if this is an external API, but I'd use objects instead of arrays if you want to allow modifications of the returned value.

Using arrays is error prone, since it's not enough to specify the method to return a reference, but you'll have to assign the return value by reference as well. I think this is something you might easily overlook. But again, I'm not familiar with the PHPCR yet, so I don't know if any end-user is going to use this method :)

// does not work
$value = $obj->getNodeDefinitionTemplates();

// works
$value =& $obj->getNodeDefinitionTemplates();

@Seldaek
Copy link
Contributor

@Seldaek Seldaek commented on 91512cc Dec 6, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, it's kinda dangerous, but not sure either how useful this thing is at this point.

@dbu
Copy link
Member

@dbu dbu commented on 91512cc Dec 6, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could add this warning into the doc comment so people who read the comment get the hint. having a list object or something sounds like too much weight to me.

@rndstr
Copy link
Contributor Author

@rndstr rndstr commented on 91512cc Dec 6, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree. I suggest using ArrayObject internally.

[edit] On second thought: It's a language feature, why not use it? It gives you the possibility to decide whether you want to receive a reference or not. Although I don't know whether the requirement of the ampersand has changed in recent PHP versions or not (i.e., leading to confusion). But there is not much to refactor for using an object.. and can easily be done at a later point if required.

@Seldaek
Copy link
Contributor

@Seldaek Seldaek commented on 91512cc Dec 7, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is if you start using an object then you should not return a reference anymore. That's PHP4 style and it sucks. But arrays are not objects and need to be referenced explicitly otherwise they're just copied around. If the API says it has to return a mutable list, I think that should be an object. And ArrayObject seems like a good fit.

@rndstr: The reference feature in PHP just confuses most people because it's barely ever used so I don't think it's a good idea to make use of that if we can avoid it easily.

@dbu
Copy link
Member

@dbu dbu commented on 91512cc Dec 7, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after reading up the ArrayObject thing: we could define the interface saying it returns something implementing Traversable , ArrayAccess and Countable. then jackalope can just use ArrayObject, but if at some point we realize for performance or whatever we want something else, its always possible. rndstr, can you change it that way? and write some tests that check if it actually works :-)

@rndstr
Copy link
Contributor Author

@rndstr rndstr commented on 91512cc Dec 7, 2010

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I already wrote the tests, refactoring to use ArrayObject now :)

Please sign in to comment.