Resolve image caches in background. Using message queue.

[makasim]

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

The PR adds an ability to resolve image caches (apply filters and so on) in background process.
Right now if the new image is uploaded the cache images are not created. There is a controller for that and at first request the cache image is created and stored to the cache. The approach works but has some disadvantages.

1. The first request to the cache takes more time ( we have to read the original image, apply filters and upload the filtered image to the cache.
2. Varnish may cache the controller pass and not the actual cache image.

This PR adds optional dependency on enqueue/enqueue-bundle and allows to process the images once they are uploaded.

Here's how you to use this:

1. Install the EnqueueBundle
2. Enable integration in LiipImagineBundle

liip_imagine:
    enqueue: true
    # other options

3. Send a message once the image is uploaded

<?php
use Liip\ImagineBundle\Async\Topics;
use Liip\ImagineBundle\Async\ResolveCache;

/** @var ProducerInterface $producer */
$producer = $this->get('enqueue.producer');

// resolve all caches 
$producer->send(Topics::RESOLVE_CACHE, new ResolveCache('the/path/img.png'));

// resolve specific cache
$producer->send(Topics::RESOLVE_CACHE, new ResolveCache('the/path/img.png', ['fooFilter']));

// force resolve (removes the cache if exists)
$producer->send(Topics::RESOLVE_CACHE, new ResolveCache('the/path/img.png', null, true));

4. Run the consumer

You can run many instances to improve performance.

$ ./app/console enqueue:consume --setup-broker -vvv

This is a RFC. Tests and doc will be added if the feature is accepted. There are other processors may be added as well, to delete cache or to cache a single filter.

[cedricziel]

I like the idea of producing images asynchronously. - In fact I would like to see it added. The container makes it a pretty easy to just plug it in. +1 from my side.

[robfrawley]

I love the intention of this PR, but I hate adding more "optional dependencies" that require "manual configuration". Is there a library, instead of a bundle, that we can use, so we're only adding an "optional dependency" and not also "required manual configuration" needed to set up the extra bundle? This isn't a deal breaker for me; just interested if there are any zero-configuration options.

[makasim]

Is there a library, instead of a bundle, that we can use

Yes there is the library and the bundle does not do much, it is only a glue layer over the library stuff. Everything could be used in plain php, even consumers though it would require a bit more work to setup

that require "manual configuration"

There is no much we can do with it. Enqueue supports different transport (AMQP, Stomp, Redis, Amazon SQS) they all require different configuration options. It does not make much difference whether we use library or the bundle we still have to do that

[robfrawley]

Fair enough; I'm onboard then. After some tests and docs are added (which, as you pointed out, you were waiting on to ensure you didn't waste your time) this looks like a fairly simple addition in terms of added code complexity, yet brings with it a very cool and much-requested functionality.

It appears as though php-enqueue/enqueue-bundle is primarily maintained by yourself, so I would only want to ensure you intend to regularly update the bundle, especially as it applies to the upcoming 2.x release, which will likely support Symfony 4.x.

Otherwise, his looks like an excellent addition @makasim.

[robfrawley]

Wouldn't it make more sense for this pull request to be against the 2.x branch due to the really high requirement of PHP 5.6 for the external php-enqueue/enqueue-bundle dependency (versus the 5.3 support advertised for Imagine Bundle 1.x)?

I know flyststem already require PHP 5.4 but no other optional dependencies require anything higher just yet. What do you think of merging this into 2.x?

[makasim]

@robfrawley In general I dont mind merging it to 2.x though I tried to tweak travis script to install enqueue only on 5.6 or higher PHP versions and the tests marked skipped if no enqueue installed (the case for PHP 5.3).

Also composer helps here a lot. It would not allow a developer to install enqueue on old PHP version preventing them from breaking things.

[robfrawley]

If merged in 1.x these need to be resolved FQCNs for merge into 1.x but is fine if we merge into 2.x.

[robfrawley]

If merged in 1.x these need to be resolved FQCNs for merge into 1.x but is fine if we merge into 2.x.

[robfrawley]

All the short array syntaxes need to go, as well if you want to merge this against 1.x. Not the case if you chose 2.x instead.

