Problem/Motivation

It'd be nice if there was an easy way to specify a custom disposition for each media entity e.g. through a query string.

e.g.

Proposed resolution

Provide a query string which alters the default disposition.

It'd also be nice if the module included a config schema definition for its settings.

Remaining tasks

Provide issue fork/patch.

User interface changes

N/A

API changes

N/A

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:

Comments

codebymikey created an issue. See original summary.

codebymikey’s picture

Some cursory code reviews:

  • The module also has an implicit dependency on the path_alias module in the Controller, so should probably be declared in the module dependencies definition.
  • The $response object initially created here is no longer used.

immaculatexavier made their first commit to this issue’s fork.

smustgrave’s picture

Thank you for reporting and the feature request. This was one of my first modules so definitely room for improvements. To make sure this ticket doesn't include too much I opened

#3295337: Create config schema
#3295338: Update module dependency to include path_alias

smustgrave’s picture

So addressed #3295338: Update module dependency to include path_alias already and #3295337: Create config schema I'm reviewing

To implement something like media_entity_download think we would need to extend the image formatter most likely with one of these hooks https://www.drupal.org/node/2130757

That along the lines you were thinking?

codebymikey’s picture

To make sure this ticket doesn't include too much I opened

#3295337: Create config schema
#3295338: Update module dependency to include path_alias

Thanks for the relatively quick fix!

To implement something like media_entity_download think we would need to extend the image formatter most likely with one of these hooks

Haha, I wasn't even thinking that far ahead, I was just thinking of having a query string like ?attachment or ?dl detected in the controller which determines whether the media item should be forcefully downloaded as an attachment when necessary even if the browser can typically open it inline just fine (aka., the current default behaviour, which I'm fine with).

The media entity may contain documents and archives file rather than images, so the image formatter approach might not be a feasible solution (unless I'm misunderstanding the scope of the implementation).

media_entity_download implements its own formatter to address a similar issue to this, I don't personally need a one size fits all disposition for all media entities, I think it makes sense for it to be on an entity by entity basis, by specifying the query string where necessary (and is more of a power user tool).

smustgrave’s picture

I'm just glad this module is proving useful for others!

I think we can add the detection of ?attachement under this ticket. And open a new one for making it configurable. You're right will need to have a hook that's dynamic based on the field (image or file).

So for this one

  1. Add detection code to the controller
  2. Add test cases since I just got those in (thanks for the update too!)
  3. Probably need to update README too.

Missing anything?

codebymikey’s picture

Nope, that all makes sense to me :)

smustgrave’s picture

Status: Active » Needs review

Ready for review

codebymikey’s picture

I didn't wanna update the PR without first getting feedback from you, so I've added some of my thoughts on the PR to get your input on them.

smustgrave’s picture

all fantastic feedback. And go for it! no issues here

smustgrave’s picture

Made some updates based on the feedback but the tests are still failing.

codebymikey’s picture

I also made some changes and had the tests failing locally, so didn't wanna push them up until I had time to debug why.
Most of the changes were pretty innocent, so was slightly interesting to see the tests fail.

I fixed the initial failed test (I think this is the one you're also having) which were because all calls to $this->createMedia(); essentially share the same underlying file entity internally, so after unlinking it here, subsequent calls to it will have the file missing from disk and lead to inconsistent behaviour, the files need to be unique per $this->createMedia(); call.

I then ran into another issue with the failed test when I introduced custom path aliases to ensure the behaviour was still consistent with or without aliases, this is what I have in my backlog to fix before I push up.

codebymikey’s picture

And lol, slightly interesting to see @immaculatexavier supposedly made their first commit to this issue’s fork and was given a draft credit, but there's no actual commits. Wonder what happened there.

smustgrave’s picture

I’ll fix the credit before marking complete. And wonder if it’s a routing issue? When I append ?download it works but redirects back to media/123 so maybe that’s why content header isn’t updating?

smustgrave’s picture

So for now I don't mind commenting out those test cases. Can't figure out why it's not registering the content header. Looked at the examples you provided and doing almost hte same as them.

Anyway besides those is there anything else failing?

codebymikey’s picture

Fixed the broken test cases, it was related to the test case reusing the same test file internally (especailly during deletions), as well as $this->file, which meant the test was referencing the wrong file instance while doing its comparisons. I've moved the references into local variables with appropriate names, as well as removed reliance on the path_alias module.

It shouldn't be necessary for this to work since the controller shouldn't be concerned with the alias at that point. It should've already resolved the media entity it's working with.

The change introduces a bunch of other minor improvements, so I've added them in a new branch for review/cherry-picking purposes.

smustgrave’s picture

Status: Needs review » Fixed

Nice! Merged

Status: Fixed » Closed (fixed)

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