Adds support for Etag header and not modified status

[adri]

This adds basic support for the Etag header by generating a hash of the response content.

If a request supplies an Etag value which matches the generated hash of the response, a StatusCode of 304 and empty content is set on the response.

For enabling the Etag support in the configuration:

    'etag' => array(
        'override' => true
    )

A value should not be needed because it is automatically generated.

What do you think?

[adri]

@renatomefidf had the idea to support a callback in the configuration which allows to generate and check custom Etag implementations. I'll add it to this pull request.

[corentin-larose]

@adri, thanks, I will review this as soon as you completed your PR. Using callbacks in the config is ok as long as these are not anonymous functions placed in the config file.

[adri]

@corentin-larose Thanks. I was thinking of providing a class FQN. By defining a factory for this class, other dependencies could be injected. For the ETags it is handy to have a database connection or a mapper reference for example.

'callback' => 'Namespace\ClassFQN'

[corentin-larose]

@adri, yes, I would do exactly the same.

[adri]

@corentin-larose Currently the ETag is a hash of the response content. With a callback we could generate an Etag without computing the response content and return early on a Etag match.

While implementing a "early return" I have issues hooking into the right part of the event loop(s). Somewhere where the action is known and the fetch/fetchAll is not called yet would be perfect. Do you have an idea where to hook into?

---

What we tried: The most interesting Resource methods to get an ETag for would be fetch and fetchAll. So I was thinking to do something like this in the HttpCacheListener.

        $this->listeners[] = $events->attach(MvcEvent::EVENT_DISPATCH, [$this, 'onDispatch'], -1000);

// ... onDispatch
       if ($callback->checkEtag($e->getRouteMatch(), $requestEtag)) {
            $response->setStatusCode(304);
            $response->setContent(null);
            $e->stopPropagation();

            // Return early
            return $response;
        }

But the fetching seems to happen within a dispatch event listener in the AbstractRestfulController. When hooking into before this dispatch listener we have to find out the action (fetch/fetchAll) based on the RouteMatch ourselves. It would be nicer to not duplicate the logic in AbstractRestfulController.

Another way might be to overwrite the dispatch method of the Resource. But on the provided ResourceEvent there is no Response and the Status Code cannot be set.

[adri]

@corentin-larose I've updated the pull request with the possibility to configure a custom generator(formerly callback). I couldn't find a way to "abort early" but at least users are free to choose what algorithm to use when creating the Etag. A default etag generator is included, which uses md5.

The PR should be ready to review.

[adri]

Ah sorry noticed that I used the wrong header. I'll let you know when it is fixed.

[adri]

Done :) Ready for review

[adri]

Great feedback, I'll make the changes.

[corentin-larose]

@adri, still thinking about the early return

[weierophinney]

Regarding short-circuiting, I have an idea: don't have setETag() return $this, but instead have it return either the $response or null. If $response is returned, then you can short-circuit by returning the response instance.

[weierophinney]

@corentin-larose When this is ready to merge, it should be merged to the develop branch for an upcoming 1.3.0 release.

[adri]

@corentin-larose @weierophinney Done with fixing the review outcomes. Thanks for the feedback, let me know if I can do more.

@weierophinney Do you mean short-circuiting as in avoiding calling the fetch/fetchAll on the controller or returning right after onResponse is called? I wonder how much the performance gain is when returning onResponse.

[corentin-larose]

@adri will review, then if all ok, will merge in the dev branch for the next release.

[adri]

Great, thanks @corentin-larose!

[corentin-larose]

@adri, I didn't forget your PR, I'm preparing the 1.3 (or 2.0 not sure yet) of the library which will allow to use the cache with either controller/action or route name based config. The changes in code are rather simple, but I have to think of the BC breaks.

[adri]

Ping :) Any updates on this?

[corentin-larose]

@weierophinney is going to make the second review in a couple of days.

[jvandijk]

@weierophinney are there still plans to add this functionality?

[weierophinney]

Merged to develop for release with 1.3.0 (should be today).