Add general purpose Attributes, replace Layoutable

[clue]

• Implementation
  • Add general purpose Attributes
  • Replace Layoutable
• 100% test coverage
• Documentation

Fixes #100
Supersedes #40
Fixes #84
Unblocks #97

[clemens-tolboom]

Why is this by ref. This will be confusing. Say

$ar['color'] = 'red';
$a_attr = new AttributeBagReference($a);
$ar['color'] = 'blue';
$b_attr = new AttributeBagReference($a);

if ($a_attr->getAttribute('color') == $b_attr->getAttribute('color'))
{
    echo "Weird";
}

Or did I miss something?

[clue]

The AttributeBagRef is designed to be a lightweight temporary instance so that we can expose the attributes through the AttributeBag interface without actually storing the instance. I.e. every time you call $vertex->getAttributeBag() you get another instance (which is just an implementation detail), but you can still access all properties nonetheless.

The AttributeBagContainer has it's own internal set of attributes and could be used as an alternative to the AttributeBagRef. Using it instead would require every Vertex to have (or at least lazy-load) a dedicated instance and then implement it like this:

class Vertex
{
    public function getAttribute($name)
    {
        return $this->getAttributeBag()->getAttribute($name);
    }
}

I suspected this to be inefficient (additional instances, more references, forwarded method calls etc.), but we should probably run some benchmarks first.

[clemens-tolboom]

The code needs some more documentation but nice PR. I have no time to test this right now but will within a month.

[clue]

I'm not yet decided how useful a getNames() method actually is. It causes a bit of code duplication and it's the only method without "attribute" in its name.

Personally, I'd tend towards dropping it completely, but I'd love to hear some other opinions.

[nubs]

I agree. array_keys($bag->getAttributes()) seems useful enough to me?

[clue]

Absolutely, I guess it's safe to just drop this altogether.

[clue]

The code needs some more documentation but nice PR

Thanks for your feedback :) Yeah, I'm looking towards adding more documentation, but I felt it's already in a state to receive some feedback. Any input is welcome!

[nubs]

adding a $default = null parameter to this method could be useful. That would allow the user to override the value returned if the attribute doesn't exist. Thoughts?

[clue]

Makes perfect sense, thanks for the suggestion!

[nubs]

I'm definitely interested in seeing this feature added - is there anything that could be done to get this moving any faster (e.g., contributing documentation)? 👍

[clue]

is there anything that could be done to get this moving any faster […] ?

Thanks @nubs, your feedback is very much appreciated and certainly helps pushing this further! Contributing documentation is appreciated just as much. Feel free to your our IRC channel (#graphp on freenode.net) or directly file a PR 👍

[clue]

Besides the obvious lack of documentation, there's two more things I'd like to address before this can be merged:

• Naming: I'm a bit unhappy with the method name postfixes:

  • getAttribute($name) and setAttribute($name, $value)
  • getAttributes() and setAttributes($values)
  • getAttributeBag()

  I'm considering to rename the former, but in that case we should probably re-consider our naming scheme. I'm not really decided yet, but it could look something like this:

  • getAttributeValue($name) and setAttributeValue($name, $value)
  • getAttributesArray() and setAttributesArray($values)
  • getAttributeBag()

• Unsetting attributes and Null-Attributes: There's currently no way to unset attributes and/or differentiate a non-existing key from a null value.

Any thoughts?

[nubs]

I don't particularly like getAttributesArray - the word array bothers me as it isn't very clear (associative vs non-associative) and because it fixes the implementation to an array which may not be desirable (although I can't think of anything specific for this case).

Why not implement ArrayAccess and optionally Iterator? That would help solve the naming issues, unsetting attributes, and the handling of null vs not set (via offsetExists).

I'm not sure how that would play out with namespaced attributes, so some more thought needs to be given there. Something like guzzles collections and particular set_path would be nice (I wish guzzle's collection classes/functions were just split out as a separate package from guzzle proper). If we had that, it would handle getting/setting data nested at arbitrary levels in a structure, effectively replacing namespaced keys with proper data structure without sacrificing ease of access from the user's standpoint.

[nubs]

The set methods in this class don't return $this for the fluent interface like the other implementations of AttributeBag do. Is that on purpose? Should the fluent interface be part of the AttributeAware and AttributeBag interfaces?

[clemens-tolboom]

I don't particularly like getAttributesArray

I agree on the postfix Array

(just 2cents :-/)

[clue]

I'm a bit unhappy with the method name […]

This hasn't really changed ever since this has been raised. I agree on your comments that the suggested names aren't really better either. So I'm all for leaving this as-is for the moment and change the API once we come up with a better name.

Why not implement ArrayAccess and optionally Iterator?

This makes perfect sense on the AttributeBag. Given our above discussion I'd rather postpone this to a later release though.