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

[5.4] Optimize UrlGenerator->isValidUrl() #18566

Merged
merged 1 commit into from
Mar 29, 2017
Merged

[5.4] Optimize UrlGenerator->isValidUrl() #18566

merged 1 commit into from
Mar 29, 2017

Conversation

vlakoff
Copy link
Contributor

@vlakoff vlakoff commented Mar 29, 2017

UrlGenerator->to() and UrlGenerator->asset() are quite slow. Investigating about this, I found out UrlGenerator->validUrl() was a bottleneck, taking by itself about half of the total execution time.

Then, I found out this method could be made significantly faster:

$path = 'foobar';
$nb = 10000;

$t1 = microtime(true);
for ($i = $nb; $i--; ) {
    if (Str::startsWith($path, ['#', '//', 'mailto:', 'tel:', 'http://', 'https://'])) {}
}
$t2 = microtime(true);
for ($i = $nb; $i--; ) {
    if (preg_match('~^(#|//|https?://|mailto:|tel:)~', $path)) {}
}
$t3 = microtime(true);

echo $t2 - $t1, "\n", $t3 - $t2;

Before: 673 ms
After: 76 ms

@taylorotwell taylorotwell merged commit 94272a4 into laravel:5.4 Mar 29, 2017
@@ -444,7 +444,7 @@ public function format($root, $path)
*/
public function isValidUrl($path)
{
if (! Str::startsWith($path, ['#', '//', 'mailto:', 'tel:', 'http://', 'https://'])) {
if (! preg_match('~^(#|//|https?://|mailto:|tel:)~', $path)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it should include http as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does, https?:// matches https:// and http://

@vlakoff vlakoff deleted the url_generator branch March 30, 2017 17:04
@vlakoff
Copy link
Contributor Author

vlakoff commented Mar 30, 2017

I investigated why the http[s] protocols had been added (ec1b1ae) while the filter_var below supposedly supports them already (and does naive checks thus is quite permissive).

So, I guess it's to allow internationalized domain names, which are not supported by filter_var():

filter_var('http://스타벅스코리아.com', FILTER_VALIDATE_URL) // false

@fernandobandeira
Copy link
Contributor

It should work with php-intl enabled...

Anyway shouldn't we consider making this check more strict? Currently it seems that http://anything will pass even if theres no .tld there...

@vlakoff
Copy link
Contributor Author

vlakoff commented Mar 30, 2017

It should be fine as it is. This method is not really for proper validation, but just to determine if the argument is a route or an URL, so that the developer can pass both to e.g. url() without caring.

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

Successfully merging this pull request may close these issues.

5 participants