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

Generate WebP in liip:imagine:cache:resolve CLI command and async resolve cache messages #1347

Merged
merged 9 commits into from
Oct 16, 2021

Conversation

peter-gribanov
Copy link
Contributor

@peter-gribanov peter-gribanov commented Jan 13, 2021

Q A
Branch? 2.0
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #1319
License MIT
Doc PR

The liip:imagine:cache:resolve CLI command warms up the cache for the specified image sources with all or specified filters applied, and prints the list of cache files. I forgot that this command should also create a image in WebP format if WebP generation is enabled.

  • Add new mthod FilterService::warmsUpCache();
  • The mthod FilterService::bustCache() now return result of busting;
  • The liip:imagine:cache:resolve command now warms up image in WebP format too;
  • The liip:imagine:cache:remove command now remove image in WebP format too;
  • The ResolveCacheProcessor wat async warms up cache now warmed up image in WebP format too;
  • Add unit tests for FilterPathContainer;
  • Add unit tests for FilterService.

@coveralls
Copy link

coveralls commented Jan 13, 2021

Coverage Status

Coverage increased (+2.3%) to 86.295% when pulling 4c3e68f on peter-gribanov:cache_resolve_command into 7282ab2 on liip:2.x.

@peter-gribanov
Copy link
Contributor Author

Build failed because for PHP 8 used PHPUnit 9 what is not compatible with current tests. See fix #1381

@peter-gribanov peter-gribanov marked this pull request as draft June 11, 2021 09:11
@peter-gribanov peter-gribanov force-pushed the cache_resolve_command branch 4 times, most recently from d8c6827 to f6293f0 Compare June 11, 2021 11:11
@peter-gribanov peter-gribanov marked this pull request as ready for review June 11, 2021 11:22
@peter-gribanov peter-gribanov marked this pull request as draft June 11, 2021 11:22
@peter-gribanov peter-gribanov force-pushed the cache_resolve_command branch from fae75c0 to 67ccad5 Compare June 11, 2021 14:05
@peter-gribanov peter-gribanov marked this pull request as ready for review June 11, 2021 14:08
@peter-gribanov peter-gribanov changed the title Generate WebP in liip:imagine:cache:resolve CLI command Generate WebP in liip:imagine:cache:resolve CLI command and async resolve cache messages Jun 11, 2021
@peter-gribanov
Copy link
Contributor Author

@lsmith77 it would be nice to merge this PR. It fixes some bugs not implemented in #1307, adds unit tests and adds a cache warming function. If this PR is too large, then i can split it into several PRs.

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

sorry for the late reply. this looks good to me, and a nice refactoring to move the warmup into the filter service.

i have some nitpicks, can you please have a look?

@dbu dbu mentioned this pull request Oct 5, 2021
@dbu
Copy link
Member

dbu commented Oct 5, 2021

if #1360 is merged first, we need to adjust the logic in the symfony messenger processor to follow the changes done here.

@peter-gribanov
Copy link
Contributor Author

@dbu renamed the method to warmUpCache.

@peter-gribanov
Copy link
Contributor Author

The order of the merger is not critical. I can make changes to my PR after #1360 merge.

Copy link
Member

@dbu dbu left a comment

Choose a reason for hiding this comment

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

thanks a lot! i really like that we move from talking about "resolving" to "warmup" 👍

#1360 won the race ;-) can you please rebase this branch on 2.x and apply your refactoring to WarmupCacheHandler?

i am worried a bit by how much mocking we are doing in one of the unit tests. but if you are happy with it, we can leave it like that, just want to ask the question.

/**
* @covers \Liip\ImagineBundle\Service\FilterService
*/
final class FilterServiceTest extends TestCase
Copy link
Member

Choose a reason for hiding this comment

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

there is a LOT of mocking going on in this test. i wonder if we could not move towards a more functional test that uses the real managers instead of mocking. if tests fail, it will be a bit harder to know where the mistake was - but if they succeed, they create more trust that things actually work as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that it would be great to supplement the unit tests with functional tests, but unfortunately i don't have time to do this 😕

@peter-gribanov
Copy link
Contributor Author

#1360 won the race ;-) can you please rebase this branch on 2.x and apply your refactoring to WarmupCacheHandler?

done

@dbu dbu merged commit 018394c into liip:2.x Oct 16, 2021
@dbu
Copy link
Member

dbu commented Oct 16, 2021

thanks a lot!

i will look into creating a new minor release next week.

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.

4 participants