Fix "Trying to get property of non-object" notice when calling get_blogaddress_by_id() with an invalid id.

[nerrad]

Patch coming.

[nerrad]

fix php notice for get_blogaddress_by_id with invalid id.

[johnjamesjacoby]

Seems fine enough as a small improvement.

---

Thinking out loud:

• In this case, we can trust the results of get_blog_details() as it either returns false or an object which will always have both the domain and path variables set. But, because plugins are forced to fill in many multisite/network/mapping gaps, the possibility of an unexpected cache value is higher than usual.
• We could set a default 0 value for the $blog_id parameter to suppress another possible & similar notice, but other functions like get_id_from_blogname() would need similar scrutinization.
• We could check the object more specifically, too:

  return ( isset( $bloginfo->domain ) && isset( $bloginfo->path ) ) ? ... : '';
  

[nerrad]

Would be even better if the object was not a stdClass object and instead an actual WP_Site_Blog object or something like that because then one could just do an instanceof check. But that's a much bigger deal outside the scope of the ticket. The isset() checks would be fine too.

The point is there needs to be SOME sort of validation of the returned value here.

[johnjamesjacoby]

Replying to nerrad:

Would be even better if the object was not a stdClass object

Agree, true WP_Site and WP_Networkobjects would be ideal. And unit tests. :)

[nerrad]

unit tests I can do for this ticket and will try to get them done tonight.

Long term though, are there already plans for implementing WP_Site and WP_Network objects? If not, would a good intermediate step be to create such objects using the existing stdClass properties? If so, I'd be open to starting another ticket and doing some preliminary work on that?

[nerrad]

use isset to check for required properties and add unit tests.

[nerrad]

Tests added and I went with your suggestion for using isset() checks johnjamesjacoby

[nerrad]

Use this one! Typo in previous patch.

[nerrad]

sigh. the times when I wish I could delete submitted patches... This is the one that should be used.

[nerrad]

The last patch should be used, fixes a typo in the unit tests.

[jeremyfelt]

This is great @nerrad, thanks! We should hold on this for now, but get it in as soon as the 4.2 cycle starts.

A couple notes:

• Is there an opportunity for us to change the response to false if an invalid ID is passed? The current behavior is http:// rather than an empty string, so we're breaking a bit anyway.
• I'm a little wary of the ternary, but it's likely ok to leave.
• Let's split the unit test into two. One with a valid ID, one with an invalid ID. We definitely don't need a new site via factory and the invalid test. We can probably get away without one in the valid test as well and just use 1 as the ID.
• It's also ugly that http:// is hardcoded here, but that may be for another ticket.

[nerrad]

I returned an empty string because in the original http:// is a string. In my opinion returning the same data type is better and since the caller is expecting a string to be returned...

Splitting the unit test in two works, and yeah you're right about the unit tests not needing new sites. I'll implement.

I don't like http:// being hardcoded either, maybe this would be a good opportunity to use something like set_url_scheme() to setup the url correctly? I imagine the function as is is prone to bugs when used on multisite installs that are https:// only.

Yeah and future release (4.2) is fine for me, I know we're late in the current dev cycle.

[jeremyfelt]

@nerrad I think changing the hard coded http:// is definitely another ticket. Looks like a bit of clean up in the unit tests and we're good.

[nacin]

Changing that to HTTPS in a distant past brought down .com. I know because it was my change. There be dragons. (Also, there be a ticket somewhere.)

[nerrad]

just posting I seen this, I'll get the cleanup of the tests in soon (hopefully by end of day).

@nacin ... ooo treading carefully then :)

[nerrad]

split tests into two don't create site.

[jeremyfelt]

In 31157:

Check for existence of data from get_blogaddress_by_id() before returning a URL

• Prevent a notice when an invalid ID is used with get_blogaddres_by_id().
• Return a falsy empty string rather than the previous "​http://".
• Add unit tests for get_blogaddress_by_id().

Props nerrad.

Fixes #30566.

[SergeyBiryukov]

Looks like get_blog_details() returns false on failure.

Could we just check for a truthy value and remove the isset() checks? See 30566.patch​.

[nerrad]

Replying to SergeyBiryukov:

Looks like get_blog_details() returns false on failure.

Could we just check for a truthy value and remove the isset() checks? See 30566.patch​.

I was going to just check for a truthy value, but get_blog_details() also has a late filter run on it. So its *possible* a plugin might return a non truthy value. The isset checks cover that possibility.

[SergeyBiryukov]

Replying to nerrad:

I was going to just check for a truthy value, but get_blog_details() also has a late filter run on it. So its *possible* a plugin might return a non truthy value.

Yeah, but returning anything other than a blog object would cause an issue in get_blog_details() itself when calculating the cache key​. So that would be a developer error that would result in a notice anyway.

[nerrad]

AHH gotcha... you are correct. Well then all that's really needed is to update the tests for non truthy return on invalid id.

[jeremyfelt]

Good point @SergeyBiryukov. If we can't expect the proper object or false here, then we'll see the problem before get_blogaddress_by_id() gets the return.

[jeremyfelt]

[31178] missed the ticket.

[nerrad]

@jeremyfelt, the tests will need updated. I'll do a patch and will add.

[nerrad]

oh wait nevermind its still returning an empty string. doh.