File formatter render absolute url to file

1. +++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php
   @@ -110,7 +110,7 @@ public function testEntityDisplaySettings() {
   -    $expected['settings'] = array();
   +    $expected['settings'] = array('absolute_url' => TRUE);
   

   So wait, we make it absolute by default?

2. +++ b/core/modules/file/config/schema/file.schema.yml
   @@ -82,6 +82,10 @@ field.formatter.settings.file_table:
   +      absolute_url:
   +        type: boolean
   +        label: 'Render as absolute URL'
   

   The indentation is a little bit off.

3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/UrlPlainFormatter.php
   @@ -20,12 +21,61 @@ class UrlPlainFormatter extends FileFormatterBase {
   +    if ($this->getSetting('absolute_url') && $this->getSetting('absolute_url') == 1) {
   ...
   +      if ($this->getSetting('absolute_url') && $this->getSetting('absolute_url') == 1) {
   

   Its a boolean, I think $this->getSeting('absolute_url') should be enough.

I think that now that #2517030: Add a URL formatter for the image field is in, we should also update the new image_url formatter to have the same option. Also we could use some tests here and we definitely need an upgrade path.

I agree we need tests. But I'm not sure we need an upgrade path.

+++ b/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php
@@ -115,6 +115,7 @@ public function testEntityDisplaySettings() {
+    $expected['settings'] = array('absolute_url' => FALSE);

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/UrlPlainFormatter.php
@@ -20,12 +21,61 @@ class UrlPlainFormatter extends FileFormatterBase {
+    $form['absolute_url'] = array(
...
+    $summary = array();
...
     $elements = array();
...
       $elements[$delta] = array(
...
         '#cache' => array(

Nit: short array syntax.

And here's a re-roll with the array stuff fixed. No interdiff as its just a re-roll (with array short syntax fixes).

reroll to 8.6.x

It looks like from #37 we should to this for image_url formatter as well. This is small enough, let's just do it all in one place.

So, NW for that and tests.

Patch for the #37. Adding the absolute url for the Image Url field formatter.

1. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/UrlPlainFormatter.php
   @@ -20,12 +21,61 @@ class UrlPlainFormatter extends FileFormatterBase {
   +    $summary = [];
   +
   +    if ($this->getSetting('absolute_url')) {
   +      $summary[] = t('Rendered as absolute url');
   +    }
   +    else {
   +      $summary[] = t('Rendered as relative url');
   +    }
   

   This can be shorten as:

   $summary[] = $this->getSetting('absolute_url') ? t('Rendered as absolute url') : t('Rendered as relative url');
   

2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
   @@ -23,7 +23,9 @@ class ImageUrlFormatter extends ImageFormatter {
   +
   

   This empty line is not required.

3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
   @@ -43,8 +51,16 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
   +    $summary = [];
   +
   +    if ($this->getSetting('absolute_url')) {
   +      $summary[] = t('Rendered as absolute url');
   +    }
   +    else {
   +      $summary[] = t('Rendered as relative url');
   +    }
   

   This can be shorten as:

   $summary[] = $this->getSetting('absolute_url') ? t('Rendered as absolute url') : t('Rendered as relative url');
   

4. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
   @@ -63,9 +79,14 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +
   

This empty line is not required.

Here are some of the feedback. @jyoti.singh Can you please provide interdiff as well. We need tests for this patch as well.

Thank you @msankhala for the feedback , updating the patch for the same.

Attaching another patch for the comment #52

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/UrlPlainFormatter.php
@@ -20,12 +21,55 @@ class UrlPlainFormatter extends FileFormatterBase {
+

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
@@ -23,7 +23,9 @@ class ImageUrlFormatter extends ImageFormatter {
+

@@ -43,8 +51,10 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+

@@ -63,9 +73,14 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+

These empty lines can be removed. @jyoti.singh Can you please provide the interdiff as well? https://www.drupal.org/documentation/git/interdiff
so it makes it easy to review what you have modified since the last patch.

This patch requires test cases as well. I think the test for these should go inside FileFieldDisplayTest and ImageFieldDisplayTest.

For Core 8.7 the last patch didn't work so I remade it.

Sorry, got bad name for the patch, re-uploading it.

Is there a special reason why you do not use the $relative parameter of createFileUrl() directly? This method already has all the logic to return a relative or absolute URL.

Because that's new and didn't exist when this code was written, agree that it should be used now.

I'm also wondering about the use case from the issue summary:

> When sending the values in a mail (contact form), it should render the absolute url so you can click directly to the file from within a received mail.

Drupal core should now automatically convert relative URL's in e-mails to absolute, are there other reasons for this? We've worked hard in Drupal 8 to avoid absolute urls whenever possible.

Drupal core should now automatically convert relative URL's in e-mails to absolute, are there other reasons for this? We've worked hard in Drupal 8 to avoid absolute urls whenever possible.

RSS Feeds

Applies cleanly on 8.9, tested with views formatter, works fine.

> RSS Feeds

Yeah, covered: \Drupal\Core\EventSubscriber\RssResponseRelativeUrlFilter

@berdir: Uh nice... didn't knew about that one. That looks very promising... But apart from that I still think it is a good idea to let the site builder decide if for whatever reason there is an absolute URL output required.

Maybe there should be some sort of short description on the checkbox field explaining why absolute URLs may lead to trouble in certain cases.

...so if this feature should still get in, my finding in comment #62 should still be implemented - let me know if I should change the patch for it, or if this issue will more likely to be closed?!

I see, this works but for description field only. My use case also has html in other rss item field (don't ask), hence why.

Still I tend to agree with #68.

Adding three test cases to validate field formatter changes and configuration.

Reuploading the above patch with relevant fixes.
- It contains three test cases to validate field formatter changes and configuration.
- PHPCs error fixes in the above patch.

I've intentionally kept diff against comment #61 so we can easily see the diff between those.

Update:
Accidentally added interdiff with .patch extension instead of .txt

+++ b/core/modules/file/src/Plugin/Field/FieldFormatter/UrlPlainFormatter.php
@@ -18,16 +19,60 @@
     foreach ($this->getEntitiesToView($items, $langcode) as $delta => $file) {
+      if ($this->getSetting('absolute_url')) {
+        $markup = file_create_url($file->getFileUri());
+      }
+      else {
+        $markup = file_url_transform_relative(file_create_url($file->getFileUri()));
+      }
+
       assert($file instanceof FileInterface);
       $elements[$delta] = [
-        '#markup' => $file->createFileUrl(),
+        '#markup' => $markup,

the assert() needs to move up before a method on $file is called, and we can still use createFileUrl(TRUE/FALSE) here. In fact, you can just use $file->createFileUrl(!$this->getSetting('absolute_url'))

Also, if we do make it absolute, we need to add the url.site cache context and should assert that as well.

@Berdir, thanks for the suggestions.
I've included those in the current patch. I've also updated test cases accordingly.

Resolving issues in testImageFieldFormattersPrivate() test case.

@berdir, somehow functional javascript tests are failing on DrupalCI. Is there any way to identify which is causing issues?

+++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
@@ -228,6 +228,31 @@ public function _testImageFieldFormatters($scheme) {
+    $expected_url = file_create_url($image_uri);
+    $this->assertEquals($expected_url, $node->{$field_name}->view($display_options)[0]['#markup']);
+    if ($scheme === 'public') {
+      $this->assertSession()->responseHeaderContains('X-Drupal-Cache-Contexts', 'url.site');
+    }
+    if ($scheme === 'private') {
+      $this->drupalLogin($this->adminUser);
+      $this->drupalGet('node/' . $node->id());
+      $this->assertEquals($expected_url, $node->{$field_name}->view($display_options)[0]['#markup']);
+      $this->assertSession()->responseHeaderContains('X-Drupal-Cache-Contexts', 'url.site');
+      $this->drupalLogout();

I'm.. not sure what this is testing exactly. You're asserting the response, but you're not actually visiting a page here, at least for the public case, so your assertion has nothing to do with what you are rendering, the url.site cache context just happens to be on the page the test last visited. And it doesn't work for private, because a few lines above, access denied is tested for private and that page doesn't have it.

But we don't actually need to do that, instead, lets just assert the render array directly, you're already looking at #markup, just assert ...['#cache']['contexts'] directly, with an assertContains(). And then also ensure that it's not there if you set absolute_url to FALSE.

Updating test cases as per the above comment.

Reroll the patch to work on the latest version

Patch on #82 failed to apply, here's a reroll.

Updated deprecated code in #83.

file_create_url() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use the appropriate method on \Drupal\Core\File\FileUrlGeneratorInterface instead. See https://www.drupal.org/node/2940031

file_url_transform_relative() is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use \Drupal\Core\File\FileUrlGenerator::transformRelative() instead. See https://www.drupal.org/node/2940031

Thanks!

I am working in it.

Hi,
I have created a patch for the absolute url for image. Please review it.

Reroll the patch to work on 9.4.x

patch #85 re-rolled (9.5.x).

Make it work for 9.4.8

Make it work for 9.5.0

Fixed CCF for #99.
Please review.

Attempt to fix the test case failures.
Both the failing test cases are passing on local now.

It seems that older instance of file_create_url($uri) should be replaced with $file_url_generator->generateAbsoluteString($image_uri).
It was set as $file_url_generator->generateString($image_uri).

Seems in all the reroll starting in #93 some files were add/removed from the original patch.

Was there a reason why?

Hi @smustgrave Not sure what do you mean about "original patch"? Which one is original? The first one? Then it's normal that other people have expanded it, improved it and added tests.
If you're talking about another patch, why do you consider it original?

I looked at the patch development trend and concluded that #103 is a logical result of the current patch evolution.
I installed and tested the patch for two Drupal sites: 9.4 and 9.5.
The patch works perfectly for me.

FYI. for 9.4 I used #97, but I had to update that patch a bit. Fixed the same URL issue as in #103
Added my patch to the issue but hid it, because 9.4 is already outdated.

Will let someone else review. #88 attempted a change that had a few files more then in #85.

Then #93 rerolled #85, so skipping #88 was that change not needed? No one seems to answer that.

Lack of interdiffs and comments don't explain why things are added or removed.

To me this needs an issue summary update now.

I have reviewed both the patches closely. #85 and #88

The changes in #88 seems completely in wrong direction.
Basically, key thing i.e. field formatter ImageUrlFormatter.php itself is missing from the patch.

I am hiding the patch given in #88 so we can have proper sequence.

Adding issue summary for clear direction.

Thanks to everyone for getting this older issue close to commit.

It appears that this is a self RTBC, #105. So this, at least, needs another review.

This is a feature request so we need a patch on 10.1.x. I am changing the version. Refer to the allowed changes policy for details.

I was going to look at the patch but, honestly, I do not know which one I should review. Hopefully, that will be clear when there is a patch for 10.1.x.

This also needs a title that better explains the change.

Thanks.

This is turning out to be somewhat confusing.

It seems that the patch in #93 is re-rolled against #85 because #92 and #88 contains the diff related to quick edit module.

So #88 and #92 seems somewhat incorrect to me.

#105 seems against 9.4.x, so we can ignore the patch.

Coming to conclusion:
@quietone, I noticed that the patch in #103 is getting applied on latest 10.1.x head.
I have triggered re-test of #103 to see how it goes.

@mohit_aghera, thanks! I was really quite lost and spending too much time trying to figure this out. So, the patch to work with is #103.

As best I can tell, the last review of the patch was in #79, and we have a confirmation that it works in #103. So, we still need a fresh review. I am changing the status to NR for that.

The next step is to review the patch in #103.

Reviewing #103

+  mapping:
+    absolute_url:
+      type: boolean
+      label: 'Render as absolute URL'

Appears to be adding a new field option, so probably needs a change record.

Also will need an upgrade path for existing sites.

- Added a post update hook to change the field formatter settings.
- Add a test case to validate the post update hook.

Think the only thing left would be the change record! Almost there!

this solution does not work with image styles, absolute URL is generated for original image only :(

Change record added here https://www.drupal.org/node/3368703

Keeping this in needs work as I'm yet to check #115

I was able to reproduce the issue mentioned in #115
Attached a patch and test case for the same.

Fixing test case failures.

Patch no longer applies cleanly to 11.x

error: patch failed: core/modules/file/tests/src/Functional/FileFieldDisplayTest.php:234
error: core/modules/file/tests/src/Functional/FileFieldDisplayTest.php: patch does not apply

but it was such a small change I just went ahead and did it.

With the patch applied verified the update hook ran without issue
Verified the view display setting on an Image field.
Created a node with the field setting set to absolute.
Verified the URL rendered as absolute.

- Patch is not getting applied to latest 11.x head. I did the rebase of the patch.
- Fixing test case failures. Test cases are passing on local now.

Let's see how it goes for CI pipeline.
I'm not quite sure why it was working previously and suddenly stopped working.

Seeing all green so restoring status. Thanks @mohit_aghera!

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions.

Thanks for getting this 7 year old issue to RTBC!

This is making a change to the UI, adding the usability tag. There should be a screenshot for the change in the issue summary, or linked to from there.

That made me think of the change record so I read that. It does needs updating to provide the reader with the information needed to use this new feature. It should be explicit about where the new setting is found, which form and the url. Also it would help to have a screen shot. I've added the tag for an update to the change record.

I then read the patch, noting the following points.

1. index 06ae05febb..686564f73b 100644
   --- a/core/modules/field/tests/src/Kernel/Migrate/d6/MigrateFieldFormatterSettingsTest.php
   

   Why is there no corresponding change in the same test for migration from Drupal 7 sources?

2. +++ b/core/modules/file/file.post_update.php
   @@ -7,6 +7,7 @@
   +use Drupal\Core\Entity\Entity\EntityViewDisplay;
   

   nit: should be added alphabetically

3. +++ b/core/modules/file/src/Plugin/Field/FieldFormatter/UrlPlainFormatter.php
   @@ -27,11 +61,16 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +      // Add url.site cache context if url is absolute URL.
   

   I am all for comments but this seems unnecessary as the if block is clear.

4. +++ b/core/modules/file/tests/src/Functional/Update/FileFieldFormatterUpdateTest.php
   @@ -0,0 +1,69 @@
   + * Tests the default values set by the update hook for "absolute_url" setting.
   

   This is really testing the update of the value. I think this is an improvement.

   " * Tests update of the 'absolute_url' file and image field formatter setting."

5. +++ b/core/modules/file/tests/src/Functional/Update/FileFieldFormatterUpdateTest.php
   @@ -0,0 +1,69 @@
   +              $this->assertEquals(FALSE, $settings['settings']['absolute_url']);
   

   This should use 'assertFalse'

6. +++ b/core/modules/file/tests/src/FunctionalJavascript/FileManagedFileElementTest.php
   @@ -106,6 +108,49 @@ public function testManagedFile() {
   +   * Test "Absolute url" settings on field display mode settings form.
   

   No quite meeting the standard, also is should be 'absolute_url'. Drupal API documentation standards for functions
   I suggest, "Tests the 'absolute_url' setting on the field display setting form."

7. +++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
   @@ -229,12 +230,57 @@ public function _testImageFieldFormatters($scheme) {
   +    // Test the absolute_url option with the image style setting.
   

   Similar to above. I suggest, "Tests the absolute_url setting with the image style setting."

8. +++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
   @@ -229,12 +230,57 @@ public function _testImageFieldFormatters($scheme) {
   +    if ($scheme === 'public') {
   ...
   +    if ($scheme === 'public') {
   

   These need work, there is a common assert to each if/else.

9. +++ b/core/modules/image/tests/src/Functional/ImageFieldDisplayTest.php
   @@ -229,12 +230,57 @@ public function _testImageFieldFormatters($scheme) {
   +    if ($scheme === 'public') {
   

   The same code is executed for public and private.

Finally, the comments are inconsistent when referring to the new setting. Sometimes 'option' is used instead of 'setting' it would be helpful to be consistent on that.

So, nothing major just a few things to tidy up.

Adding an updated patch + interdiff with changes based on most of the feedback in #125. Specific points:

1. The D7 MigrateFieldFormatterSettingsTest does not have any cases of file_url_plain which differs from the D6 version. Not sure on how to proceed here as it seems like it'd be a bigger change in the test migration?

8 + 9. It seems like the public vs private paths ultimately ended up with the same assertions so that the only difference came down to private triggering a logout in the test. That didn't seem necessary as the tests were asserting on the existence of the url.site cache context which should exist regardless of auth state. Ended up removing the if paths.

Also updated the Change Record with more descriptions and Issue Summary to include screenshots.

#3041402: Add option absolute url in formatter URL to image closed as a duplicate and moved over credit.

Updated to use a MR because the patch triggered DrupalCI process looks to be failing consistently due to disk space issues in the Jenkins based builds.

Same changes as patch in #126, the interdiff between 122 and 126 is represented in this commit

Left some feedback, all minor stuff.

Addressed feedback from #133. Not sure about MR resolving policy, setting this back to Needs Review to get eyes on it.

My feedback has been addressed.

I'm triaging RTBC issues. I see I have been here before and this is steadily making progress. I read the IS, which appears to be up to date, and the comments. I didn't find any unanswered questions.

This is adding a new string to the UI and I see from the screenshots that there is inconsistent capitalization of URL. Also, I don't think that 'render' is used in the Drupal UI. So, this needs input from usability for the correct wording. I don't know why I didn't see this before. I am setting this to needs work for getting feedback from the usability team. I'll tag it as well, for visibility.

Although, someone here can ask in #usability for advice. If you do, add who responded to your comment. Thanks.

I did not review the MR or consider if this deprecation can occur in 10.3.

Usability review

We discussed this issue at #3411359: Drupal Usability Meeting 2024-01-05. That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @benjifisher, @Emma Horrell, @rkoller, @shaal, and @simohell.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

At first thanks on working on the issue as well as that the issue summary was in good shape. That saved a lot of time and left more time to focus on the issue itself. We've based our explorations and discussions on the latest state of MR5882 on a Drupal 11.x-dev install with a content type which had a file, image and media field added. At first a few points we've noticed:

• On the Manage display page of a content type, per default, the summary for those three fields shows Rendered as relative url. When you edit the field you are only presented with the Render as absolute url checkbox - no signs of a setting for the relative url which was shown and referred to in the summary overview. The state of "relative url" is hidden or better is just communicated indirectly. If the checkbox is unchecked you get the relative url, if the checkbox is checked you get the absolute url - that fact adds a cognitive load to the user.
• There was an agreement with the concern raised in #137 about the usage of the term render in this context. The group agreed that it is probably not quite the right word. The term render would probably be more appropriate for example in the context of rendering templates.
• The description If checked, links will be rendered as absolute urls. in context with the checkbox label Render as absolute url isn't really explaining anything or adding any extra information. It is mostly repeating the checkbox label prepending the constraint If checked, links will be. At the same time things become tricky if you want to actually cover and explain all the dos and don'ts when a checkbox is checked. The description would become lengthy and hard to read.
• url is only lower case. According to https://www.drupal.org/docs/develop/user-interface-standards/interface-t... acronyms like URL should be capitalized.

There was a clear consensus in the group about the following recommendations:

1. Change the strings on the summary for a field from Render as relative url and Render as absolute url to Relative URL and Absolute URL. Ideally the details of interest, Relative URL and Absolute URL, should be front-loaded (currently the user has to scan past 10 characters before the detail of interest is reached). For this context we also agreed on less is more and simply removed the "Render as ".
2. Change the interface component from a checkbox to radio buttons. For the legend we would suggest to go with Show link as and for the radio button labels go with Relative URL and Absolute URL.
   For one by using radio buttons the relative URL option would become explicit in the UI and the case that each option would start redundant with Render as as well as the term "render" would be avoided by moving to the more neutral word Show in the legend. We were only a bit uncertain at first about adding a radio button group with just two options. But while writing up the comment I've revisited the user interface standards for radio buttons on d.o: https://www.drupal.org/docs/develop/user-interface-standards/radio-buttons and having a list of two is a legitimate case https://www.drupal.org/docs/develop/user-interface-standards/radio-butto.... Avoiding redundancy by moving parts of the radio button labels to the legend is also in line and recommended in https://www.drupal.org/docs/develop/user-interface-standards/radio-butto....
3. By switching to radio buttons both options are more or less self explanatory. The only detail that might be helpful to the user is to provide example(s) of the different output(s). If it is technically possible the "ideal" case would be that the description dynamically changes based on the selected radio button. So it is still a single description, when Relative URL is selected the suggestion would be
   Example: /sites/default/files/image.png, when Absolute URL is selected the suggestion would be
   Example: https://www.example.com/sites/default/files/image.png instead. We've just slightly shortened the examples used on https://www.drupal.org/node/3368703. But by using radio buttons and a description that adjusts dynamically to the selected radio button, things would become completely self-explanatory and clear. In case the dynamic description is out of the scope for this issue it could be also moved to a follow up issue?

At the end one additional detail that was noted during the discussion - not a recommendation or suggestion but more of question. It was asked if the functionality was tested on a site that utilizes for example the Domain module or any other similar module and if there might be potential problems?

I'll remove the Needs usability review tag again.

And one additional note i've noticed when i've looked at the change record before the meeting (@angrytoast pointed me to it helping me to understand the scope and problem space of the issue). I've already commented on Slack in the #ux channel that the current state of the change record is potentially misleading in the context of the views based REST export json responses.

In the first section (Example screenshots for site builders:) you have screenshots with the UI before the patch and the changes after the patch is applied. The second section (Example to illustrate the change with a views based REST export json response) is applying the exact same before and after pattern. That way a user might assume that before shows the response without the patch applied while after shows the response with the patch applied. Instead the before is the response for the Relative URL option while after is the response for the Absolut URL option. Maybe the easiest fix might be to simply replace before and after with Relative URL and Absolute URL? The headline could be left as is even and I've used the slightly shortened example URLs from #138.

Example to illustrate the change with a views based REST export json response:

Relative URL:
/sites/default/files/image.png

Absolute URL:
https://www.example.com/sites/default/files/image.png