Problem/Motivation

Port:

  • CDN_BASIC_FARFUTURE_* constants in cdn.constants.inc
  • cdn_file_url_alter()'s remaining logic in cdn.module
  • cdn_cdn_unique_file_identifier_info() in cdn.module (should become a new plugin type)
  • cdn_menu() in cdn.module
  • _cdn_ufi_deployment_id() in cdn.module
  • cdn.basic.farfuture.inc
  • cdn_admin_details_form() and cdn_admin_details_form_validate() in cdn.admin.inc
  • cdn_ui/help/admin-details-mode-pull-far-future.html and cdn_ui/help/admin-details-mode-pull-ufi.html
  • CDNOriginPullFarFutureTestCase in tests/cdn.test

Proposed resolution

First we should verify that we even want to port this.

Remaining tasks

User interface changes

API changes

Data model changes

Yes.

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Assigned: Unassigned » wim leers

I discussed this with @Fabianx. And while doing so, I realized that much (most!) of the typical cost that comes with Far Future expiration is in fact a non-issue in Drupal 8 thanks to Dynamic Page Cache.

Fabianx said he'd keep this functionality if he were the maintainer.

So, it's decided, I will port this.

11:26:03 <WimLeers> I ported the CDN module
11:26:14 <WimLeers> But I'm not sure if it's worthwhile to port the "Far Future expiration" functionality
11:26:28 <Fabianx-screen> Pros / Cons?
11:26:30 <WimLeers> so what that does, is rewrite file URLs so that they change whenever the file changes
11:26:40 <WimLeers> e.g. embed mtime or file hash or deployment ID
11:26:41 <Fabianx-screen> I used far-future before.
11:26:52 <Fabianx-screen> yes
11:26:58 <WimLeers> the benefit is that changed files show up immediately
11:27:09 <WimLeers> and you don't have to do anything advanced to get it
11:27:19 <WimLeers> because the CDN module serves the file instead of the web server
11:27:24 <WimLeers> so the CDN module can serve it with the right headers
11:27:24 <Fabianx-screen> yes
11:27:28 <WimLeers> this is fine because the CDN caches it
11:27:31 <WimLeers> the huge downside…
11:27:43 <WimLeers> … is that this means reading mtime on every non-cached request
11:27:51 <WimLeers> of course, with D8 the cost of that is much much much lower
11:27:53 <WimLeers> thanks to Dynamic Page CAche
11:28:13 <WimLeers> Still, I'd love to hear your opinion
11:28:29 <Fabianx-screen> Would it not be enough to do:
11:28:37 <Fabianx-screen> file?hash instead of the far-future thing?
11:28:55 <WimLeers> 1) some proxies strip query strings for static files
11:29:25 <WimLeers> 2) sure, in general it will be, but I _still_ need A) to serve the file by the CDN module to set the right headers, B) calculate the hash, which is the really expensive part
11:29:49 <WimLeers> And for point A: yes, users could modify their web server settings, but that's somewhat difficult and is not possible on shared hosting
11:30:11 <WimLeers> the serving of the file by the CDN module is not the big concern for me, it's the hash calculation
11:30:40 <Fabianx-screen> I think it is good functionality and I would probably keep it (if I were the maintainer).
11:30:47 <WimLeers> ok
11:30:51 <WimLeers> thanks for the feedback!
wim leers’s picture

I've been working on this quite a bit in the past two weeks. But even though I've simplified the Far Future expiration functionality a lot (I've removed the auto-generating of files), and even though Symfony now handles some significant portions (serving of files including Accept-Ranges: bytes support), it's still very complex. The main reason: everything is configurable. Which means all of the settings have to be parsed, interpreted, and routed to the right code, which then is best implemented as plugins, et cetera.

But for who am I doing this? I think the vast, vast majority of users never even changed their Far Future expiration configuration. The complexity of this functionality (and the improved way I was implementing it) is causing the oh-so-simple CDN module for D8 to have at least a doubling in terms of complexity. Which causes me great concern. I want this module to be simple to use, but also simple to debug and maintain. I don't think this extra complexity is worth it.

So, I'm posting the local commits I've made so far, and I'm going to revert the last commit and parts of earlier commits. It's only that started to address the configurability aspect. I'm going to remove the detailed configurability of Far Future expiration. And instead, I'm just going to make it always use mtime, for all files. That's good enough, and it vastly, vastly simplifies things.

wim leers’s picture

From 28 files changed, 558 insertions(+), 661 deletions(-), and not even be remotely completed…

… to 20 files changed, 284 insertions(+), 850 deletions(-)

So much simpler!

(Rather than reverting a commit, I just added a new commit.)

wim leers’s picture

Status: Active » Needs review
StatusFileSize
new70.4 KB
new121.07 KB
new29.89 KB

Clean-up + test coverage.

Let's see if this passes tests.

The last submitted patch, 5: cdn_farfuture-2708787-5-all_commits_combined.patch, failed testing.

The last submitted patch, 5: cdn_farfuture-2708787-5-all_commits_combined.patch, failed testing.

wim leers’s picture

Clean-up, both in terms of consistent naming (always farfuture) of methods, as well as file names. Most importantly: this adds an integration test for the farfuture functionality.

The last submitted patch, 8: cdn_farfuture-2708787-8-all_commits_combined.patch, failed testing.

The last submitted patch, 8: cdn_farfuture-2708787-8-all_commits_combined.patch, failed testing.

wim leers’s picture

Fix nitpicks, and re-enable the uninstallation test coverage.

I've been unable to pinpoint the root cause of that one last failure. I've confirmed that reverting this patch causes tests to pass again. So this is somehow causing the fail, I just don't understand how yet. Fixing that won't be for today, it's getting very late here.

So close!

The last submitted patch, 11: cdn_farfuture-2708787-11-all_commits_combined.patch, failed testing.

The last submitted patch, 11: cdn_farfuture-2708787-11-all_commits_combined.patch, failed testing.

wim leers’s picture

Found it. Was a silly small thing. Had to be.

wim leers’s picture

Finishing touches.

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

I declare this ready now :)

  • Wim Leers committed a6fdf18 on 8.x-3.x
    Issue #2708787 by Wim Leers: Port Far Future expiration to 8.x-3.x
    
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

24 files changed, 451 insertions(+), 1065 deletions(-)

Glorious simplification :) In part because Symfony provides some of the things that the CDN module used to do. And in part because it now will not attempt to generate files that don't exist yet (which was the cause of MANY bug reports in D7), nor is it configurable: it simply uses mtime for all files, rather than configurable-per-directory "unique file identifiers". The omission of the most brittle functionality and configurability allows for a sharp decrease in complexity.

wim leers’s picture

By the way, this is what its configuration UI looks like, including its tour tip.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.