WordPress.org

Make WordPress Core

Opened 3 months ago

Closed 6 weeks ago

#52030 closed defect (bug) (fixed)

Media metaboxes return fatal error if no author metadata present

Reported by: carloscastilloadhoc Owned by: SergeyBiryukov
Milestone: 5.6.1 Priority: normal
Severity: normal Version: 5.6
Component: Media Keywords: has-patch commit fixed-major
Focuses: Cc:

Description

Images uploaded programatically to the media library via the wp_insert_attachment function won't have author metadata. This causes a fatal error in the metaboxes when you try to use the "edit media" option in the media library.

Specifically: Uncaught Error: Call to a member function exists() on bool in wp-admin/includes/media.php line 3295

This can be fixed for example by expanding the condition "if($author->exists())" with "if($author && $author->exists())".

While I haven't personally encountered them, this may cause additional errors in other places where the same check is performed (for example the wp_prepare_attachment_for_js function in wp-includes/media.php).

Thank you and kind regards.

Attachments (1)

52030.patch (457 bytes) - added by antpb 3 months ago.
initial patch for attachment_submitbox_metadata

Download all attachments as: .zip

Change History (11)

#1 @hellofromTonya
3 months ago

  • Milestone changed from Awaiting Review to 5.6.1

Hello @carloscastilloadhoc,

Welcome to WordPress Trac! Thank you for this ticket.

Introduced in #46866 and changeset [49207], a check is missing for get_userdata() as it returns false when no user is found. At a minimum, this code should be guarded. Moving this ticket into 5.6.1 to add this guard.

For the case where an image is programmatically created without an assigned user, more discussions is needed. Pinging @antpb @garrett-eclipse for their thoughts.

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


3 months ago

#3 @SergeyBiryukov
3 months ago

As noted above, the code in attachment_submitbox_metadata() uses get_userdata():

$author = get_userdata( $post->post_author );

which indeed returns false on failure.

However, the similar code in wp_prepare_attachment_for_js() creates a new instance of WP_User instead:

$author = new WP_User( $attachment->post_author );

Would be great to have some consistency here.

@antpb
3 months ago

initial patch for attachment_submitbox_metadata

#4 @antpb
3 months ago

  • Keywords has-patch needs-testing added

Thanks @SergeyBiryukov , good call on consistency. I've uploaded an initial patch for us to work off of. @garrett-eclipse, I would really appreciate a review. :) Is there any other level of guard this needs? I assume since new WP_User() it can't result in fatal.

Last edited 3 months ago by antpb (previous) (diff)

#5 follow-up: @garrett-eclipse
3 months ago

  • Keywords commit added; needs-testing removed

Thanks for the catch @carloscastilloadhoc, the triage @hellofromTonya, the idea @SergeyBiryukov and the patch @antpb. It's looking and working good for me so am marking for commit. Carlos if you could confirm the change fixes your install that would also be appreciated.

#6 in reply to: ↑ 5 @carloscastilloadhoc
3 months ago

Replying to garrett-eclipse:

Thanks for the catch @carloscastilloadhoc, the triage @hellofromTonya, the idea @SergeyBiryukov and the patch @antpb. It's looking and working good for me so am marking for commit. Carlos if you could confirm the change fixes your install that would also be appreciated.

I do confirm the patch fixes the issue on my end, thanks.

#7 @SergeyBiryukov
2 months ago

#52145 was marked as a duplicate.

#8 @SergeyBiryukov
7 weeks ago

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

In 49979:

Media: Use consistent method for instantiating an attachment author object in Media Library.

Previously, attachments without an author could cause a PHP fatal error due to calling the ::exists() method on a false value.

Props antpb, carloscastilloadhoc, hellofromTonya, garrett-eclipse.
Fixes #52030.

#9 @SergeyBiryukov
7 weeks ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backporting to the 5.6 branch.

#10 @SergeyBiryukov
6 weeks ago

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

In 49995:

Media: Use consistent method for instantiating an attachment author object in Media Library.

Previously, attachments without an author could cause a PHP fatal error due to calling the ::exists() method on a false value.

Follow-up to [49207].

Props antpb, carloscastilloadhoc, hellofromTonya, garrett-eclipse.
Merges [49979] to the 5.6 branch.
Fixes #52030.

Note: See TracTickets for help on using tickets.