By submitting patches for review, you agree to the Developer Certificate of Origin. A copy of this agreement can be found in the project root, named CONTRIBUTING.txt.

Problem/Motivation

It would be amazing if this module would support direct download, rather than just inline display of the media item!

Steps to reproduce

N/A

Proposed resolution

Emulate what Dropbox does, and check for dl=1 in the query string of the media item. If it's there, then set the Content-Disposition header to attachment to force a direct download.

Remaining tasks

  • Get maintainer's blessing
  • Create a merge request with tests
  • Review/manually test
  • Commit it!

User interface changes

None.

API changes

None. I don't think this counts as API.

Data model changes

None.

Issue fork media_download-3222704

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

phenaproxima created an issue. See original summary.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Issue summary: View changes

Minor formatting change in the issue summary.

clayfreeman’s picture

clayfreeman
he/him
Primary language English
commented

I'm in favor of this idea.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review

Welp, that was pretty simple!

phenaproxima opened merge request !1

clayfreeman’s picture

clayfreeman
he/him
Primary language English
commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs change record

This looks pretty good! Pending CI results, my initial review is as follows:

  1. -    $response->setContentDisposition('inline');
+    // Force a direct download if dl=1 is in the query string.
+    if ($request->query->get('dl') === '1') {
+      $response->setContentDisposition('attachment');
+    }
+    else {
+      $response->setContentDisposition('inline');
+    }

    We should update the cachable metadata to ensure that the response varies on context url.query_args:dl.

  2. I think it'd also be wise to invalidate the http_response cache tag for thoroughness; otherwise a manual cache rebuild might be necessary for dl=1 to take effect.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs work » Needs review

Good idea on the cacheability metadata. I added the cache context, and corresponding test coverage, but I'm not sure what you mean by this:

I think it'd also be wise to invalidate the http_response cache tag for thoroughness; otherwise a manual cache rebuild might be necessary for dl=1 to take effect.

In which cases should we invalidate the tag? Just if dl=1, or in all situations?

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

In Slack, @clayfreeman clarified that the http_response cache tag should be cleared in an update hook, just to make things a little more seamless. So I've added a post-update function for that. No automated test coverage for it, since a) I wouldn't know how to test that and b) no data models are being changed.

clayfreeman’s picture

clayfreeman
he/him
Primary language English
commented
Status: Needs review » Fixed

This looks great to me! Thanks for your interest in the project :)

clayfreeman’s picture

clayfreeman
he/him
Primary language English
commented
Issue tags: -Needs change record

CR created & published here: https://www.drupal.org/node/3222821

  • clayfreeman committed 115d82a on 1.1.x 
    Improve inline docs for DownloadController::save()

- Documents the new...

Status: Fixed » Closed (fixed)

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