Url 'javascript:void();' results in fatal error about url

[freekmurze]

Currently this package is not able to handle such urls, so I'm thinking it's a good thing that the exception is being thrown.

What behaviour would you expect? Feel free submit a PR.

[klor]

I would expect any unsupported protocol to die gracefully - and without breaking the current script ... so in case a url uses a non-supported protocol, a notification may be given at the most, whereas raising a fatal error and breaking the script might be too much ... read also about error levels.

Right now, the right way to use Spatie\Url when parsing a set of unknown urls is not so elegant:

<?php
require '../vendor/autoload.php';

use Spatie\Url\Url;

$urls = [
	'https:www.example.com/',
	'javascript:void();',
];

foreach ($urls as $v) {
	try {
		echo Url::fromString($v);
	} catch (Exception $e) {
		echo $e;
	}
}

// Better
foreach ($urls as $v) {
	echo Url::fromString($v);
}

Note also that it is not possible to use the @-operator to make Spatie Url silent as echo @Url::fromString('javascript1:void();'); results in another fatal error.

I fixed the issue by modifying Url.php:L37 (this allows me to use Spatie Url without a try-catch):

const VALID_SCHEMES = ['http', 'https', 'mailto', 'javascript'];
                                                                                   ^

PS: Issues are being closed super-fast here..

[freekmurze]

Thank you for the detailed explanation. Changing this behaviour now could be considered a breaking change. Some people might depend on the current behaviour of an exception being thrown.

If you need this I suggest you use a more fleshed out package like https://github.com/thephpleague/uri

[klor]

Happy New Year! And thanks for the consideration and suggestion.

This can also be seen as a bug in the implementation, as the RFC3986 URI specification does not require an exception to be raised in this scenario. The specification only requires that an InvalidArgumentException is raised if the user attempts to manipulate the scheme with withScheme(), but it's ok to retrieve a link with an invalid scheme - and preventing the user from doing so goes beyond the specification.

Note also that this library does not have a mechanism to ask "is this a valid url?" (because it's not a validator) - so there is no way to prevent the fatal error from occurring in the first place, which also calls for not being too harsh on invalid links.

Anyways, to anyone else who face this issue, the easy fix is to comment out Url.php L218:

// throw InvalidArgument::invalidScheme($scheme);

PS: Another thought.. The library would benefit from having a way to notify the user, for instance a logging mechanism or maybe just use trigger_error('Error!',E_USER_NOTICE). Right now it's only possible to throw exceptions ("fatal errors" that break the script). This sample code shows how trigger_error() can notify without breaking:

echo "Watch me dance!";
trigger_error('Error!',E_USER_NOTICE);
echo "I'm still dancing!";

Output:

Watch me dance!
PHP Notice: Error! spatie-bug.php on line 5
Notice: Error! spatie-bug.php on line 5
I'm still dancing![Finished in 0.0s]

[freekmurze]

I'll take this in consideration when we make a new major release of this package. Thanks for the suggestion.