#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 | Owned by: | 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)
Change History (33)
This ticket was mentioned in Slack in #core by nerrad. View the logs.
10 years ago
#2
@
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 returnsfalse
or an object which will always have both thedomain
andpath
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 likeget_id_from_blogname()
would need similar scrutinization. - We could check the object more specifically, too:
return ( isset( $bloginfo->domain ) && isset( $bloginfo->path ) ) ? ... : '';
#4
follow-up:
↓ 5
@
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
@
10 years ago
Replying to nerrad:
Would be even better if the object was not a stdClass object
Agree, true WP_Site
and WP_Network
objects would be ideal. And unit tests. :)
#6
@
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?
#7
@
10 years ago
- Keywords has-patch added; needs-unit-tests removed
Tests added and I went with your suggestion for using isset()
checks johnjamesjacoby
@
10 years ago
sigh. the times when I wish I could delete submitted patches... This is the one that should be used.
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
@
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 ishttp://
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
@
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.
#14
@
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
@
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
@
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 :)
#17
@
10 years ago
- Owner set to jeremyfelt
- Resolution set to fixed
- Status changed from new to closed
In 31157:
#18
follow-up:
↓ 19
@
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:
↓ 20
@
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
@
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
@
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
@
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
@
10 years ago
- Resolution set to fixed
- Status changed from reopened to closed
[31178] missed the ticket.
#24
@
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
@
10 years ago
- Resolution set to worksforme
- Status changed from reopened to closed
oh wait nevermind its still returning an empty string. doh.
fix php notice for get_blogaddress_by_id with invalid id.