WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#46300 closed enhancement (fixed)

Return type/value of get_site is not being checked in wp_insert_site

Reported by: david.binda Owned by: SergeyBiryukov
Milestone: 5.1.1 Priority: normal
Severity: normal Version: 5.1
Component: Networks and Sites Keywords: has-patch commit
Focuses: multisite Cc:

Description

The get_site function may return null in case the site is not found ( see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/ms-site.php?rev=44727#L293 ), otherwise returns a WP_Site object.

The function is being called from inside the wp_insert_site (see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/ms-site.php?rev=44727#L70 ) and the WP_Site object is implicitly expected, as the follow-up code treats the $new_site variable as an object (eg.: https://core.trac.wordpress.org/browser/trunk/src/wp-includes/ms-site.php?rev=44727#L114).

I haven't run into any issues, but it might be worth checking the return type and return WP_Error out of the wp_insert_site instead of 0 in case the call to get_site fails. (the 0 would be an result of null->id, producing null casted to int).

Attachments (2)

46300.0.diff (412 bytes) - added by vanyukov 3 years ago.
return WP_Error on null
46300.1.diff (407 bytes) - added by lukecarbis 3 years ago.
Fix typo in error message.

Download all attachments as: .zip

Change History (12)

#1 @pento
3 years ago

  • Milestone changed from Awaiting Review to 5.1.1

Good find, thanks @davidbinda! Putting this in 5.1.1, as it's mostly about better error case handling. If get_site() has failed immediately after the $wpdb->insert() succeeds, there are probably bigger problems. 🙂

#2 @david.binda
3 years ago

Totally agree! Thank you.

This ticket was mentioned in Slack in #core-multisite by lukecarbis. View the logs.


3 years ago

@vanyukov
3 years ago

return WP_Error on null

#4 @vanyukov
3 years ago

  • Keywords has-patch added

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


3 years ago

#6 @pbiron
3 years ago

The patch looks good to me, except for the typo in the error message (/retrieves/retrieve/).

@lukecarbis
3 years ago

Fix typo in error message.

#7 @lukecarbis
3 years ago

  • Keywords commit added

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


3 years ago

#9 @SergeyBiryukov
3 years ago

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

In 44799:

Networks and Sites: Check the result of get_site() in wp_insert_site().

Props davidbinda, vanyukov, lukecarbis.
Fixes #46300.

#10 @SergeyBiryukov
3 years ago

In 44800:

Networks and Sites: Check the result of get_site() in wp_insert_site().

Props davidbinda, vanyukov, lukecarbis.
Merges [44799] to the 5.1 branch.
Fixes #46300.

Note: See TracTickets for help on using tickets.