thephpleague / uri-schemes

Collection of URI Immutable Value Objects
https://uri.thephpleague.com/schemes/
MIT License
216 stars 7 forks source link

The __toString method from Uri has a side effect #10

Closed LucWollants closed 6 years ago

LucWollants commented 6 years ago

Issue summary

When calling the (string) conversion or the __toString method on Uri the property $uri gets changed. This is most likely done for performance reasons. But this method is a pure getter method and should not cause any side effects.

Code snippet

$uri = Uri::createFromString('https://github.com/thephpleague/uri');
// The uri property is null
var_dump($uri);

$uriAsString = (string) $uri;
// The uri property is https://github.com/thephpleague/uri
var_dump($uri);

Solution

When calling the createFromString call getUriString inside the body of that factory method and store the created string uri it inside the uri property. Then the code on line https://github.com/thephpleague/uri-schemes/blob/master/src/AbstractUri.php#L682 would not trigger the uri initialisation and the side effect after construction.

nyamsprod commented 6 years ago

Thanks for using the library. And yes indeed this side effect exists but I'm not aware of any issue regarding this behavior since the $uri property is internal to the value object. I'll gladly consider your solution if you could tell me where this side effect could be problematic ?

nyamsprod commented 6 years ago

An alternative to your solution would be to re-introduce the __debugInfo method. I thought I had implemented it but seems I forgot it 😥

LucWollants commented 6 years ago

First of all thanks for taking the time to read up on this report.

We don't have a real breaking issue but it took us some time to figure out why comparing Uri's which were stored inside the database as a string column and created from the database from that same string column would no longer match.

We created a custom type for doctrine and noticed it inside or repository tests: https://github.com/VSVverkeerskunde/gvq-api/blob/feature/VSVGVQ-7/src/Question/Repositories/Mappings/UriType.php#L40

Basically what happens in the unit tests is this:

$uri = Uri::createFromString('https://github.com/thephpleague/uri');
$sameUri = Uri::createFromString('https://github.com/thephpleague/uri');
if ($uri == $sameUri) {
    echo 'They have the same properties';
}

$sameUri->__toString();
if ($uri != $sameUri) {
    echo 'They don\'t the same properties';
}
nyamsprod commented 6 years ago

Ok understood but you should not compare value object like normal objects. Value object should be compare against there value so two URI objects are considered equal only if their string representations are equals. The facts that 2 instances are not the same is irrelevant in case of immutable value objects which URI objects are. So again, I could add the debugInfo but in your case it will not solve your real issue which is your tests.

nyamsprod commented 6 years ago

You can get more informations here https://codete.com/blog/value-objects/

LucWollants commented 6 years ago

Thanks for pointing out that blog post!

We indeed try to use value objects to create domain models in a consistent state and keep them immutable. The underlying problem is the use of the PHPUnit framework and the method assertEquals. As we do here https://github.com/VSVverkeerskunde/gvq-api/blob/feature/VSVGVQ-7/tests/Question/Repositories/QuestionDoctrineRepositoryTest.php#L98

Instead we could introduce our own equals method on the Question domain model and use that custom comparison.

nyamsprod commented 6 years ago

@LucWollants did you fix your issue ? If yes could you close this issue ?

LucWollants commented 6 years ago

I did indeed implement a work-around.