2.x Fix theme images generation (using Twig filters and such)

[Moustachos]

Ticket: #2453

Issue

Without this fix, if your theme's tree structure was (2.x default child theme structure):
-- my-theme-name/
--- images/
---- foo.jpg
--- theme/
---- functions.php
---- style.css

You weren't able to use Twig's image filters (like |resize) on your theme's images, because ImageHelper::theme_url_to_dir() failed to replace theme uri in image uri.

It was trying to replace my-theme-name/theme in passed uri, but my-theme-name/theme isn't the root directory of your theme, it must be my-theme-name.

Trying to use any Twig image filter (like |resize) on a theme image resulted in Timber errors like this one:
[25-May-2021 14:20:37 UTC] [ Timber ] Error loading /[...]/themes/my-theme-name/themehttps://[...]/themes/my-theme-name/build/images/my-image.png

And the resized image was never generated.

Solution

Add a new ThemeHelper class, wrapping WordPress's default get_stylesheet() / ... methods, keeping their initial behavior and just removing the /theme (/controllers or whatever comes after the /) from theme's name.

That way, when you call ThemeHelper::get_stylesheet_directory_uri(), you'll be sure that returned uri will be your theme's root directory uri, not my-theme-name/theme.

Impact

No breaking changes.

Usage Changes

When you want to retrieve theme's name / directory / directory uri and don't bother about the potential /theme suffix: use regular WordPress get_stylesheet() / ... methods.

When you want to retrieve theme's name and potentially use it with file system (based on the principle that theme's name should be matching theme's root directory in tree structure): use ThemeHelper::get_stylesheet() / ... methods.

Considerations

I didn't make any change to Theme class & LocationManager classes, because I wasn't sure if my fix was relevant there or if it would potentially break stuff.

Testing

No unit tests needed I guess, however I made changes to unit tests using WordPress's default get_stylesheet() / ... methods (like test/test-timber-image.php).

[Moustachos]

@gchtr any news on this one? :)

[jarednova]

Thank you @Moustachos ! At a (quick) glance this seems 💯 . Thanks for thinking through the code structure (making it a class that mirrors other helpers) and the backwards compat issues.

As you've no doubt seen, we're in the heat of closing out some other very big PRs related to the meta structure of 2.0, so I'm labeling this as a "2.x Future" item for now. Once those are set, we can turn our attention to a more thorough review and merge of what you've prepared here.

[Moustachos]

@jarednova thanks for the update 👍

[mcaskill]

I've tested this PR in a project that uses Timber v2 and it works very well.

Would be nice to merge these changes.

[Moustachos]

Thanks for your feedback @mcaskill!

I've been using this code on many websites ever since I posted this PR and found no issue so far.

[gchtr]

Okay, I see that we need this if it it concerns images in the default 2.x starter theme.

I already started fixing the merge conflicts and will check whether we can add some tests that confirm the fixes. You can expect this to be merged soon.

[gchtr]

I thought about this change again, especially after @nlemoine’s comment in #2716 (review). And I have to agree with him. I think it’s probably better if we don’t merge the changes of this pull request.

I created a draft pull request in timber/starter-theme#139 to revert the changes made to the default theme structure.

Having a nice folder structure with WordPress template files in a theme/ subfolder is cool. But if there are issues like these, a lot of other issues might follow. Do we really want to work around these issues? Is the benefit of having a nicer file structure bigger than having to manage these various issues? If we shouldn’t use get_stylesheet_directory() or get_template_directory() directly in Timber and use a wrapper function and also advise developers to do so, then that probably only overcomplicates things.

With Timber, we’re trying more and more to have a good base that allows you to hook other dependencies into it and not be too opinionated about certain things. So if you still wanted to use that custom folder structure, we could add filters in the appropriate places in Timber core that would allow you to fix these issues themselves. But we probably shouldn’t handle this case in Timber core itself.

A lot of work and efforts already went into this and I’m very sorry if we can’t merge this in. But sometimes we have to try a thing to see how well it works out.

Before I’m going to close this, I wanted to ask: What are you all thinking about this?

[Moustachos]

Well @gchtr & @nlemoine, the thing is that this change also works with standard file structure.

So I don't really see what would be the issue with merging in Timber core instead of starter theme.

Since image generation is handled in core, how do you plan on implementing this in starter theme?

About the overcomplicated part:

• if you're using standard file structure: you can just use get_stylesheet_directory(), get_template_directory()
• if you're using 2.x starter theme file structure: all you have to do is calling the same methods in ThemeHelper

It's not that hard IMO, what do you think?

[gchtr]

The thing is that this change also works with standard file structure.

Yes it does. But it only does if you decide that your images live in the main folder of your theme. It doesn’t work anymore if you decide to put your image files into the theme subfolder. I don’t want to force developers to use a theme structure, they should be free to choose.

What if you placed your images folder inside the theme subfolder instead? With that, we wouldn’t need a ThemeHelper class at all.

Since image generation is handled in core, how do you plan on implementing this in starter theme?

We wouldn’t implement this in the starter theme, either. In timber/starter-theme#139, we would revert putting the theme files in a theme subfolder. There would be no theme subfolder. So there would be no special case to handle in Timber core.

If you still wanted to use a theme subfolder, we’re happy to accept a pull request with new PHP filters that allow you to use your ThemeHelper class as a custom extension. For example, we could add a timber/theme_url_to_dir filter that allows you to filter the result of Timber\ImageHelper::theme_url_to_dir(). You could even make this available as a Composer package, so others could use it.

We don’t say you can’t use this, we just say we wouldn’t want to handle this in Timber core.

About the overcomplicated part:

