Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 9 years ago

#30566 closed defect (bug) (fixed)

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

Reported by: nerrad's profile nerrad Owned by: jeremyfelt's profile jeremyfelt
Milestone: 4.2 Priority: normal
Severity: normal Version: 3.0
Component: Networks and Sites Keywords: has-patch
Focuses: multisite Cc:

Description

Patch coming.

Attachments (6)

fix-notice.30566.diff (529 bytes) - added by nerrad 10 years ago.
fix php notice for get_blogaddress_by_id with invalid id.
fix-notice-with-tests.30566.diff (1.8 KB) - added by nerrad 10 years ago.
use isset to check for required properties and add unit tests.
fix-notice-with-tests-2.30566.diff (1.8 KB) - added by nerrad 10 years ago.
Use this one! Typo in previous patch.
fix-notice-with-tests-3.30566.diff (1.8 KB) - added by nerrad 10 years ago.
sigh. the times when I wish I could delete submitted patches... This is the one that should be used.
fix-notice-with-tests-4.30566.diff (1.7 KB) - added by nerrad 10 years ago.
split tests into two don't create site.
30566.patch (1.0 KB) - added by SergeyBiryukov 10 years ago.

Download all attachments as: .zip

Change History (33)

@nerrad
10 years ago

fix php notice for get_blogaddress_by_id with invalid id.

This ticket was mentioned in Slack in #core by nerrad. View the logs.


10 years ago

#2 @johnjamesjacoby
10 years ago

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 ) ) ? ... : '';
    

#3 @boonebgorges
10 years ago

  • Keywords needs-unit-tests added

#4 follow-up: @nerrad
10 years ago

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.

#5 in reply to: ↑ 4 @johnjamesjacoby
10 years ago

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. :)

#6 @nerrad
10 years ago

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
10 years ago

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

#7 @nerrad
10 years ago

  • Keywords has-patch added; needs-unit-tests removed

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

Last edited 10 years ago by nerrad (previous) (diff)

@nerrad
10 years ago

Use this one! Typo in previous patch.

@nerrad
10 years ago

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

#8 @nerrad
10 years ago

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

This ticket was mentioned in Slack in #core by nerrad. View the logs.


10 years ago

This ticket was mentioned in Slack in #core by nerrad. View the logs.


10 years ago

#11 @jeremyfelt
10 years ago

  • Milestone changed from Awaiting Review to Future Release

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.

#12 @nerrad
10 years ago

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.

Last edited 10 years ago by nerrad (previous) (diff)

#13 @johnbillion
10 years ago

  • Version changed from trunk to 3.0

#14 @jeremyfelt
10 years ago

  • Milestone changed from Future Release to 4.2

@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.

#15 @nacin
10 years ago

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.)

#16 @nerrad
10 years ago

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

@nacin ... ooo treading carefully then :)

Version 0, edited 10 years ago by nerrad (next)

@nerrad
10 years ago

split tests into two don't create site.

#17 @jeremyfelt
10 years ago

  • Owner set to jeremyfelt
  • Resolution set to fixed
  • Status changed from new to closed

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.

#18 follow-up: @SergeyBiryukov
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

#19 in reply to: ↑ 18 ; follow-up: @nerrad
10 years ago

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.

#20 in reply to: ↑ 19 @SergeyBiryukov
10 years ago

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.

#21 @nerrad
10 years ago

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

#22 @jeremyfelt
10 years ago

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.

#23 @jeremyfelt
10 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

[31178] missed the ticket.

#24 @nerrad
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#25 @nerrad
10 years ago

  • Resolution set to worksforme
  • Status changed from reopened to closed

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

#26 @SergeyBiryukov
10 years ago

  • Resolution changed from worksforme to fixed

This ticket was mentioned in Slack in #core-themes by joedolson. View the logs.


9 years ago

Note: See TracTickets for help on using tickets.