Problem/Motivation

Our approach to downloadable URLs is to have the absolute path to the image, so the consumer can fill in the rest. This is not the desired approach, nor the approach taken by Consumer Image Styles.

Proposed resolution

Use the fully resolvable URL in the returned object.

API changes

The url property on files now contains the full download URL.

CommentFileSizeAuthor
#2 2929600--use-full-url-file--2.patch1.48 KBe0ipso
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

e0ipso created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
FileSize
2929600--use-full-url-file--2.patch1.48 KB

I realize this breaks BC, but I wanted to start the discussion.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +BC break
Related issues: +#2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field
  1. Why break BC for this when this is already a temporary feature until #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field lands?
  2. This is not the desired approach
    Why is this not desired?
  3. nor the approach taken by Consumer Image Styles.
    Then why shouldn't Consumer Image Styles update its approach?
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented
  1. I have not played with that patch to see if it would work right away.
  2. Because we want a URL with the FQDN.
  3. What I meant is that the current approach is not desired, and the current approach does not match the (better) approach taken by Consumer Image Styles

I created this issue merely to allow people to turn this feature on easily. This is just a convenient way to host a patch.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Okay.

I disagree with the use of a FQDN. If you want that though, then you should chime in on #2825487: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field soon!

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Oh! I'll read more on this tomorrow.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

👍

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Can you link to the specific comment that has the reasoning you're highlighting in #5?

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

See #2825487-12: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field.4 + #2825487-109: Fix normalization of File entities: file entities should expose the file URL as a computed property on the 'uri' base field.

All of Drupal core has been switching to root-relative file URLs, see #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send and #2099205: When uploading and inserting an image trough the WYSIWYG plugin a relative path should be used for the image source (src) .

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Postponed

This is just a convenient way to host a patch.

Marking postponed, so that people don't think this patch needs a review.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
CreditAttribution: e0ipso as a volunteer and at Lullabot commented

Marking postponed, so that people don't think this patch needs a review.

Good call. Thanks.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Postponed » Closed (outdated)

The upcoming 2.x branch of JSON API will be able to drop this work-around, because Drupal 8.5 core fixed the problem in core! i.e. once #2926463: [>=8.5] Remove JSON API's "file URL" field work-around now that Drupal core 8.5 fixed it lands in the 2.x branch (once the 2.x branch actively branches off from the 1.x branch), this will no longer be relevant.