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

Replace Twig_SimpleFunction and Twig_Function with Timber_Function by extending both classes depending on Twig version #1464

Merged
merged 21 commits into from Jul 17, 2017

Conversation

@luism-s
Copy link
Contributor

luism-s commented Jun 25, 2017

Issue

#1417

tl;dr: A fatal error was thrown by activating a WPML Translation Management plugin stating that Twig_Function was expected instead of Twig_SimpleFunction.

This only happens when timber is installed via composer in the theme folder. So the cause could be that the file that was extending Twig_Function into Twig_SimpleFunction (in Twig code) was loaded after all the plugins.

Another point is that Twig_Simple_Function is getting deprecated in Twig 2.x, which Timber uses.

Solution

Search & Replace for Twig_SimpleFunction to Twig_Function.

Impact

Users should now be using Twig_Function, instead of Twig_SimpleFunction
Nothing that I'm aware of. The class named Twig_SimpleFunctionthat was extending Twig_Function was empty, so I guess there are no problems.

Usage

Use Twig_Function instead

Considerations

To protect the ones still using Twig_SimpleFunction, we could create a file with the sole purpose of creating a class named Twig_SimpleFunction and extending it from Twig_Function

Update

Due to the discussion on #1478 I had the idea to instead of using Twig_Function or Twig_SimpleFunction, make a new one called Timber_Function, which extends from each one depending on the Twig version the site is running.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 93.427% when pulling b2b3fe3 on luism-s:develop into 8f54e7c on timber:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 25, 2017

Coverage Status

Coverage remained the same at 93.427% when pulling b2b3fe3 on luism-s:develop into 8f54e7c on timber:master.

@luism-s

This comment has been minimized.

Copy link
Contributor Author

luism-s commented Jun 25, 2017

oops, Travis failed, but only on PHP5, don't know why

@jarednova

This comment has been minimized.

Copy link
Member

jarednova commented Jun 27, 2017

@luism-s thanks! I'm rerunning those PHP 5 tests, it said there was a MySQL error...

jarednova added a commit that referenced this pull request Jun 27, 2017
jarednova added a commit that referenced this pull request Jun 27, 2017
@jarednova jarednova mentioned this pull request Jun 27, 2017
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jul 7, 2017

Codecov Report

Merging #1464 into master will decrease coverage by 0.13%.
The diff coverage is 80%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1464      +/-   ##
============================================
- Coverage     93.96%   93.83%   -0.14%     
- Complexity     1415     1444      +29     
============================================
  Files            46       47       +1     
  Lines          3415     3586     +171     
============================================
+ Hits           3209     3365     +156     
- Misses          206      221      +15
Impacted Files Coverage Δ Complexity Δ
lib/Twig_Function.php 0% <0%> (ø) 0 <0> (?)
lib/Twig.php 96.98% <100%> (+3.61%) 49 <0> (ø) ⬇️
lib/FunctionWrapper.php 91.17% <100%> (+0.26%) 17 <1> (ø) ⬇️
lib/Admin.php 92.3% <0%> (-3.7%) 11% <0%> (ø)
lib/MenuItem.php 70.87% <0%> (-2.54%) 49% <0%> (ø)
lib/Menu.php 86.81% <0%> (-1.56%) 44% <0%> (ø)
lib/LocationManager.php 98.57% <0%> (-1.43%) 25% <0%> (ø)
lib/Image/Operation/Resize.php 97.14% <0%> (-0.92%) 30% <0%> (ø)
lib/Term.php 95.89% <0%> (-0.59%) 57% <0%> (ø)
lib/Loader.php 95.12% <0%> (-0.51%) 69% <0%> (ø)
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 642130e...0e4ea6c. Read the comment docs.

@jarednova jarednova mentioned this pull request Jul 7, 2017
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 7, 2017

Coverage Status

Coverage remained the same at 93.427% when pulling bc30442 on luism-s:develop into 642130e on timber:master.

1 similar comment
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 7, 2017

Coverage Status

Coverage remained the same at 93.427% when pulling bc30442 on luism-s:develop into 642130e on timber:master.

luism-s added 10 commits Jul 7, 2017
…ile this is a wonderful future, that's only about 12% of WP users. So I want to be future-friendly while supporting the other ~88% of users who are on 5.x." bah not fun
…ending on Twig version
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 7, 2017

Coverage Status

Coverage increased (+0.1%) to 93.56% when pulling 7917d09 on luism-s:develop into 642130e on timber:master.

@luism-s

This comment has been minimized.

Copy link
Contributor Author

luism-s commented Jul 7, 2017

@jarednova I made it. Only codecov is failing due to the large number of lines I changed on that file. I covered some lines of code with unit tests, but there's still some to go.

What do you think of this solution?

@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@timber timber deleted a comment from coveralls Jul 8, 2017
@jarednova

This comment has been minimized.

Copy link
Member

jarednova commented Jul 17, 2017

@luism-s thanks for updating this! Glad to see it works (my confusion on #1478). My only question is whether the naming makes sense. Right now its full path is...

Timber\Timber_Function

... because the functionality is really about exposing functions to Twig, I'm wondering if Timber\Twig_Function might make more sense?

@luism-s

This comment has been minimized.

Copy link
Contributor Author

luism-s commented Jul 17, 2017

Hm, OK, I really don't mind your name idea. I can do it later ;)

@luism-s

This comment has been minimized.

Copy link
Contributor Author

luism-s commented Jul 17, 2017

@jarednova Renaming the function to Twig_Function made the tests in PHP5 fail. Maybe it's a conflict with Twig's Twig_Function? I don'know what I'm doing wrong, honestly

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage increased (+0.3%) to 93.688% when pulling 662e090 on luism-s:develop into 642130e on timber:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage increased (+0.3%) to 93.688% when pulling 1780137 on luism-s:develop into 642130e on timber:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage increased (+0.3%) to 93.688% when pulling 0e4ea6c on luism-s:develop into 642130e on timber:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 17, 2017

Coverage Status

Coverage increased (+0.3%) to 93.688% when pulling 0e4ea6c on luism-s:develop into 642130e on timber:master.

@luism-s

This comment has been minimized.

Copy link
Contributor Author

luism-s commented Jul 17, 2017

Ok I figured out the problem. Good news, all tests checked 👌 ready to merge

@jarednova

This comment has been minimized.

Copy link
Member

jarednova commented Jul 17, 2017

🎉 Hooray 🎉 ! Thanks so much @luism-s — wouldn't have thought that one would be so tough, but it really helps smooth out the Twig 2.0 integration. Thanks for all your work!!!

@jarednova jarednova merged commit d9c6f81 into timber:master Jul 17, 2017
3 of 5 checks passed
3 of 5 checks passed
codecov/patch 80% of diff hit (target 93.96%)
Details
codecov/project 93.83% (-0.14%) compared to 642130e
Details
Scrutinizer 6 new issues, 1 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 93.688%
Details
@luism-s

This comment has been minimized.

Copy link
Contributor Author

luism-s commented Jul 17, 2017

All I wanted was really to be able to update Timber to the latest version, now I can :) When are you releasing the next release (pleonasm unintended)?

@jarednova

This comment has been minimized.

Copy link
Member

jarednova commented Jul 17, 2017

@luism-s I like to do those on Tuesdays (my normal night for working late) — so look out for it tomorrow!

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

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.