Comments

Grimreaper created an issue. See original summary.

grimreaper’s picture

Assigned: grimreaper » Unassigned
Status: Active » Needs review
StatusFileSize
new2.58 KB

Here is the patch.

Thanks for the review.

grimreaper’s picture

Same patch but made against the 8.x-1.0-alpha1 tag.

grimreaper’s picture

Sorry, I had not tested the dependency injection. Here are two new patches that do it properly.

mark_fullmer’s picture

Assigned: Unassigned » mark_fullmer
StatusFileSize
new74.92 KB
new73.92 KB
new2.18 KB

The previous patches weren't in the right format to be apply-able; I've recreated the same changes in the attached patch. I've also verified the fix, though I'll forego RTBC'ing this, since I'm adding a patch now.

For future reference, a quick manual verification of this (in the absence of automated tests) is below:

/**
 * Implements hook_ENTITY_TYPE_view_alter().
 */
function my_module_block_content_view_alter(array &$build, BlockContentInterface $entity, EntityViewDisplayInterface $display) {
    // Add 'max age' to bubble up expiration via custom event subscriber.
    $build['#cache']['max-age'] = '30';
}

A content block placed on a page will yield the following:

.... while a page without that block (assuming a global TTL set to "60" will show:

Importantly, I think this "Expires" behavior was not the original intent of the module (rather, it was intended for reverse proxies & edge caches; however, I think there are valid use cases for letting the "Expires" value bubble up to the Internal Page Cache's cache_page bin, as this change accomplishes (e.g., a widget that is responsible for retrieving external content every X minutes).

leon kessler’s picture

I don't see why this patch should not be incorporated into the project. Yes most people will be using this module with an external cache/CDN, and probably have the internal page cache module uninstalled.

However, for those that need to use the page_cache module, it's either this patch, or patching Drupal core with https://www.drupal.org/project/drupal/issues/2352009#comment-14064154 (which is the latest patch on that issue at this current time).

I don't believe there is any harm in setting the correct Expires header, but I suppose there could be some unexpected consequences for someones setup (although you could argue if this does cause an issue for someone, it's a symptom of a bug in their infrastructure, rather than with the Drupal instance itself).

feng-shui’s picture

Re-roll against 2.x.

prudloff’s picture

We should probably decide which patch to keep between #2962699: Expires header not set and #2916705: Page cache isn't invalidated.

prudloff’s picture

Status: Needs review » Closed (duplicate)

Closing as a duplicate of #2916705: Page cache isn't invalidated, the two patches basically do the same thing and the other one has a more recent patch.

prudloff’s picture