Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PSR-7] Adding a Errata section to PSR-7 to clarify UriInterface modification mechanism #619

Closed
wants to merge 1 commit into from
Closed

[PSR-7] Adding a Errata section to PSR-7 to clarify UriInterface modification mechanism #619

wants to merge 1 commit into from

Conversation

nyamsprod
Copy link

Adding clarification when invalid URI string are produced after modification of a UriInterface with a valid URI component/part value.

Adding clarification when invalid URI string are produced after modification of a UriInterface with a valid URI component/part value.
@weierophinney
Copy link
Contributor

👍 This is an important detail; both @nyamsprod and I have seen issues crop up in PSR-7 implementations already from folks expecting that the URI will always be well-formed, and raise exceptions as soon as it is not. Documenting that new URI instances may be invalid due to missing components will help alleviate those concerns.

@Tobion
Copy link
Contributor

Tobion commented Dec 9, 2015

Could you give an example where a resulting URI is not valid and which is not handled in UriInterface::__toString phpdoc?

Btw, there is actually a workaround to throw exceptions in __toString. See symfony/symfony#15076
Maybe we should recommend this practice in the meta.

@nyamsprod
Copy link
Author

@Tobion

Let's use Zend Diactoros to illustrate:

use Zend\Diactoros\Uri;

$uri = new Uri('http://www.example.com');
$invalidUri = $uri->withHost('')->withPath('foo/bar');

echo $invalidUri, PHP_EOL; // returns http:///foo/bar

The URI parts are all valid but the resulting URI string representation is not

@Tobion
Copy link
Contributor

Tobion commented Dec 9, 2015

Well, this is a wrong behavior from Zend\Diactoros\Uri. The given example must return http:foo/bar which is a valid URI and which is according to PSR-7: no authority -> no // and also no / in front of the path.

It might not be a valid URI in HTTP scheme, but a valid URI in general. Imagine the same with scheme mailto and path example@example.org ->mailto:example@example.org.

@nyamsprod
Copy link
Author

Quoting the PSR-7 doc

UriInterface models HTTP and HTTPS URIs as specified in RFC 3986 (the primary use case).

PSR-7 UriInterface does not models general URI. So even if Diactoros is wrong nothing in PSR-7 says that a valid modification can end up generating an Invalid URI object. That's why I think a clarification is indeed needed. URI validation should be done on a scheme based, like I did with League\Uri

@michaelcullum
Copy link
Member

Errata requires a vote and the vote must be started by a voting member. @weierophinney?

@Tobion
Copy link
Contributor

Tobion commented Dec 9, 2015

@nyamsprod I agree a clarification could be a good idea. But maybe we should instead clarifiy that the possibility to error out of __toString when the URI components cannot form a valid URI (in the given scheme) is possible with the above linked workaround. So the assumption that toString must not throw an exception is wrong.


Supplying a valid value to a `UriInterface` modifying method does not guarantee that the resulting `UriInterface`
object will generate a valid URI string. Because no exception can be thrown in a `__toString` method, the invalid
URI string can only be detected when the new instance is created. In such situation, a `RuntimeException` exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How I understand is, that the RuntimeException may be thrown in the constructor of an URI implementing UriInterface, right?
I think the mention of the RuntimeException does not make sense here as PSR-7 does not specify the constructor at all. So the constructor can throw any exception and is implementation-specific.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception could also be thrown with the modifying methods too. Since with* return a new/clone methods with the modified information.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO it's a value object so no 'domain' logic inside. Exception should be thrown only by the objects which use it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a value object so it should validate its properties on construction. Otherwise it is not a value object. The modifying method generate a new object so the resulting properties should be validated indivdually and against each other to ensure the value object is valid.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It all depends on the complexity of the domain model. For a simple model validation can be inefficient and unnecessary. For areas more complex you can avoid duplication of code in the other components by using the decorator:

class ValidUri implements \Psr\http\Message\UriInterface {
     / **
      *   throws \DomainException
      * /
     public function __construct (\Psr\http\Message\UriInterface $uri)
     {
     }
}

In summary, I think the standard should not be forcing customers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krzysiekpiasecki the issue is that you seem to think that an URI is a simple model, which is not (rfc3986 contains around 60 pages!!). I don't understand the added value of a decorator please check my simple example

use Zend\Diactoros\Uri;

$uri = new Uri('http://www.example.com');
$invalidUri = $uri->withHost('')->withPath('foo/bar');

echo $invalidUri, PHP_EOL; // returns http:///foo/bar

What would a decorator adds or simplify ? using UriInterface::withPath should thrown an exception.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue is that you seem to think that an URI is a simple model

I mean the application model.

Very simple application with one component using URI and complex application with many components using it. In simple cases i prefer for example 'filter_var' and also add my own validation rules, etc.

I don't understand the added value of a decorator
What would a decorator adds ?

  • Constitues and extracts custom validation layer,
  • No duplication in complex model, when value object is not validated by itself, but components must relay on him.

As I sad it's only my opinion about this errata :)

Regards

@michaelcullum
Copy link
Member

Ping @weierophinney. Any thoughts on what you want to do in regards to this? Vote for errata or close?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants