Make WordPress Core

Opened 20 months ago

Closed 19 months ago

Last modified 19 months ago

#56180 closed feature request (fixed)

get_header_image(): may consider adding filter in 'get_header_image()'

Reported by: hztyfoon's profile hztyfoon Owned by: audrasjb's profile audrasjb
Milestone: 6.1 Priority: normal
Severity: normal Version: 2.1
Component: Themes Keywords: has-patch has-unit-tests has-testing-info commit needs-dev-note
Focuses: administration Cc:

Description

Hey everyone, 🙂

I think we can add a filter hook in 'header_image' function in 'wp-includes/theme.php' file.

Original Idea by: @audrasjb.

Attachments (1)

56180.1.diff (2.3 KB) - added by costdev 20 months ago.
Add filter to get_header_image(). Includes unit tests.

Download all attachments as: .zip

Change History (28)

This ticket was mentioned in PR #2958 on WordPress/wordpress-develop by hz-tyfoon.


20 months ago
#1

  • Keywords has-patch added

Trac ticket: 56180

#2 @audrasjb
20 months ago

Related: #56158

(see comment 3)

#3 @hztyfoon
20 months ago

I just thought I should mention my good friend @manzurahammed as he helped me with the patch. :)

#4 @audrasjb
20 months ago

Looks good to me. I added a commit to your PR to address a few WPCS issues.

#5 @hztyfoon
20 months ago

Thanks @audrasjb a lot for fixing those typos. 🤗
Looking pretty good to me.

#6 @audrasjb
20 months ago

  • Milestone changed from Awaiting Review to 6.1
  • Owner set to audrasjb
  • Status changed from new to reviewing

#10 @mukesh27
20 months ago

  • Keywords needs-unit-tests added

Hi there!

Thanks for the ticket and patch.

The approach looks good to me. Can you please add a Unit test that covers the new filter header_image?

#11 follow-up: @SergeyBiryukov
20 months ago

It looks like the proposed filter would affect header_image(), but not get_header_image(), which may be confusing.

Should this be a filter on the result of get_header_image() instead? That way it would work for both functions.

See the get_header_image_tag and get_header_video_url filters for similar examples.

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

#12 @SergeyBiryukov
20 months ago

I'm also not sure that "Displays" should be changed to "Prints" as part of this PR, it would be inconsistent with other similar descriptions:

 * Displays localized stylesheet link element.
 * Displays the custom header text color in 3- or 6-digit hexadecimal form (minus the hash symbol).
 * Displays the image markup for a custom header image.
 * Displays header image URL.
 * Displays header video URL.
 * Displays background image path.
 * Displays background color value.

For some of these, "Prints" might indeed be more appropriate than "Displays", but I would suggest changing that in a separate ticket.

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

#13 in reply to: ↑ 11 @audrasjb
20 months ago

Replying to SergeyBiryukov:

It looks like the proposed filter would affect header_image(), but not get_header_image(), which may be confusing.

Should this be a filter on the result of get_header_image() instead? That way it would work for both functions.

Great point!

#14 @ironprogrammer
20 months ago

  • Keywords needs-testing-info needs-testing added

#15 @costdev
20 months ago

I agree on both points @SergeyBiryukov - the filter should be in get_header_image() instead, and the "Displays" > "Prints" change is outside the scope of this ticket and should be removed from the PR.

@costdev
20 months ago

Add filter to get_header_image(). Includes unit tests.

#16 @costdev
20 months ago

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

Testing Instructions

Steps to Test

  1. Add the following code to functions.php:
    <?php
    
    add_filter( 'header_image', 'my_custom_header_image' );
    function my_custom_header_image( $header_image ) {
            return 'http://example.org/my-custom-header-image.jpg';
    }
    
    add_action( 'admin_notices', 'my_custom_header_image_notice' );
    function my_custom_header_image_notice() {
            echo '<div class="notice notice-info"><p>'. get_header_image() . '</p></div>';
    }
    
  2. Navigate to the Dashboard.

Expected Results

  • A notice should appear containing the following: http://example.org/my-custom-header-image.jpg

#17 @hztyfoon
20 months ago

  • Summary changed from header_image(): may consider adding filter in 'header_image()' to get_header_image(): may consider adding filter in 'get_header_image()'

⬆️ changing the summery from 'header_image' to 'get_header_image'.

Thanks a ton @SergeyBiryukov for those 2 points. 👍
I also agree with both of them. Will try to update the PR accordingly.

Also, thanks @costdev. I've took a look at your .diff & you've also added unit_test there. great work.

hz-tyfoon commented on PR #2958:


20 months ago
#18

Just updated the PR.
removed the filter from header_image & added a filter in get_header_image.

Looks like it may make more sense to add the filter to get_header_image function as this way it'll work for both header_image & get_header_image (as inside header_image() there's only get_header_image() and then just printing the image url ).

Thanks @SergeyBiryukov for pointing this out.

#19 @hztyfoon
20 months ago

Updated the PR. Would you all be kind enough to check. 🙂

#20 @audrasjb
20 months ago

Thanks for updating the PR but it looks like it doesn't include the Unit Tests proposed by @costdev in 56180.1.diff. It would be useful if you can update your PR to implement those unit tests, but if you can't, it's probably better to close the PR in favor of 56180.1.diff :)

#21 @hztyfoon
20 months ago

Ok, closed the PR in favor of 56180.1.diff by @costdev as it already includes unit tests.

Just a minor change suggestion: Wouldn't the hook_name get_header_image make more sense than header_image ?

Last edited 20 months ago by hztyfoon (previous) (diff)

#22 @audrasjb
19 months ago

Indeed, so it's more consistent with the other get_header_image_tag_attributes filter.
PR incoming.

#24 @audrasjb
19 months ago

  • Keywords commit added; needs-testing removed

Tested using the following snippet, works fine:

add_filter( 'get_header_image', function() {
	return 'https://example.com/image.jpg'; // Replace this with the image of your choice.
} );

Adding needs-dev-note keyword.
Marking for commit.

#25 @audrasjb
19 months ago

  • Keywords needs-dev-note added

#26 @audrasjb
19 months ago

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

In 53741:

Themes: Add a hook to filter theme header image URL.

This changeset introduces the get_header_image filter, which can be used to modify header image URL returned by get_header_image(), in themes that support the Header Image feature.

Props hztyfoon, audrasjb, mukesh27, SergeyBiryukov, costdev.
Fixes #56180.

Note: See TracTickets for help on using tickets.