Opened 4 years ago
Last modified 4 years ago
#48565 new defect (bug)
The `site_url` function doesn't work with objects implemented `__toString`
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | trivial | Version: | 5.2.4 |
Component: | General | Keywords: | needs-patch close |
Focuses: | Cc: |
Description
The site_url
function omits the the path if the passed parameter is an object with __toString
method implemented.
for example:
<?php class Str { protected $string; public function __construct($string) { $this->string = $string; } public function doSomething() { // do something with $this->string; } public function __toString() { return $this->string; } } $path = new Str('/store/locations/'); $path->doSomething(); echo site_url($path); // The above will return https://example.com/ // whereas it should return https://example.com/store/locations/
Change History (6)
#2
in reply to:
↑ 1
@
4 years ago
Yeah, I agree but I also think this should be addressed if possible because it doesn't feel good to call __toString
directly since it meant to be called by PHP?
Though I don't feed really good about it but I suggest we should have a helper maybe called wp_is_string
and replace is_string
calls with that, If you think that's a good idea I will be more than happy to submit a patch.
Replying to pbiron:
Thanx for the ticket and welcome to Trac!
That's because get_site_url() (which is called by
site_url()
) has an explicit check for whether the$path
param is a string (see L3165). If it's not a string, then it is ignored.
While it would certainly be possible to add an additional check for whether
$path
is an object that has a__toString()
method, why would that be desired? If it were done in this one case, then it could be argued that it should be done for ANY "string" param to ANY function/method in core...and doing so doesn't seem reasonable to me.
I'd suggest you just call
__toString()
yourself when you callsite_url()
.
#3
@
4 years ago
I'm not sure whether a wp_is_string()
is a good idea or not. In what context do you need to pass an such an object to site_url()
?
I just did a quick search and there are almost 250 calls to is_string()
in core...so it would take some effort to assess the performance impact of the extra function call overhead. A single extra function call is negligible, but something called in a large loop or recursively might result in a noticeable impact.
The helper would be real easy:
function wp_is_string( $str ) {
return is_string( $str ) || ( is_object( $str ) && method_exists( $str, '__toString' ) );
}
#5
follow-up:
↓ 6
@
4 years ago
- Keywords close added; dev-feedback 2nd-opinion removed
This is an interesting one. An object of a class with a __toString()
method is not a string, however it will get cast to a string when necessary (for example if it gets passed to a parameter with a type hint of string
, or gets explicitly casted with (string)
).
Personally I think this is enough of an edge case that this ticket should be closed off. The additional overhead to handle this case is not worth it. Just cast it to a string before passing into functions that expect a string.
#6
in reply to:
↑ 5
@
4 years ago
Replying to johnbillion:
it will get cast to a string when necessary (for example if it gets passed to a parameter with a type hint of
string
, or gets explicitly casted with(string)
).
I didn't realize that such casting happened on anything other PHP internals (e.g., strlen()
). Looking at the PHP Docs, unfortunately, the ability to use string
as a type hint wasn't introduced until 7.0, so until the minimum PHP requirement reaches that we couldn't add such type hints.
Plus, in the case of get_site_url()
I think the reason for the is_string()
check is to guard against passing, e.g., an array
, without raising runtime errors. And using the type hint would generating runtime errors if a non-string (or something that can't be passed to string) were passed. I'm not sure that would be desirable behavior in this case.
Thanx for the ticket and welcome to Trac!
That's because get_site_url() (which is called by
site_url()
) has an explicit check for whether the$path
param is a string (see L3165). If it's not a string, then it is ignored.While it would certainly be possible to add an additional check for whether
$path
is an object that has a__toString()
method, why would that be desired? If it were done in this one case, then it could be argued that it should be done for ANY "string" param to ANY function/method in core...and doing so doesn't seem reasonable to me.I'd suggest you just call
__toString()
yourself when you callsite_url()
.