[PSR-7] Handling invalid arguments in MessageInterface without typehint

[Márk Sági-Kazár]

Hi there,

I looked at the Psr MessageInterface and found that the handling of invalid data is a bit inconsistent (or there is no restriction).

The setBody function takes a typehinted stream parameter and should throw an InvalidArgumentException in case of invalid body?

What about the other functions?

I mean: the interface describes very precisley that the header setters accept a value which is a string, array of strings or can be casted to string. What about invalid data? Should be checked and thrown an InvalidArgumentException as well or just cast everything (value or values in array) to string and let PHP fail with an error?

I think Phil Sturgeon's DBAD logic can be interpreted here as well, so for me the second is acceptable, meaning it is the developers responsibility to provide valid data and any other cases is treated as an error.

What is the "standard" for this?

Best regards,

Márk Sági-Kazár

[Márk Sági-Kazár]

This is of course valid for the other interfaces. It is an important question since interop disappeares if one implementation throws an exception an other triggers an error.

[Matthew Weier O'Phinney]

On Oct 11, 2014 6:14 PM, "Márk Sági-Kazár" <mark.sa...@gmail.com> wrote:
> I looked at the Psr MessageInterface and found that the handling of invalid data is a bit inconsistent (or there is no restriction).
>
> The setBody function takes a typehinted stream parameter and should throw an InvalidArgumentException in case of invalid body?
>
> What about the other functions?
>
> I mean: the interface describes very precisley that the header setters accept a value which is a string, array of strings or can be casted to string. What about invalid data? Should be checked and thrown an InvalidArgumentException as well or just cast everything (value or values in array) to string and let PHP fail with an error?

Typically, you will throw an exception in cases of invalid input. In writing/editing the interfaces, I did not explicitly indicate this for two reasons:

- once you start adding @throws annotations, somebody will come along and start disputing which exception type you've specified (because everybody has an opinion on these), and/or insist on adding exception interfaces to the proposal (which will cause others to start disputing naming and/or necessity of them).
- how errors are detected and handled is largely an implementation detail, and should be left to the libraries implementing the interfaces.

Regarding this last point, my assertion is that most implementations will follow the spec closely, and handle unexpected input with exceptions, and that there will be few enough differences that interop will not be hampered. What I've seen so far of the other proposals that provide interfaces also suggests that we'll typically see between 1 and 5 widely used implementations, and these will primarily differ only in terms of extensibility; basic functionality will be largely identical, particularly when it comes to the consumer API defined by the shared interfaces.

> I think Phil Sturgeon's DBAD logic can be interpreted here as well, so for me the second is acceptable, meaning it is the developers responsibility to provide valid data and any other cases is treated as an error.

Precisely - developers should provide input the interfaces would expect, and expect an error condition should they fail to do so.

[Márk Sági-Kazár]

I think this is unclear at the moment, since the interface mentions throwing an exception at some points (see the first post).

A common case (I usually can't agree with myself either) is when an object not implementing the __toString function is being casted to integer. (which is the case in MessageInterface headers)

Then what? Let it fail with an error? Type check all headers and throw an exception? (Does that worth the resources at all?)

These are the questions coming into my mind and I have a weird feeling about not beeing answered by the standard.

I think this worth mentioning somewhere.

[Michael Dowling]

The example given is simply that I forgot to remove the "@throws" annotation when I added a type hint to the interface. It can safely be removed.

A couple points should be made:

1. Passing invalid arguments to method calls is not a recoverable error.

2. PHP has SPL exceptions that are already "standard" exceptions which means we will almost never need PSR specific exceptions.

Thanks,

Michael

--
You received this message because you are subscribed to the Google Groups "PHP Framework Interoperability Group" group.
To unsubscribe from this group and stop receiving emails from it, send an email to php-fig+u...@googlegroups.com.
To post to this group, send email to php...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/php-fig/47db3135-9a13-40ed-95e5-01a85b172d87%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Márk Sági-Kazár]

That's it. But: shouldn't it be applied in general? It appeared in connection with the HTTP Message Psr, but I think it might be applied for any other similar cases.

[Matthew Weier O'Phinney]

On Sun, Oct 12, 2014 at 5:39 PM, Márk Sági-Kazár
<mark.sa...@gmail.com> wrote:
> I think this is unclear at the moment, since the interface mentions throwing
> an exception at some points (see the first post).

As Mike indicated, this was done for the MessageInterface, but not for
other interfaces, and he intended to remove it. Based on your
feedback, and feedback from others, however, I've created a new PR for
the proposal that adds in @throws annotations for all setters, as well
as methods that accept ambiguous arguments.

- https://github.com/php-fig/fig-standards/pull/345

> A common case (I usually can't agree with myself either) is when an object
> not implementing the __toString function is being casted to integer. (which
> is the case in MessageInterface headers)

This does not work anyways. PHP has no way to cast objects to integers
built in, and only uses __toString() if you attempt to cast to string.
(You will ALWAYS get a PHP notice indicating the object cannot be cast
to int, and casting will always result in the integer 1.)

> Then what? Let it fail with an error? Type check all headers and throw an
> exception? (Does that worth the resources at all?)

See above -- casting to integer will not work properly regardless, so
there's nothing we can or should do to catch and/or report this.

A couple notes:

- Anything implementing StreamableInterface MUST implement
__toString(), as we defined it in the interface itself. How that works
is up to the implementor.
- Implementors of StreamableInterface::__toString() are bound to PHP's
runtime behavior, and MUST NOT raise an exception in __toString().
(See http://php.net/manual/en/language.oop5.magic.php#object.tostring
-- "You cannot throw an exception from within a __toString() method.
Doing so will result in a fatal error.")
- Any other casting besides to a string also must follow PHP's runtime behavior.

--
Matthew Weier O'Phinney
mweiero...@gmail.com
https://mwop.net/

[Márk Sági-Kazár]

I'm a bit confused now: according to Mike those @throws annotations should be removed. Also invalid parameters should end up in a non recoverable error.

> A common case (I usually can't agree with myself either) is when an object 
> not implementing the __toString function is being casted to integer. (which 
> is the case in MessageInterface headers) 

Sorry, I wanted to write string and not integer. However this refers to a passed object argument to the MessageInterface not implementing the __toString method (so not speaking of StreamableInterface).

So the question of implementation: should the setter check if the parameter is an argument and if yes check for a __toString method and throw exception when needed (same for array of objects) or just try to cast it to STRING and let PHP fail in case of error?

I thought (according to Mike) that the answer is the latter, so do not mess with argument validation.

It is also unclear that in case of adding an object to the headers should it be casted to string when added or only when the getter is called? At first it seems it does not matter.

The getHeader and getHeaderAsArray methods return with a string or array of strings, however it is uncler whether the getHeaders should return the originally passed values or strings. This might be a minority, but this whole string casting thing would be better if all the cases were handled.

[Matthew Weier O'Phinney]

On Mon, Oct 13, 2014 at 9:32 AM, Márk Sági-Kazár
<mark.sa...@gmail.com> wrote:
> I'm a bit confused now: according to Mike those @throws annotations should
> be removed.

Based on YOUR feedback and others, I decided they should be explicitly stated.

> Also invalid parameters should end up in a non recoverable
> error.

That's what an exception is. It's non-recoverable from the point of
view that execution cannot continue from the point at which it was
thrown UNLESS it's wrapped in a try/catch block that allows recovery.
If we document that the method @throws, then the expectation is that
implementors will throw, but not catch. Since any catch logic would
then be a step or more above the implementation, the method logic will
not continue after the exception is thrown.

>> A common case (I usually can't agree with myself either) is when an object
>> not implementing the __toString function is being casted to integer.
>> (which
>> is the case in MessageInterface headers)
>
> Sorry, I wanted to write string and not integer. However this refers to a
> passed object argument to the MessageInterface not implementing the
> __toString method (so not speaking of StreamableInterface).
>
> So the question of implementation: should the setter check if the parameter
> is an argument and if yes check for a __toString method and throw exception
> when needed (same for array of objects) or just try to cast it to STRING and
> let PHP fail in case of error?

Okay, to clarify for others (as I had to dig into the interface myself
to understand where you were referring to), this has to do with cases
like headers, where we allow the values to be objects so long as they
can be cast to a string.

I implemented this in phly/http over the weekend (not yet pushed,
however), and have it using method_exists($value, '__toString') to
check if the object may be cast to a string; if it cannot, I raise an
exception.

> I thought (according to Mike) that the answer is the latter, so do not mess
> with argument validation.

That's also a valid approach. PHP will raise a catchable fatal error
in such a case. These will cause the engine to halt if not handled --
but if they are handled, then it's a similar situation to raising an
exception; unless the method that raises the error also handles it,
application flow returns to the caller.

> It is also unclear that in case of adding an object to the headers should it
> be casted to string when added or only when the getter is called? At first
> it seems it does not matter.

That's an implementation detail. You can either do it immediately, or
only on retrieval. In phly/http I did it immediately, as it simplifies
the internal logic; a case could be made for deferring until
retrieval, however, as it would allow manipulation of the header after
adding it to the response or request. This would need to be documented
by the implementor, so that consumers know what to expect.

> The getHeader and getHeaderAsArray methods return with a string or array of
> strings, however it is uncler whether the getHeaders should return the
> originally passed values or strings.

Strings -- because the implication is that retrieval is when you want
to emit the header values.

> This might be a minority, but this
> whole string casting thing would be better if all the cases were handled.

I've added a PR to make what getHeaders() should return explicit:

- https://github.com/php-fig/fig-standards/pull/347

> --
> You received this message because you are subscribed to the Google Groups
> "PHP Framework Interoperability Group" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to php-fig+u...@googlegroups.com.
> To post to this group, send email to php...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/php-fig/33dd91b5-93f2-44c2-a1df-bd3c100ae53d%40googlegroups.com.

>
> For more options, visit https://groups.google.com/d/optout.

[Márk Sági-Kazár]

Thank you for your answer, I appreciate it.

[Larry Garfield]

On 10/13/2014 10:24 AM, Matthew Weier O'Phinney wrote:
> On Mon, Oct 13, 2014 at 9:32 AM, Márk Sági-Kazár
> <mark.sa...@gmail.com> wrote:
>> I'm a bit confused now: according to Mike those @throws annotations should
>> be removed.
> Based on YOUR feedback and others, I decided they should be explicitly stated.

FWIW, I agree with explicitly specifying the error conditions. As we've
seen with HTML itself, specifying what to do when everything is broken
is just as important if not moreso than specifying what to do when all
is right with the world. :-)

>> It is also unclear that in case of adding an object to the headers should it
>> be casted to string when added or only when the getter is called? At first
>> it seems it does not matter.
> That's an implementation detail. You can either do it immediately, or
> only on retrieval. In phly/http I did it immediately, as it simplifies
> the internal logic; a case could be made for deferring until
> retrieval, however, as it would allow manipulation of the header after
> adding it to the response or request. This would need to be documented
> by the implementor, so that consumers know what to expect.

The above statement seems to contradict:

> I've added a PR to make what getHeaders() should return explicit: -
> https://github.com/php-fig/fig-standards/pull/347

The first says that an implementer could store the stringable object and
then return it later for further manipulation. The PR however says
strings, not strings-or-stringable-objects.

I can see an argument either way on forcing just strings there, but we
should be deliberate about which we do.

--Larry Garfield

[Matthew Weier O'Phinney]

No, no, what I was saying was this: it's up to the implementor to
determine if they will store the object or the string value of the
object internally. However, getHeaders() will ALWAYS return the string
value.

The point I was making before was that if an implementor delays
serialization until retrieval, that leaves the door open for somebody
to manipulate the object between passing it to the message and
retrieving headers from the message.

> The PR however says strings,
> not strings-or-stringable-objects.
>
> I can see an argument either way on forcing just strings there, but we
> should be deliberate about which we do.

There's no contradiction with the PR I've opened; what the PR changes is this:

- When setting/adding header values, you can provide an object capable
of serialization to a string.
- When retrieving headers, you will ALWAYS get the string values in return.

The only real question at this point is if we should specify in
(add|set)Headers?() that any objects passed as values MUST be
serialized IMMEDIATELY, or if we can leave that as an implementation
detail. As noted above, the choice of delaying serialization has a
potential impact on runtime code.