The `site_url` function doesn't work with objects implemented `__toString`

[azeemhassni]

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/

[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 call site_url().

[azeemhassni]

Yeah, I agree but I also think this should be addressed if possible because it doesn't feel good to call __toString directly because it meant 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 call site_url().

[pbiron]

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' ) );
}

[johnbillion]

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.

[pbiron]

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.

[hellofromTonya]

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.

I agree with @johnbillion. This one is an interesting edge case that can be handled in before passing the path to site_url(). I don't think the overhead though benefits the majority of users.

With no further follow-up for 4 years, I'll close this ticket as wontfix.

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.

@pbiron this kind of strict data type checking should be part of a architectural design discussion of how to handle function/method input validation. For back-compat, consideration is needed for how Core has handled non-string data types. Your point is valid and something to holistically consider across Core's codebase.