zircote / Hal

A PHP implementation of HAL http://stateless.co/hal_specification.html
93 stars 6 forks source link

Allow Hal\Resource::__construct to accept an instance of \Hal\Link in first argument #12

Closed noah-goodrich closed 10 years ago

noah-goodrich commented 11 years ago

This will allow users of the library to extend the Link class and manually inject the new class instance for the Resource's 'self' link.

hjr3 commented 11 years ago

I am fine with this change. We should also update the docblock to note that string or Link is acceptable.

baldurrensch commented 11 years ago

Also, if you could add a unit test, that would be great.

logistiker commented 11 years ago

While this change makes it possible to pass in your own link object, it is still an unacceptable change. You should not instantiate Link anywhere in Resource. If link is needed, then accept it using dependency injection within the construct or a method if it is needed and type check it to make sure it is of that type class. If you don't do this, then you can't write your unit test in isolation (which is the goal of unit testing). The Resource unit test should never instantiate Link. You should only be testing Resource. You should be mocking Link wherever it's needed within Resource unit tests.

hjr3 commented 11 years ago

@logistiker we are not breaking backwards compatibility.

hjr3 commented 11 years ago

@noah-goodrich sorry this slipped through the cracks. I do not think we noticed you added your unit test.

@baldurrensch any objections to merging this now?

logistiker commented 11 years ago

Here's what the construct for Resource should really look like (the defaults are already handled in Link):

public function __construct(Link $link, array $data = array())
{
    $this->setLink($link);
    $this->setData($data);
}
logistiker commented 11 years ago

A break in compatibility shouldn't be an issue if people utilized composer.lock properly.

noah-goodrich commented 11 years ago

@logistiker While your suggested interface is probably more correct, that WOULD break backward compatibility and therefore isn't really the intent of this pull request. My pull request simply allows you to pass in an instance of a Link so that you can extend the default Link class.

Changing the interface for the __construct() method is probably something you should create a new issue for so that it can discussed separately from this pull request.

hjr3 commented 10 years ago

I opened up https://github.com/zircote/Hal/pull/17 so we can get this merged in.

hjr3 commented 10 years ago

I am adding a SelfLink class to make sure the only link relation that can be passed in is a valid self link relation.

Convo with @baldurrensch and I:

hjr3: so what happens when i do new Resource(new Link($url, 'nope')); is that a fatal error? hjr3: should i actually make a class called SelfLink that forces the link relation to be self and then Resource only accepts a SelfLink as opposd to a normal Link? brensch: potentially. or the Resource wraps the Link in a SelfLink hjr3: that ignores the link relation specified? brensch and SelfLink basically overwrites the specified rel

I chose my idea, but it is clearly better :wink:

hjr3 commented 10 years ago

I merged #17