Composition

[evert]

Better @staabm ?

[staabm]

Goal should be to get rid of extends Sabre\Xml\Service and have the service as a property instead.

A lib user should at best not extend classes from a library class because it creates tight coupling

[evert]

I guess I personally just hate that type of thinking ;) I don't love working with hard rules like that, because I think it's always worth thinking about what's the best interface for a specific situation. I want to always try optimize for the 'golden path'.

In this particular case there's nothing to be gained from having an xml service as a property of the atom service, because the atom service actually doesn't add any new behavior. The implication of having it was a property means that users will have to call $atomService->service->method() for 100% of the time.

[staabm]

If you looks from a point a regular app which consumes sabre-xml would stand, the benefit is that the app-xml-service (in this case the atom-xml-service) will have a very small/focused interface which doesnt depend on a external class/package which is not in the users control.

So the apps xml generatiom/reading is completely indepedent from the lib which provides the underlying xml mechanics and therefore can be swapped out at will.

The point is not getting a better service class but a better overall design in which my app is not coupled to the libraries it depends on. Inherit from classes you not own is a anti pattern.

[staabm]

Code like

require __DIR__ . '/../vendor/autoload.php';
$input = file_get_contents('http://evertpot.com/atom.xml');
$service = new Sabre\Xml\Atom\Service();
print_r($service->parse($input));

Atm depends on sabre-xml Service class interface, which shouldnt be the case imo
(Instead of only depending on the atom-xmlservice interface)

Hopefully my point is more clear now

[evert]

Atm depends on sabre-xml Service class interface, which shouldnt be the case imo
(Instead of only depending on the atom-xmlservice interface)
Hopefully my point is more clear now

I guess I don't really see the problem. If I understand your suggestion correctly, you're proposing a change that would turn that code-sample into:

require __DIR__ . '/../vendor/autoload.php';
$input = file_get_contents('http://evertpot.com/atom.xml');
$service = new Sabre\Xml\Atom\Service();
print_r($service->service->parse($input)); // only this line changed

But imho the end-result is the same, except that it's slightly worse ;)

[staabm]

Nope, the end result will work with the same example code/api.

The atom service will provide the api and delegate to its private/protected property

[evert]

Then you are suggesting the Atom service implementing the same interface as the Xml interface, but effectively proxy all the calls.

I'd wonder: why? I understand that the API sabre/xml-atom exposes is dependent on how sabre/xml is implemented, but that api is guaranteed to be stable due to the version constraint. If sabre/xml breaks BC in the future then the option for sabre/xml-atom is to either provide a compatibility layer at that moment, or go along with the change and introduce the same BC change on it's own interface.

And aside from that, if you're the guy that doesn't just need a quick way to parse / generate atom files but you have a more sophisticated setup, then I think you should always typehint on Sabre\Xml\Service and never on Sabre\Xml\Atom\Service. Sabre\Xml\Service is the api, this class is just a small convenience wrapper.

[staabm]

Ok lets face it from another angle.

Lets assume sabre-xml-atom is a fully fledged app which contains several service classes.
The purpose of the atom-service class in the apps context is a way to read and write objects and transfer/persist them using xml. The atom-service class therefore will provide methods, named by use-cases (domain actions) etc, not a parse&write method we have today.

The service class will hide the mechanics used for reading/writing the xml in it (sabre-xml). The controller classes which work with the service classes will not know which "technology" is used to generate and read the xml etc.

Imo the sabre-xml-atom package represents a tiny example application and not a lib which provides additional generl purpose classes meant for consumption as part of a greater application. Therefore it should represent best practices on howto integrate sabre-xml in a application.

Since we use inheritance atm the api of the atom-service class provides more methods as required by a atom app (e.g. GetReader(), getWriter() etc) and also exposes state of the service (the public *Map properties).
Because of this it is possible that e.g. logic in Controllers might fiddle with the state of the service or call methods which are meant for lower level consumption etc.