Skip to content
This repository was archived by the owner on Jan 26, 2022. It is now read-only.

Conversation

@kjellr
Copy link
Collaborator

@kjellr kjellr commented Nov 12, 2021

While trying out some child themes for Twenty Twenty-Two, I discovered that the flying bird header image used in the theme's header-large-dark.html template shows a 404 when its used in a child theme. It's being pulled from a pattern that references the image via get_stylesheet_directory_uri():

<figure class="wp-block-image alignfull size-full"><img src="' . esc_url( get_stylesheet_directory_uri() ) . '/assets/images/flight-path-on-transparent-c.png" alt="' . esc_attr__( 'Illustration of a bird flying.', 'twentytwentytwo' ) . '"/></figure>

If we switch to get_template_directory_uri(), it works again. This isn't usually necessary (all of the other patterns work fine), but I think this generally makes sense, since this pattern only exists so that it can be used in a template.

@kjellr kjellr added the [Type] Bug Something isn't working label Nov 12, 2021
@kjellr kjellr requested a review from jffng November 12, 2021 17:57
@kjellr kjellr self-assigned this Nov 12, 2021
<!-- wp:image {"align":"full","sizeSlug":"full","linkDestination":"none"} -->
<figure class="wp-block-image alignfull size-full"><img src="' . esc_url( get_stylesheet_directory_uri() ) . '/assets/images/flight-path-on-transparent-c.png" alt="' . esc_attr__( 'Illustration of a bird flying.', 'twentytwentytwo' ) . '"/></figure>
<figure class="wp-block-image alignfull size-full"><img src="' . esc_url( get_template_directory_uri() ) . '/assets/images/flight-path-on-transparent-c.png" alt="' . esc_attr__( 'Illustration of a bird flying.', 'twentytwentytwo' ) . '"/></figure>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the other patterns don't break, but can you think of any other patterns where it would make sense to update the image path to use get_template_directory_uri?

Maybe it makes sense to update all of the dynamic URLs to use get_template_directory_uri, to be safe.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not entirely sure. Twenty Twenty-Two uses stylesheet_directory_uri so I think that is technically a fine way to do it? I'd be interested in some insight from other theme devs too.

@justintadlock
Copy link

I would use get_theme_file_uri() for any image references. That way, child themes could actually overwrite the image while falling back to the template directory.

https://developer.wordpress.org/reference/functions/get_theme_file_uri/

@kjellr
Copy link
Collaborator Author

kjellr commented Dec 3, 2021

I like that suggestion, thanks @justintadlock. It doesn't seem to fix the template issue though. When I switch to that, I still see a broken header image for our templates.

Also, I took a closer look and we've been were inconsistent in the way we've been referencing images for patterns — I took a closer look and some were already using get_template_directory_uri(), while most were using get_stylesheet_directory_uri(). 😕 Might be worth switching everything except for these template-centric patterns to get_theme_file_uri() in a separate PR.

@kjellr
Copy link
Collaborator Author

kjellr commented Dec 8, 2021

Closing in favor of #283.

@kjellr kjellr closed this Dec 8, 2021
@kjellr kjellr deleted the try/use-template-directory-in-pattern branch December 8, 2021 15:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

[Type] Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants