fix: improve isRelative performance #20095
Conversation
SamMousa
commented
Jan 3, 2024
SamMousa
commented
| Q | A |
|---|---|
| Is bugfix? | ✔️ |
| New feature? | ❌ |
| Breaks BC? | ❌ |
| Fixed issues | #17191 #20077 |
PR Summary
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #20095 +/- ##
===========================================
- Coverage 48.02% 18.72% -29.30%
===========================================
Files 445 445
Lines 43890 43664 -226
===========================================
- Hits 21078 8176 -12902
- Misses 22812 35488 +12676 ☔ View full report in Codecov by Sentry. |
| @@ -378,8 +378,7 @@ public static function home($scheme = false) | |||
| */ | |||
| public static function isRelative($url) | |||
| { | |||
| $urlComponents = parse_url($url, PHP_URL_SCHEME); | |||
| return strncmp($url, '//', 2) && empty($urlComponents); | |||
| return preg_match('~^[[:alpha:]][[:alnum:]+-.]*://|^//~', $url) === 0; | |||
There was a problem hiding this comment.
Can we get away here with just using \w with additional +-. ? It adds underscore to the list but should not be problematic I think.
There was a problem hiding this comment.
I agree it wouldn't be problematic, but why make it less correct? What's the benefit?
There was a problem hiding this comment.
Hm, not sure why but in my head this feels faster? 🤣 strange how the mind works, hehe. Ok, @rob006 could you take a look?
There was a problem hiding this comment.
I think we should choose pattern that is faster - IMO it is not worth to have more strict check here to cover edge cases like 123://example.com, if it will be slower for basically all real-world use cases.
Also, IMO it should be '~^[[:alpha:]][[:alnum:]+-.]+://|^//~', to not accept URLs like ://example.com
There was a problem hiding this comment.
Both your examples are already not accepted. The first character has to be alpha, then 0 or more alphanumerics (and a few special chars).
Have you got data to show that this regex is slow? It is simple and has an anchor at the start, I don't see why it would be slow. I'm skeptical any change would be speeding it up immensely, since most time is spent in compilation not evaluation of the expression.
|
Thank you! |