• if you're using standard file structure: you can just use get_stylesheet_directory(), get_template_directory()
• if you're using 2.x starter theme file structure: all you have to do is calling the same methods in ImageHelper

It's not that hard IMO, what do you think?

We’ve learned in the last couple of years that it’s never a case of "you can just use" or "all you have to do". The problem here is that:

• We would always have to think about whether to use the helper methods or the get_stylesheet_directory() or get_template_directory() functions directly in Timber core, whenever we need a theme path.
• We would have to explain the difference clearly in the documentation and when to use what.
• We would have to inform developers about the change.
• For future updates, we would always have to carry that use case with us.

Not to mention all the issues with a custom theme folder structure that we didn’t discover yet. What if some functionality relies on the subfolder being in the path?

Does it work with plugins that allow you to override template files in your theme, like WooCommerce or The Events Calendar? If it doesn’t work out of the box, we would have to throw more code at Timber and possibly create exceptions for different integrations. This quickly leads to a lot of problems. Since you’d put your theme-related files into a theme subfolder, shouldn’t the system expect that all theme-related files live there?

There would definitely be questions about this and new issues created, because some people might not find the relevant documentation for this. It can lead to confusion.

We’ve had lots of issues with paths and third-party compatibility in the past. Adding another level of helper methods probably isn’t helping us in the future, unfortunately.

One reason why it’s taking us so long for Timber v2 to be released is because of a lot of edge cases that were once brought into Timber and which we now have to carefully remove again, because they are just making things more complicated and are the source of unexpected behavior.

I think that this could become one of those cases.

[nlemoine]

Thanks @gchtr for the in depth analysis and very detailed explanation! I totally agree with all this. A "nice to have" feature (theme structure) shouldn't come with so much drawbacks/adaptations while it targets a very few percentage of Timber users (although this percentage might increase if the starter theme offers this structure as the default one).

I also like the idea of not having all those WP template files at the root of my themes. But as @szepeviktor mentioned here, this can also be achieved with the awesome Hierarchy package. I'm using it since quite a long time now and it works great but I don't think it should belong in core. We could maybe add something in the documentation if one would like to go that way.

[szepeviktor]

while it targets a very few percentage of Timber users

@nlemoine Do you suggest that Timber users don't use Composer to install and manage their WordPress installation?
Isn't space age here yet??

[nlemoine]

Do you suggest that Timber users don't use Composer to install and manage their WordPress installation?
Isn't space age here yet??

@szepeviktor I was referring to the theme structure merged here. I've been managing WordPress with composer since 1957. I would never state something like this.

Don't read me wrong, I'm all for having a better theme structure. But this is not the right way to do it and I was just pointing out this can be achieved with https://github.com/Brain-WP/Hierarchy (which I'm using and contributed to).

if you're using standard file structure: you can just use get_stylesheet_directory(), get_template_directory()
if you're using 2.x starter theme file structure: all you have to do is calling the same methods in ThemeHelper

☝️ This looks very very obviously wrong to me.

[szepeviktor]

I have to admit that I'm quiet detached from daily development but I think while developing with Timber all you need is a function that returns the URL of the assets directory as every downloadable file is in that directory.

function get_asset_url(string $path = ''): string
{
    return sprintf('%s/assets%s', dirname(\get_template_directory_uri()), $path);
}

[mcaskill]

The Hierarchy package was what I was using in my last WP project which I thought worked very well and would probably be what I would use in my next project. For example, I'm not a big fan of having a bunch of PHP template files that just serve as proxies to render the Twig files. I would just have an index.php file that calls Hierarchy pass the resolved templates to Timber.

I'm all for adding the necessary missing filters to mutate custom paths for the image generation.

In all our projects, including our recent Timber projects, we usually have a get_asset_url() like function, as @szepeviktor shared.

[nlemoine]

I would just have an index.php file that calls Hierarchy pass the resolved templates to Timber.

You don't even need an index.php file if you hook in template_redirect.

[szepeviktor]

PHP template files that just serve as proxies

Yes, they are really just proxies. Reverting route to template file matching.

[acobster]

I am also fairly detached from WP development these days, but I feel pretty strongly that libraries like Timber should be as unopinionated as possible about things like directory structure, especially when the gain is a nice-to-have.

We’ve learned in the last couple of years that it’s never a case of "you can just use" or "all you have to do".

This is the right take IMO. This PR is a cool idea, but the mental burden it imposes on developers is still a burden, however small. You shouldn't have to play computer in your head to know which API to call. Whether or not to use an API should be an all-or-nothing thing whenever possible, i.e. when you decide to adopt a library, you decide to "buy in" to its API. This should live in an abstraction where the boundary is clear - install the dep, and always use the wrapper.

I do hope this work successfully lands in its own abstraction! Thanks all.

[Moustachos]

I get your point about the fact that image generation should work out of the box without having to call specific methods depending on your file structure.

However, I didn't think this PR would end up being closed after almost 2 years pending, because 2.x starter theme file structure isn't standard...

I guess two filters in theme_url_to_dir() on $site_root & $tmp variables would do the trick.

[gchtr]

Thanks for all your feedback on this.

What I’m getting is that for the future we can add the relevant filters to change the file paths. If somebody could open a pull request for this, I’d be happy to look at it.

We can also add documentation for how to use Brain-WP/Hierarchy in a Timber theme. This library has come up a few times in the past. I’d also be happy if someone could take a stab at this, because I don’t use it myself and have no experience with it.

@Moustachos I’m sorry it took us so long to come to that conclusion, especially because this was your first contribution to Timber. I see that this can feel disheartening. But I strongly feel that the discussion this pull request has sparked will only make Timber better.

I’m going to close this, now.