Feature/add problem interface

nyamsprod commented 3 years ago

@Crell This PR tentatively tries to introduce an Interface for the ApiProblem package.

I say tentatively because I wonder if this should be done using something la PHP-FIG or if this is useful to enable swapping or using other package using object following RFC7807 like https://github.com/phpro/api-problem .

Also I tried to extract the interface without introducing any BC break

If you find this useful the remaining/open question are:

Naming issues:

should the interface be simply called Problem vs ApiProblemInterface
should we used Problem::toArray vs Problem::asArray

New methods/classes

the set/getExtensions methods are added to enable access to the extension members easily
should we introduce a specific Exception if a setter method fails ?

Out of scope features

Currently the Problem interface does not support the JsonSerializable or the ArrayAccess interface as such any conversion feature like asXml or asJson are not present in the interface and are specific to the current implementation.
In contrary asArray is kept in the interface as the other representations can be infer from it. As a consequence, HttpConverter is rewritten internally to only work with the asArray method.
Using PHP native JsonException as this requires at least a PHP requirement bump and maybe a full BC break.

let me know what you think

Crell commented 3 years ago

Thanks, @nyamsprod. I'm not sure I agree with this, however. This is a concrete value object, so an interface isn't really necessary. I only know of two implementations; this one and the one built into Laminas. There isn't much value in trying to make a value object swappable when the implementation is mostly boilerplate.

Basically, I don't really see what value this adds.

nyamsprod commented 3 years ago

@Crell thanks for the feedback. Indeed having an interface for a value object feels strange but I find myself working on projects where I indeed have multiple implementation of the ApiProblem object 😢 but I fully understand your reasoning.

Having said that here's what I would propose as an alternative:

1) Add the ApiProblem::setExtensions and ApiProblem::getExtensions methods.

public function setExtensions(iterable $extensions): ApiProblem;
public function getExtensions(): array;

While the ArrayAccess interface provides a nice API around extensions members there's no current easy way to access/set all extension members at once.

2) Removing the following comment on the ApiProblem class.

Subclassing this class to provide defaults for different problem types for your application is encouraged.

If we are dealing with a value object we should promote composition over inheritance whenever possible no ?

For instance I usually use the package as shown below:

<?php

interface CastToProblem
{
    public function toProblem(): ApiProblem;
}

This way I ensure that I don't extend the ApiProblem or add more public API than needed.

3) Adding an exception to cover setting the status code outside the valid HTTP Status code range.

As this is expected to be an error as stated in the docblock of the method but not enforced in the code.

The following 2 would require BC Break

5) Making the class final.

4) Remove or move the package JsonException and use instead PHP Native JsonException exception.

If you find those alternative worth pursuing I could submit independent PR to cover them.

Those changes would IMHO remove any need for a dedicated interface.

Crell commented 3 years ago

I find myself working on projects where I indeed have multiple implementation of the ApiProblem object

Dear god why? :smile: And which other implementation? As I said, I only know of this one and Laminas. (I've not looked at Laminas' implementation in a long time.) Knowing what the compatibility issues are with whatever else you're working with would help determine what if anything can/should be done.

To your other suggestions:

1: I'm not sure how this one would help in your case, unless it's cloning methods from another implementation. I'm not against it, really, just unsure what the value is it adds without more context.

2 and 5:

If we are dealing with a value object we should promote composition over inheritance whenever possible no ?

I really wish people would stop making dogmatic statements on inheritance. :smile: It absolutely can be over-used, but it also absolutely has valid use cases, specifically where "is a special case of" really is the right logic you want to implement. In this case, a deliberate part of the design is the ability do to something like this:

class InsufficientCredit extends ApiProblem {
    public function __construct(int $balance, int $needed, array $accounts) {
        $this->title = "You do not have enough credit.";
        $this->type = "http://example.com/probs/out-of-credit";
        $this->detail = sprintf('Your current balance is %d, but the cost is %d', $balance, $needed);
        $this->extensions['balance'] = $balance;
        $this->extensions['accounts'] = $accounts;
        $this->status = 402; // Or 409, or whatever 412, or whatever code you want.
    }
}

return new InsufficientCredit(balance: 30, needed: 50, accounts: $acct_list);

And now you have an easy to use, domain-specific value object that will still compile to an ApiProblem JSON/XML message like any other. Composition is not at all helpful in this case. This is a deliberate and very good feature of this library, IMO, and I am not OK dropping it. (You should be doing the same with your exceptions in general, in fact.)

3: Validating the code sounds reasonable to me, depending on how tightly it's restricted. I'd accept a stand-alone PR for this one.

4: I think you skipped a number. :stuck_out_tongue:

6: Off the top of my head, I don't know why I used a custom exception here instead of forcing json_decode() to throw an exception. This is why comments are useful... Bad me. Perhaps because the native exception's debuggability is awful, as it lacks the built in error messages? I'm open to discussing revising this in its own issue. It probably would be at least a minor version bump if not major, but we can discuss that separately.

Crell commented 3 years ago

Hm, you closed this one but didn't open new issues? Are those still coming?

nyamsprod commented 3 years ago

@Crell yes I will create them in different PR. So they will be coming just need to free up some times to do them properly.

Crell / ApiProblem