Make WordPress Core

Opened 17 months ago

Closed 10 months ago

Last modified 10 months ago

#57775 closed defect (bug) (fixed)

wp_create_file_in_uploads Called as Action Hook

Reported by: howdy_mcgee's profile Howdy_McGee Owned by: johnbillion's profile johnbillion
Milestone: 6.4 Priority: normal
Severity: trivial Version: 2.5
Component: General Keywords: has-patch needs-testing has-testing-info
Focuses: Cc:

Description

The wp_create_file_in_uploads hook is labeled as an Action Hook in the docs but is actually called multiple times as a Filter Hook.

As an Action Hook: class-custom-image-header.php

As a Filter Hook: ajax-actions.php, class-custom-image-header.php (and more)

To keep the docs accurate, this do_action should be switched to an apply_filters even though, in this case, nothing will be done with the returned value.

Attachments (1)

57775.diff (669 bytes) - added by Howdy_McGee 17 months ago.
Action Hook switched to Filter Hook.

Download all attachments as: .zip

Change History (13)

@Howdy_McGee
17 months ago

Action Hook switched to Filter Hook.

#1 @johnbillion
11 months ago

  • Focuses docs removed
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 6.4
  • Owner set to johnbillion
  • Status changed from new to reviewing

#2 follow-up: @johnbillion
11 months ago

  • Keywords needs-refresh 2nd-opinion added

Notes:

#3 in reply to: ↑ 2 @SergeyBiryukov
11 months ago

  • Version changed from 6.1.1 to 2.5

Replying to johnbillion:

I also think this should be converted to always be a filter. Converting it to always be an action will break existing filters. I'd like a second opinion though.

That makes sense to me too.

Introduced in [6551] / #5418, setting the version accordingly.

#4 @mhshujon
10 months ago

If we change the existing action hook to a filter hook as like this https://core.trac.wordpress.org/attachment/ticket/57775/57775.diff , then what's the point of using filter hooks if we don't use the modified $file variable?

This ticket was mentioned in PR #5408 on WordPress/wordpress-develop by @nicolefurlan.


10 months ago
#5

  • Keywords needs-refresh removed

Changes instances of wp_create_file_in_uploads from action hooks to filter hooks.

Trac ticket: https://core.trac.wordpress.org/ticket/57775

#6 follow-up: @nicolefurlan
10 months ago

  • Keywords needs-testing needs-testing-info added

I just added a PR that changes both instances of wp_create_file_in_uploads from action hooks to filter hooks.

I also created a docs issue (https://github.com/WordPress/Documentation-Issue-Tracker/issues/1139) so that https://developer.wordpress.org/reference/hooks/wp_create_file_in_uploads/ can get updated to match this change. Please do let me know if that was the incorrect thing to do. I'd love to know how we can coordinate with the docs team on timing to make sure the code and documentation changes happen at the same time.

#7 @nicolefurlan
10 months ago

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

Testing Instructions

These steps define how to test the feature or enhancement, and indicates the expected behavior or results.

Steps to Test

  1. Install and activate theme Twenty Fifteen or older.
  2. Upload a custom header image on Appearance > Header.
  3. Upload an custom background image on Appearance > Background.

Expected Results

Lists each expected result or behavior, i.e. what should happen when running the test(s):

  • ✅ Uploading a custom header image on Appearance > Header continues to work as expected (the selected image is uploaded to the Media Library and displays properly on the frontend)
  • ✅ Uploading a custom background image on Appearance > Background continues to work as expected (the selected image is uploaded to the Media Library and displays properly on the frontend)

Supplemental Artifacts

#8 @nicolefurlan
10 months ago

  • Keywords 2nd-opinion removed

Removing 2nd-opinion since @SergeyBiryukov weighed in an provided an opinion.

This PR just needs testing. It would be great to get this wrapped up in 6.4.

#9 @SergeyBiryukov
10 months ago

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

In 56929:

Media: Consistently call the wp_create_file_in_uploads hook as a filter.

The filter was initially introduced for file replication purposes. Since the returned value is not always used directly, some instances were later converted to an action as part of removing unused variables, and the hook was documented as an action while still being called as a filter in other instances.

This commit aims to correct the discrepancy between the code and the documentation.

Follow-up to [4673], [4817], [4818], [6363], [6551], [13041], [25821], [33154], [33280].

Props Howdy_McGee, nicolefurlan, johnbillion, mhshujon, SergeyBiryukov.
Fixes #57775.

@SergeyBiryukov commented on PR #5408:


10 months ago
#10

Thanks for the PR! Merged in r56929.

#11 in reply to: ↑ 6 @SergeyBiryukov
10 months ago

Replying to nicolefurlan:

I also created a docs issue (https://github.com/WordPress/Documentation-Issue-Tracker/issues/1139) so that https://developer.wordpress.org/reference/hooks/wp_create_file_in_uploads/ can get updated to match this change. Please do let me know if that was the incorrect thing to do. I'd love to know how we can coordinate with the docs team on timing to make sure the code and documentation changes happen at the same time.

Thanks! As far as I can tell, the docs issue is not necessary here, the documentation will be updated automatically as soon as 6.4 is released and the DevHub code reference parser is run as part of the post-release tasks.

Last edited 10 months ago by SergeyBiryukov (previous) (diff)

#12 @nicolefurlan
10 months ago

Oh that's good to know! Thank you @SergeyBiryukov – I'll close that docs ticket.

Note: See TracTickets for help on using tickets.