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

WebP Support #1777

Merged
merged 8 commits into from
Sep 6, 2018
Merged

WebP Support #1777

merged 8 commits into from
Sep 6, 2018

Conversation

pascalknecht
Copy link
Contributor

@pascalknecht pascalknecht commented Sep 4, 2018

New version of #1638.
Improvements:

  • skip image conversion if imagewebp is not defined (can happen on some servers)
  • skip webp tests on travis

@coveralls
Copy link

coveralls commented Sep 4, 2018

Coverage Status

Coverage decreased (-0.6%) to 93.657% when pulling 576523e on webkinder:webkinder_webp into bad57ac on timber:master.

@codecov
Copy link

codecov bot commented Sep 4, 2018

Codecov Report

Merging #1777 into master will decrease coverage by 0.65%.
The diff coverage is 3.84%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1777      +/-   ##
============================================
- Coverage     94.91%   94.26%   -0.66%     
- Complexity     1526     1535       +9     
============================================
  Files            48       49       +1     
  Lines          3581     3607      +26     
============================================
+ Hits           3399     3400       +1     
- Misses          182      207      +25
Impacted Files Coverage Δ Complexity Δ
lib/ImageHelper.php 96.18% <0%> (-1.12%) 85 <1> (+1)
lib/Image/Operation/ToWebp.php 0% <0%> (ø) 8 <8> (?)
lib/Twig.php 99.43% <100%> (ø) 83 <0> (ø) ⬇️

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 bad57ac...576523e. Read the comment docs.

@pascalknecht pascalknecht changed the title Webkinder webp WebP Support Sep 4, 2018
@pascalknecht
Copy link
Contributor Author

@jarednova What do I need to do to get this merged? It is clear that the code coverage drops if I don't run the unit tests of the WebP Class as it is not supported. Only way to support it would be to install ImageMagick and that would probably further slow down the tests. Any opinion?

@jarednova jarednova merged commit 576523e into timber:master Sep 6, 2018
@jarednova
Copy link
Member

Thanks @pascalknecht for bringing this to the finish line. I made a few small tweaks in #1780 based on what you submitted...

Removed the unlinks —. Even though this is "neater", it's helpful to manually see the files generated during the tests to further verify the correct output. If we're going to do the unlinks (which a future 🌲 god might believe) lets apply it across the test suite: PNGs, JPGs and GIFs too)
While I didn't fix the imagewebp function problem, I did update the imagick install for Travis. Did it fix anything? No. But now it's up-to-date at least
Finally, I verified all the tests locally and feel comfortable ignoring the webp stuff from code coverage
Again, thanks to you and @mhz-tamb for making this happen!

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.

None yet

4 participants