[robfrawley]

I would use the parent class to handle this versus throwing this exception yourself (this is also how we handle it in other test cases within this bundle):

if (false == interface_exists('Enqueue\Psr\PsrProcessor')) {
  $this->markTestSkipped('The enqueue bundle is not installed. Skip optional tests');
}

[robfrawley]

Even though you are marking the test as skipped, the lower version PHP engines are still going to perform a compilation step on the test classes, so they need to run on PHP 5.3 if you want to merge into the 1.x branch. Otherwise, we can leave as-is and merge into the 2.x branch without issue.

I don't have a strong preference one way or the other (between using 1.x or 2.x as the base branch for this PR), just wanted to bring up the dependency discrepancy between imagine-bundle itself and your enqueue bundle and see what consensus was reached.

[makasim]

@robfrawley Ready for review. rebased and squashed. The doc and tests are there.

Though I am not familiar with rst format it would be good if someone who knows it check the doc.

[robfrawley]

Why not enable this test globally for all supported PHP versions? Or at least all PHP 7.x runs? I think this would be a better approach to ensure no incompatibilities are introduced across Symfony versions. We already have some Symfony 2.32, 2.8, 3.2 and 3.3 specific features or toggled usages.

All PHP 5.6 and PHP 7.x Runs

if [ "${TRAVIS_PHP_VERSION:0:3}" == "5.6" ] || [ "${TRAVIS_PHP_VERSION:0:1}" == "7" ]; then [...] fi

All PHP 7.x Runs

if [ "${TRAVIS_PHP_VERSION:0:1}" == "7" ]; then [...] fi

[makasim]

The dependency is optional therefore I dont want it to be tested all the time.

What if something goes wrong when enqueue is not installed and we do not notice it?

[makasim]

not only PHP version has to be taken into account but the version of Symfony as well. EnqueueBundle requires 2.8|3.0.

[robfrawley]

Can we spread this across lines?

->booleanNode('enqueue')
  ->defaultFalse()
  ->info('Enables integration with enqueue if set true. Allows resolve image caches in background by sending messages to MQ.')
->end()

[robfrawley]

Again, I'd prefer this testing wasn't based on an environment variable but globally run if the required bundle exists. (Aside from the fact that no one contributing code is going to set this when locally testing, using class_exists is a convention used regularly throughout this code base)

[robfrawley]

@makasim Have you built the RST documentation locally and ensured it's formatted as expected and without any compilation notices/warning/errors? It looks good to me on the first pass, but I always build the docs before pushing changes, myself.

Aside from my previous comments, though, this looks great!

[makasim]

Have you built the RST documentation locally and ensured it's formatted as expected and without any compilation notices/warning/errors?

@robfrawley No I haven't built it. Is there a doc on how to do it?
@javiereguiluz could you please point me how to build a doc locally (if it is possible) or other options to check its correctness?

[javiereguiluz]

The docs look correct to me and if there are warnings/notices/errors, you'll see them here: https://symfony.com/doc/build_errors

Building the docs locally requires following these steps:

1. Run sphinx-quickstart command to create the config file needed to build the docs. This command will ask you lots of questions.
2. Run sphinx-build -b html ./Resources/doc ./Resources/html-docs command to generate the docs and see if there are any warning/notice in the output.

[robfrawley]

@makasim There are some additional steps you need to take to build the docs; to do a complete build that won't throw any notices, warning, or errors (unless it is supposed to) follow:

https://github.com/liip/LiipImagineBundle/wiki/Building-RST-Documentation

[makasim]

@robfrawley I tried to build doc as it is described here https://github.com/liip/LiipImagineBundle/wiki/Building-RST-Documentation

It builds symfony docs and there are any errors and warnings but it ignores liip-imagine-bundle

Here's the container I used: https://github.com/makasim/symfony-doc-builder

I am sure I am missing something small. Could you please help?

[robfrawley]

The third build step symlinks the /Resources/doc/ folder within the Imagine Bundle root to a folder named liip-imagine-bundle within the Symfony documentation root. I don't think you are performing that step.

So in the case of the instructions, given the following directory paths

