Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update plugin to add twig functions in the way Timber 2 expects #11

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sdunham
Copy link

@sdunham sdunham commented Nov 21, 2024

PR title is pretty self-explanatory, but this plugin isn't compatible with Timber 2. It adds a custom Twig function in the way Timber 1 expects. This update migrates to adding this function in the way Timber 2 expects.

A couple of notes:

  • If possible, we should update sites which are still using this plugin to use the Sitka WP plugin, since this plugin appears to be a precursor to that. I'm making this update to ensure we have another option in case that's not feasible.
  • This should be released a v 2.0.0 since this is a breaking change
  • Before we merge this update to main, we should do one more check/survey to make sure there aren't any sites that are still using this plugin but have set up their composer file to use dev-master. As far as I can tell, that's only the AWB site (which sparked this update since it's being update to Timber 2) and Metro Parks. I've opened a PR to update MP's ompiser file to explicitly use v1.0 of the plugin.

@sdunham sdunham requested a review from akoziolsc November 21, 2024 01:17
@sdunham
Copy link
Author

sdunham commented Nov 21, 2024

@akoziolsc FYI I added you as a reviewer for this PR for now because I can't add Ryan yet (apparently he's not part of our organization yet). If you have time to review, great. If not, I'll switch it to Ryan once he's part of our organization.

));

return $twig;
add_filter('timber/twig/functions', function ($functions) {

Choose a reason for hiding this comment

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

@sdunham I think, you could use timber/twig filter instead and make it compatible across the board: https://timber.github.io/docs/v2/hooks/filters/#timber/twig
This would require us to check to make sure proper Twig related classes exist but it should be doable. This would be one way to ensure it will not cause issues with other older projects. Not requirement but suggestion.

Copy link
Author

Choose a reason for hiding this comment

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

Great suggestion, I went ahead and implemented it. I still want to make sure any sites still using this plugin specify a version instead of a branch, and work on migrating those sites to the Sitka plugin at some point in the future, but this will provide maximum backward-compatibility. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants