File formatter render absolute url to file

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.

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.

@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.

The last submitted patch, 117: 2811043-117.patch, failed testing. View results →

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.

The last submitted patch, 120: 2811043-120.patch, failed testing. View results →

- 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.

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.

smustgrave → credited mrshowerman → .

smustgrave → credited SandeepSingh199 → .

smustgrave → credited Xperd → .

✨ Add option absolute url in formatter URL to image Needs review 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, interdiff 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 📌 Drupal Usability Meeting 2024-01-05 Active . 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 ✨ File formatter render absolute url to file Needs work 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:

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

I've applied the diff from merge request on a 10.3.x site and it works great: https://git.drupalcode.org/project/drupal/-/merge_requests/5882.diff

In order to rebase the MR, @smustgrave needs to resolve the existing threads.

I changed the wording from recommendations on #138.1.
#138.2 Changing checkbox to radio button: This would require rewriting large part of the MR and tests.
#138.3: I would say its out of scope, since the dynamic functionality described does not exist yet in Drupal core.

I did tried to fix the feedback mentioned in the #138
I've attached a few screenshots about how it looks like on the backend.

For now, i've pushed the changes related to modified files.
I'll keep an eye on tests and will address additional failures.

Hiding all the old patches and files to prevent the confusion.

Addressed the feedback in #138
I'm slightly confused about changes proposed in #139 comment.
I have updated the change record screenshots so it is more useful.

Re-rolled diff from MR 5882 for anyone else who still needs compatibility with PHP 8.1.

At first sorry for the late reply. I've manually tested MR5882. From a functional perspective the MR looks fab. All the points we've raised in #138 ✨ File formatter render absolute url to file Needs work look and work as suggested now. Thank you mohit_aghera!
In regards of #139 ✨ File formatter render absolute url to file Needs work it took me a while to figure out what i meant back then and what the actual problem was I've tried to outline. Should have been clearer, apologies. :( My suggestion was, on https://www.drupal.org/node/3368703 → you have "Example to illustrate the change with a views based REST export json response". After that you have Before and After in bold. Those two titles/labels are misleading and probably mislabled. My suggestion simply was change Before to Relative URL and After to Absolute URL. And thanks for updating the images to the current state in the changelog.
I leave the status at needs review cuz i am not qualified to provide a code level review and i've readded the usability tag since the issue is considered an usability improvement.

MR appears to need a rebase.

I believe the patch in #147 doesn't allow the generation of image styles as it is not using the standard image style generation function. I found this which goes in detail https://www.unimitysolutions.com/insights/image-style-token-itok-and-drupal . I have this issue on my Drupal backend.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

False positive

Left some comments/questions on the MR

@larowlan All the feedback mentioned in the PR is fixed now.
PR is green, I think it is ready for another round of review.