• Imagine: /path/to/imagine-bundle
• Symfony Docs: /path/to/imagine-bundle/symfony-docs

The third build step ln -s ../Resources/doc/ liip-imagine-bundle would create a symlink at

/path/to/imagine-bundle/symfony-docs/liip-imagine-bundle

pointing to

/path/to/imagine-bundle/Resources/doc

You should be able to quite literally copy the steps from the wiki and end up with a valid result.

[makasim]

There is no that step in the README.md but I did perform it. Here's what I have in docs folder (cleaned up output a bit):

symfony-doc-builder(master) $ls -la symfony-docs/
total 1656
drwxr-xr-x  82 makasim  staff    2788 May  5 20:00 .
drwxr-xr-x   9 makasim  staff     306 May  5 20:00 ..
drwxr-xr-x  11 makasim  staff     374 May  5 20:01 _build
drwxr-xr-x  16 makasim  staff     544 May  5 19:32 _images
drwxr-xr-x   4 makasim  staff     136 May  5 19:32 _includes
drwxr-xr-x   9 makasim  staff     306 May  5 19:32 assetic
-rw-r--r--   1 makasim  staff     130 May  5 19:32 assetic.rst
drwxr-xr-x  15 makasim  staff     510 May  5 19:32 best_practices
drwxr-xr-x  11 makasim  staff     374 May  5 19:32 bundles
-rw-r--r--   1 makasim  staff    6518 May  5 19:32 bundles.rst
-rw-r--r--   1 makasim  staff  312454 May  5 19:32 changelog.rst
drwxr-xr-x  55 makasim  staff    1870 May  5 19:32 components
-rw-r--r--   1 makasim  staff    1693 May  5 19:32 index.rst
drwxr-xr-x   4 makasim  staff     136 May  5 19:32 introduction
lrwxr-xr-x   1 makasim  staff      55 May  5 19:33 liip-imagine-bundle -> ../../symfony/vendor/liip/imagine-bundle/Resources/doc/
drwxr-xr-x   9 makasim  staff     306 May  5 19:32 logging
-rw-r--r--   1 makasim  staff    2030 May  5 19:32 workflow.rst

[robfrawley]

This seems to work very well for me and enables browsing the generated source using Nginx by extending the nginx docker image. Here is the Dockerfile:

FROM nginx

WORKDIR /symfony-doc

RUN apt-get update && \
    apt-get install -y --no-install-recommends --no-install-suggests python-sphinx python-pip make git && \
    rm -rf /var/lib/apt/lists/*

RUN pip install --upgrade pip
RUN pip install setuptools
RUN pip install git+https://github.com/fabpot/sphinx-php.git

RUN rm -fr LiipImagineBundle || echo "Filesysytem clean already..."

RUN git clone https://github.com/makasim/LiipImagineBundle LiipImagineBundle
RUN git clone https://github.com/symfony/symfony-docs LiipImagineBundle/symfony-docs

RUN cd LiipImagineBundle/symfony-docs/ && ln -s ../Resources/doc/ liip-imagine-bundle
RUN cd LiipImagineBundle/symfony-docs/_build && make html

RUN cp -R LiipImagineBundle/symfony-docs/_build/html/* /usr/share/nginx/html

Build our Dockerfile and run the generated image:

docker build -t nginx-doc-test .
docker run -d -p 8080:80 nginx-doc-test

And then load it up in your browser of choice (in this case Chrome):

google-chrome http://localhost:8080/liip-imagine-bundle/

[robfrawley]

@makasim Having built it, I can confirm your page isn't compiled because you aren't referencing it anywhere. You may want to add it to the index.rst under the table of contents tree.

[makasim]

@robfrawley symlink did not work for me (there were rst files in the container) so I copied them. Everything went fine (there was not any warnings not errors). I had to add my doc to index.rst.

The doc is ok!

[robfrawley]

LGTM

[makasim]

Merging then...

[robfrawley]

@makasim Can you take a look at the 2.0 merge of your code, which includes scalar type hints and function return types, and let me know if anything stands out as being wrong.

[makasim]

@robfrawley looks good.