WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 2 years ago

#43757 closed enhancement (fixed)

`WP_REST_Attachments_Controller` includes entire admin includes for a few utility functions

Reported by: lonelyvegan Owned by: SergeyBiryukov
Milestone: 5.0 Priority: normal
Severity: minor Version: 5.1
Component: REST API Keywords: has-patch fixed-5.0
Focuses: Cc:

Description

Looks like https://core.trac.wordpress.org/browser/trunk/src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php uses only a few admin function (wp_handle_upload, wp_generate_attachment_metadata, wp_tempnam, and wp_handle_sideload) but it's requiring the entire admin.php include.

It seems we could just require file.php and image.php from the admin directly instead.

Attachments (4)

43757.diff (1.7 KB) - added by soulseekah 3 years ago.
leaner includes
43757.2.diff (1.5 KB) - added by pratikthink 3 years ago.
added correct sentence for comment
43757.3.diff (734 bytes) - added by soulseekah 2 years ago.
Handle missing wp_read_image_metadata() definition
43757.4.diff (721 bytes) - added by ocean90 2 years ago.

Download all attachments as: .zip

Change History (23)

@soulseekah
3 years ago

leaner includes

#1 @soulseekah
3 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Hey, welcome to Trac! :)

You appear to be very correct in this regard. Attached diff for leaner includes. Tests pass fine.

#2 @lonelyvegan
3 years ago

That's exactly the patch I was going to suggest but was having internet troubles and did get around to submitting it. Looks good to me, but I'm probably much to green to be reviewing patches to core. 😅 👍🏻

#3 @jrf
3 years ago

  • Focuses coding-standards removed

#5 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 5.0
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

@pratikthink
3 years ago

added correct sentence for comment

#6 @rachelbaker
2 years ago

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

In 43589:

REST API: Limit the scope of wp-admin files required in WP_REST_Attachments_Controller methods.

Narrow the scope of the included wp-admin files loaded for wp_generate_attachment_metadata(), wp_handle_upload(), wp_tempnam(), and wp_handle_sideload(). Requires only wp-admin/includes/file.php and wp-admin/includes/image.php instead of wp-admin/includes/admin.php.

Props lonelyvegan, soulseekah, pratikthink.
Fixes #43757.

#7 @ocean90
2 years ago

  • Keywords needs-patch added; has-patch needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

The change is causing a fatal error by create_item() due to the undefined function wp_read_image_metadata().

This happens because upload_from_data() and upload_from_file() are now only loading wp-admin/includes/file.php.

@soulseekah
2 years ago

Handle missing wp_read_image_metadata() definition

#8 @soulseekah
2 years ago

Good catch. These are hard to test for via unit tests since the admin.php file is always included and all these functions are defined. Required image.php in the 3rd iteration.

#9 @afercia
2 years ago

Just stumbled upon this while working on Gutenberg. Can we get this in as soon as possible please?

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


2 years ago

#11 @rachelbaker
2 years ago

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

In 43604:

REST API: Load missing required file for multisite users in WP_REST_Attachments_Controller::create_item().

Requires wp-admin/includes/image.php to make wp_read_image_metadata() function available. Fixes error introduced in [43589].

Props ocean90, soulseekah.
Fixes #43757.

#12 @SergeyBiryukov
2 years ago

  • Keywords needs-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[43589] and [43604] should be backported to the 5.0 branch.

#13 @danielbachhuber
2 years ago

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

In 43773:

REST API: Limit the scope of wp-admin files required in WP_REST_Attachments_Controller methods.

Narrow the scope of the included wp-admin files loaded for wp_generate_attachment_metadata(), wp_handle_upload(), wp_tempnam(), and wp_handle_sideload(). Requires only wp-admin/includes/file.php and wp-admin/includes/image.php instead of wp-admin/includes/admin.php.

Props ocean90, lonelyvegan, soulseekah, pratikthink.
Merges [43589], [43604] to the 5.0 branch.
Fixes #43757.

#14 @ocean90
2 years ago

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

There's still a fatal error when uploading a video or an audio file due to the missing wp_read_video_metadata()/wp_read_audio_metadata() functions. The functions are defined in wp-admin/includes/media.php and called by wp_generate_attachment_metadata() in wp-admin/includes/image.php

@ocean90
2 years ago

#15 @ocean90
2 years ago

  • Keywords has-patch added; needs-patch removed

#16 @danielbachhuber
2 years ago

In 43850:

REST API: Restore access to audio/video metadata functions.

Ensures wp_read_video_metadata()/wp_read_audio_metadata() functions are available when uploading video and audio. Fixes error introduced in [43589].

Props ocean90.
See #43757.

#17 @danielbachhuber
2 years ago

  • Keywords fixed-5.0 added

[43850] will need to be merged to trunk now.

#18 @jeremyfelt
2 years ago

In 44216:

REST API: Restore access to audio/video metadata functions.

Ensures wp_read_video_metadata()/wp_read_audio_metadata() functions are available when uploading video and audio. Fixes error introduced in [43589].

Merges [43850] from the 5.0 branch to trunk.

Props ocean90.
See #43757.

#19 @SergeyBiryukov
2 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.