#2192 - Allow users to filter Timber default Twig functions

[nlemoine]

Hello!

Sorry about the PR flood. I have some spare time so I can finally propose changes I've been thinking/waiting for a long time.

Issue

See #2192

Solution

Provide a filter to remove some functions/filters. (PR is still a draft, I'll implement the same logic for filters if accepted).

Impact

None. Obviously, if you are removing some default functions/filters, you are fully aware of consequences.

Usage Changes

None.

Considerations

Unfortunately, Twig does not support functions/filters removal once added.

Testing

Not yet, I will if/when accepted.

Side note

This PR also makes use of native Twig deprecated option.

[gchtr]

I like this a lot for a solution to the underlying problem.

Sorry about the PR flood. I have some spare time so I can finally propose changes I've been thinking/waiting for a long time.

Keep them coming ❤️! Your suggestions are very valuable!

This PR also makes use of native Twig deprecated option.

Nice! Good to know this option exists.

[nlemoine]

Keep them coming ❤️! Your suggestions are very valuable!

Thanks a lot! 😊 I'm a daily Timber user and I thought some improvements I made over projects could benefit to other users and improve the existing code base.

Nice! Good to know this option exists.

I'm not an expert at error handling but it may replace actual deprecations functions?

Helper::deprecated('{{ TimberPost() }}', '{{ get_post() }} or {{ get_posts() }}', '2.0.0');

[gchtr]

I'm not an expert at error handling but it may replace actual deprecations functions?

Helper::deprecated('{{ TimberPost() }}', '{{ get_post() }} or {{ get_posts() }}', '2.0.0');

How does the error message look like? Have you tried it out?

The Helper::deprecated() method plugs in with the @expectedDeprecated tags in test DocBlocks, for example here:

timber/tests/test-timber-twig-objects.php

Lines 15 to 17 in 922bab4

	/**
	* @expectedDeprecated {{ TimberImage() }}
	*/

. This makes it possible for us test whether functions also appear as deprecated but still work as expected.

I guess that is the only critical thing to consider here.

As I read in https://twig.symfony.com/doc/2.x/recipes.html#displaying-deprecation-notices, to make this work we would maybe have to install another package. But that would make it even more complex.

[gchtr]

Thanks a lot! 😊 I'm a daily Timber user and I thought some improvements I made over projects could benefit to other users and improve the existing code base.

I think that’s why we are all here, after all 😊.

[nlemoine]

@gchtr PR updated! I've not included #2284, this can be done a bit later.

[jarednova]

This is looking really good @nlemoine! (and keep the flood of PRs coming!) Given the amount here that's touching Twig (and Timber) internals, we'll want to make sure this runs through C-I tests before we merge (at the moment our Travis plan + integration are down).

@gchtr I concur with your review notes. Please let me know when this has your ✅ and we can see where we are on the C-I front at that point.

[jarednova]

This gets my vote. I made just one change to cover for a missing get_users. Since this touches a lot of stuff that @acobster and @gchtr are both active on I'd like one of their reviews as well before we merge

[gchtr]

This looks good to me 👍.

I’d leave it a that state and merge it in (@jarednova, I’ll let you do the honors).

I’ll add further iterations in #2419.

[acobster]

This looks great! I didn't run them but the tests look good to me.