Problem/Motivation

The image field should have formatter that supplies a URL to the image or any available image styles. A URL only formatter is needed for Field based REST displays in views.

Proposed resolution

Add a URL formatter for images.

Remaining tasks

User interface changes

Add a URL option to the formatter dropdown for image fields.

API changes

none

Data model changes

none

Why should this issue be considered for the release

Pro

  • This doesn't add any BC problem
  • This enables more people to use REST (via Views) without additional work

Con

  • It could be also seen as Task
  • There is nothing which cannot be solved by contrib

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category This issue is categorized as BUG because its really inconvenient to use REST with files. There is a) an issue to output not only the fid but also the path to the file,
but we need an additional image formatter in order to deal with image styles.
Issue priority Normal, because probably at the moment not many people use REST in D8.
Disruption A new formatter does not lead to any kind of disruption
CommentFileSizeAuthor
#125 2517030-125.patch4.27 KBJaesin
#113 interdiff-110-113.txt1.04 KBtedbow
#113 2517030-113.patch4.61 KBtedbow
#111 Screenshot 2016-10-28 09.44.04.png239.6 KBtedbow
#110 interdiff-108-110.txt2.36 KBtedbow
#110 2517030-110.patch4.56 KBtedbow
#108 interdiff-2517030-105-108.txt7.14 KBtedbow
#108 2517030-108.patch5.08 KBtedbow
#105 Screenshot 2016-10-25 10.51.57.png83.82 KBtedbow
#105 interdiff-2517030-96-105.txt1.42 KBtedbow
#105 2517030-105.patch8.01 KBtedbow
#97 Jump_to_conclusions____script_alert__test____script_.png39.83 KBjoelpittet
#97 Jump_to_conclusions____script_alert__test____script_.png55.53 KBjoelpittet
#96 interdiff-2517030-95-96.txt3.09 KBtedbow
#96 2517030-96.patch8.02 KBtedbow
#95 interdiff-2517030-87-95.txt3.48 KBtedbow
#95 2517030-95.patch7.87 KBtedbow
#87 interdiff.txt22.3 KBdawehner
#87 2517030-87.patch6.8 KBdawehner
#77 interdiff.txt4.49 KBamateescu
#77 2517030-77.patch26.43 KBamateescu
#72 add_a_url_formatter-2517030-72.patch23.29 KBclaudiu.cristea
#66 interdiff.txt1.14 KBclaudiu.cristea
#66 2517030-66.patch23.12 KBclaudiu.cristea
#63 interdiff.txt1.49 KBclaudiu.cristea
#63 2517030-63.patch23.69 KBclaudiu.cristea
#62 interdiff.txt19.7 KBclaudiu.cristea
#62 2517030-62.patch23.66 KBclaudiu.cristea
#57 interdiff-2517030-47-56.txt8.84 KBmglaman
#56 add_a_url_formatter_for-2517030-56.patch20.34 KBmglaman
#47 2517030-47.patch19.37 KBJaesin
#45 2517030-45.patch19.51 KBJaesin
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,667 pass(es). View
#45 interdiff-2517030_44-45.txt573 bytesJaesin
#44 interdiff-2517030_39-44.txt7.77 KBJaesin
#44 2517030-44.patch20.07 KBJaesin
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,661 pass(es). View
#41 interdiff-2517030_34-37.txt15.18 KBJaesin
#39 2517030-39.patch15.06 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,625 pass(es), 13 fail(s), and 0 exception(s). View
#39 interdiff.txt1.44 KBdawehner
#37 interdiff.txt13.16 KBdawehner
#37 2517030-37.patch15.12 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,549 pass(es), 14 fail(s), and 568 exception(s). View
#34 2517030-34.patch25.92 KBJaesin
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,932 pass(es). View
#34 interdiff-2517030_31-34.txt2.21 KBJaesin
#31 interdiff-2517030_27-31.txt1.72 KBJaesin
#31 2517030-31.patch25.7 KBJaesin
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,658 pass(es), 171 fail(s), and 9 exception(s). View
#27 2517030-27.patch24.8 KBJaesin
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php. View
#27 interdiff-2517030_24-27.txt18.82 KBJaesin
#24 interdiff-2517030-17-24.txt24.4 KBJaesin
#24 2517030-24.patch25.05 KBJaesin
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2517030-24.patch. Unable to apply patch. See the log in the details link for more information. View
#24 interdiff-2517030-19-24.txt6.89 KBJaesin
#19 interdiff-2517030-17-19.txt19.77 KBJaesin
#19 2517030-19.patch21.69 KBJaesin
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,583 pass(es), 5 fail(s), and 71 exception(s). View
#17 2517030-17.patch6.75 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,021 pass(es). View
#12 interdiff.txt805 bytesdawehner
#12 2517030-12.patch11.26 KBdawehner
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,548 pass(es), 4 fail(s), and 6 exception(s). View
#10 add_a_url_formatter_for-2517030-10.patch6.12 KBborisson_
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,578 pass(es). View
#10 interdiff.txt1.43 KBborisson_
#9 interdiff.txt3.58 KBdawehner
#9 2517030-8.patch6.15 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,577 pass(es). View
#4 interdiff.txt5.11 KBdawehner
#4 2517030-4.patch5.3 KBdawehner
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,954 pass(es). View
#1 2517030-1.patch2.63 KBJaesin
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,207 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Jaesin’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.63 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,207 pass(es). View

So the biggest argument I can think of for adding this to 8.0.x is that the default image formatter isn't very useful for REST export views displays when in field mode.

If I add the an image to the view, it comes out like:

[{
  "example_image" : "<img src=\"http:\/\/foo.com\/sites\/default\/files\/images\/foo\/bar\/example.jpg\" width=\"611\" height=\"350\" alt=\"\" title=\"Example Image\" typeof=\"foaf:Image\" \/>\n\n"
}]

When you probably just want:

[{
  "example_image" : "http://foo.com/sites/default/files/images/foo/bar/example.jpg"
}]
alexpott’s picture

Category: Feature request » Bug report

Discussed with @dawehner, @amateescu, @mrf and @jaesin we think this is an omission since when creating rest views in D8 you need to be able to expose a URL to image.

I think that this issue should also allow images to be formatted using the file formatters.

Jaesin’s picture

Title: Add a link formatter for the image field. » Add a URL formatter for the image field.
Issue summary: View changes
dawehner’s picture

FileSize
5.3 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 106,954 pass(es). View
5.11 KB

There we go.

The patch also missed a config schema ...

dawehner’s picture

Issue summary: View changes
Issue tags: -Needs tests

Remove tag, added change record

tim.plunkett’s picture

  1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,121 @@
    +      '#empty_option' => t('None (original image)'),
    ...
    +        '#markup' => $this->linkGenerator->generate($this->t('Configure Image Styles'), new Url('entity.image_style.collection')),
    

    t() vs $this->t(), here and elsewhere

    Also, instead of #markup shouldn't it be '#type' => 'link'?

  2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,121 @@
    +    $image_styles = image_style_options(FALSE);
    

    Ughhh why is this still procedural? Not in scope to fix here.

  3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,121 @@
    +      $cache_tags = $image_style->getCacheTags();
    ...
    +      $cache_tags = Cache::mergeTags($cache_tags, $image->getCacheTags());
    

    This seems wrong. It will merge the cache tags of each image into those of the next... I think this should be $image_style_cache_tags and $cache_tags = $image->getCacheTags(); $cache_tags = $image_style_cache_tags ? Cache::merge($cache_tags, $image_style_cache_tags) : $cache_tags;

mondrake’s picture

Adding related issues. Note that @alexpott in #2479487-8: ImageStyles can be deleted while having dependent configuration. indicated that the formatter should have a config dependency on the selected ImageStyle, and I believe that should happen here too.

borisson_’s picture

FileSize
2.53 KB
5.26 KB

Fixes remarks from #6.

dawehner’s picture

FileSize
6.15 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,577 pass(es). View
3.58 KB

This seems wrong. It will merge the cache tags of each image into those of the next... I think this should be $image_style_cache_tags and $cache_tags = $image->getCacheTags(); $cache_tags = $image_style_cache_tags ? Cache::merge($cache_tags, $image_style_cache_tags) : $cache_tags;

Good observation.

Adding related issues. Note that @alexpott in #2479487-8: Call to a member function transformDimensions() on a non-object indicated that the formatter should have a config dependency on the selected ImageStyle, and I believe that should happen here too.

Good idea

borisson_’s picture

FileSize
1.43 KB
6.12 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,578 pass(es). View

For some reason, the patch I added in #8 didn't get picked up.

mondrake’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -235,4 +235,18 @@ public function viewElements(FieldItemListInterface $items) {
+    if (!empty($image_style_setting)) {
+      $image_style = $this->imageStyleStorage->load($image_style_setting);
+      $dependencies['config'][] = $image_style->getConfigDependencyName();
+    }

I think we should also test that $image_style is not null, before calling methods on it. It may still be that $image_style_setting is referring to a non-existing style.

dawehner’s picture

Status: Needs work » Needs review
FileSize
11.26 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 107,548 pass(es), 4 fail(s), and 6 exception(s). View
805 bytes

There we go.

joshi.rohit100’s picture

  1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,123 @@
    +      $url = $image_style
    +        ? $image_style->buildUrl($image_uri)
    +        : file_create_url($image_uri);
    

    Shouldn't this be in one line for more readability ?

  2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,123 @@
    +      $cache_tags = $image_style
    +        ? Cache::mergeTags($image_style->getCacheTags(), $image->getCacheTags())
    +        : $image->getCacheTags();
    

    same here

Status: Needs review » Needs work

The last submitted patch, 12: 2517030-12.patch, failed testing.

devgurus’s picture

Hi, I'm new to Drupal. Don't know which of all these files should I use to patch. I'm using #10 and #12. Is this ok? Which order should I use? Because I'm not seeing any changes on the JSON returned.

joshi.rohit100’s picture

@devgurus : Start with #12 2517030-12.patch file.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.75 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,021 pass(es). View

Reroll + fix the bad merge

mondrake’s picture

Status: Needs review » Needs work
+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
@@ -0,0 +1,123 @@
+  /**
+   * {@inheritdoc}
+   */
+  public function settingsSummary() {
+    $summary = [];
+
+    $image_styles = image_style_options(FALSE);
+    // Unset possible 'No defined styles' option.
+    unset($image_styles['']);
+    // Styles could be lost because of enabled/disabled modules that defines
+    // their styles in code.
+    $image_style_setting = $this->getSetting('image_style');
+    if (isset($image_styles[$image_style_setting])) {
+      $summary[] = $this->t('Image style: @style', ['@style' => $image_styles[$image_style_setting]]);
+    }
+    else {
+      $summary[] = $this->t('Original image');
+    }
+  }
+

1 . Changing the formatter in the UI fails with an AJAX error in the browser console. We need a return $summary; at the end of this method.

2. It seems to me that the order of inheritance could be revised: now ImageUrlFormatter extends ImageFormatter, but there's quite some duplication of code. If we were to make ImageFormatter to extend ImageUrlFormatter, then ImageFormatter could call its parent's methods for the ImageStyle settings.

3. The config dependency is not showing up when trying to delete an ImageStyle. Steps:
a) create a new ImageStyle
b) associate it to the Article's Image field display
c) delete the ImageStyle

I would expect that the ImageStyle delete form will show the fact that there is a dependency on the field display entity, but there's no warning.


+++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
@@ -202,6 +202,26 @@ function _testImageFieldFormatters($scheme) {
diff --git a/core/modules/system/system.install b/core/modules/system/system.install

diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index ce06b7a..653cb5a 100644

index ce06b7a..653cb5a 100644
--- a/core/modules/system/system.install

--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install

+++ b/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -1265,6 +1265,9 @@ function system_update_8004() {

@@ -1265,6 +1265,9 @@ function system_update_8004() {
   // ensure they match the currently expected status.
   $manager = \Drupal::entityDefinitionUpdateManager();
   foreach (array_keys(\Drupal::entityManager()->getDefinitions()) as $entity_type_id) {
+    if (is_null($manager->getEntityType($entity_type_id))) {
+      $manager->getEntityType($entity_type_id);
+    }
     $manager->updateEntityType($manager->getEntityType($entity_type_id));
   }
 }

4. Unrelated?

Jaesin’s picture

Status: Needs work » Needs review
FileSize
21.69 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 109,583 pass(es), 5 fail(s), and 71 exception(s). View
19.77 KB

Hello @mondrake thanks allot for reviewing.

  1. Good catch. While manually testing I didn't see an error but I can confirm the summary doesn't show up in the UI.
  2. Since they both inherit ImageFormatterBase, It's probably better to move shred code to the ImageFormatterBase class.
  3. I think this is outside this issue and is about how image formats handle deleting a format when you don't select a replacement format.
  4. I didn't get your meaning in #4

This refactoring probably comes at a cost of needing a change record.

dawehner’s picture

Oh yeah the hunk in 4 is pointless ... was just caused by some debugging I made earlier

devgurus’s picture

Hi again. Sorry to bother, but there is no support on this apart of this patch I guess. If someone can please review the steps I take to patch it would help us a lot because is not working for us. These are the steps I take (with no success)
1- Install Drupal 8 using Acquia app.
2- Dowloand file "2517030-17.patch"
3- Using PHPStorm: VCS > Apply Patch > Select "2517030-17.patch" and apply
4- Enable RESTful Web Services and set permissions.
5- Add an image to the default Article Content Type.
6- Hit [miserver]/node/1?format=json
7- I received the json but with no path in the image field:

"field_image":[{"target_id":"1","display":null,"description":null,"alt":"Somthing","title":"","width":"1920","height":"1018"}]

I also try restarting the server in between.
Please help! I'm dessperate! What am I doing wrong? Someone can see the actual path of the image or is just a myth?
Thanks YOU!

Status: Needs review » Needs work

The last submitted patch, 19: 2517030-19.patch, failed testing.

Jaesin’s picture

Hello @devgurus, Look and ask here (https://www.drupal.org/patch/apply) for support on applying a patch.

You can also try #drupal-support on IRC as well (freenode).

Jaesin’s picture

Status: Needs work » Needs review
FileSize
6.89 KB
25.05 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2517030-24.patch. Unable to apply patch. See the log in the details link for more information. View
24.4 KB

I probably shouldn't try to hurry and get a patch out so I can go meet my friend.

This removes modifications to system.install and fixes issues with #19.

Lucasljj queued 24: 2517030-24.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 24: 2517030-24.patch, failed testing.

Jaesin’s picture

FileSize
18.82 KB
24.8 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php. View

There is no reason not to be able to make the url a link as well (especially since default is not linked) and the code is slightly simpler this way.

I've added a couple more tests.

Sorry about the crappy interdiff. (Lazy, out of time)

Lucasljj’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 27: 2517030-27.patch, failed testing.

The last submitted patch, 27: 2517030-27.patch, failed testing.

Jaesin’s picture

Status: Needs work » Needs review
FileSize
25.7 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,658 pass(es), 171 fail(s), and 9 exception(s). View
1.72 KB

I need sleep.

Status: Needs review » Needs work

The last submitted patch, 31: 2517030-31.patch, failed testing.

The last submitted patch, 31: 2517030-31.patch, failed testing.

Jaesin’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
25.92 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,932 pass(es). View

I neglected the responsive image module a bit that go around, This little update should resolve that.

Status: Needs review » Needs work

The last submitted patch, 34: 2517030-34.patch, failed testing.

Status: Needs work » Needs review

Jaesin queued 34: 2517030-34.patch for re-testing.

dawehner’s picture

FileSize
15.12 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,549 pass(es), 14 fail(s), and 568 exception(s). View
13.16 KB
  • Rerolled the patch
  • Ensured that the patch is as reviewable / small as possible
  • Merged all cacheable metadata instead of just the cache tags, just to follow the proper style, you never know, maybe max-age might be bubbled up for image styles.

Note: The interdiff.txt is caused by some horrible merge.

Status: Needs review » Needs work

The last submitted patch, 37: 2517030-37.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
15.06 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,625 pass(es), 13 fail(s), and 0 exception(s). View

Let's fix some failures

Status: Needs review » Needs work

The last submitted patch, 39: 2517030-39.patch, failed testing.

Jaesin’s picture

Status: Needs work » Needs review
FileSize
15.18 KB
+++ b/core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
@@ -116,10 +116,14 @@ public static function create(ContainerInterface $container, array $configuratio
+    ¶

I noticed this white space error in the patch.

Status: Needs review » Needs work

The last submitted patch, 39: 2517030-39.patch, failed testing.

The last submitted patch, 37: 2517030-37.patch, failed testing.

Jaesin’s picture

Status: Needs work » Needs review
FileSize
20.07 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,661 pass(es). View
7.77 KB

Link and image styles were removed from the URL formatter in the last patch.

Jaesin’s picture

FileSize
573 bytes
19.51 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,667 pass(es). View
+++ b/core/modules/image/config/schema/image.schema.yml
--- /dev/null
+++ b/core/modules/image/css/image.theme.css

+++ b/core/modules/image/css/image.theme.css
+++ b/core/modules/image/css/image.theme.css
@@ -0,0 +1,21 @@

@@ -0,0 +1,21 @@
+
+/**
+ * Image upload widget.
+ */
+.image-preview {
+  float: left; /* LTR */
+  padding: 0 10px 10px 0; /* LTR */
+}
+[dir="rtl"] .image-preview {
+  float: right;
+  padding: 0 0 10px 10px;
+}
+.image-widget-data {
+  float: left; /* LTR */
+}
+[dir="rtl"] .image-widget-data {
+  float: right;
+}
+.image-widget-data .text-field {
+  width: auto;
+}

Opps, I don't think that is supposed to be there.

dawehner’s picture

Link and image styles were removed from the URL formatter in the last patch.

Oh damnit. That merge conflict was a bit confusing indeed.

Jaesin’s picture

FileSize
19.37 KB

Re-roll:

dawehner’s picture

@jaesin
Do you think this should be sent to the 'rc target triage' or is this 8.0.x material at that point?

dawehner’s picture

Issue summary: View changes
Issue tags: +rc target triage

This issue is really useful for anyone using image styles and rest at the same time.

This issue is kinda similar to #2060677: Add target_type, target_uuid to serialized output of entity reference fields in non-HAL formats to be honest, it just makes things sane.
Added some PRO/CONs for the RC target triage

xjm’s picture

Version: 8.0.x-dev » 8.1.x-dev
Category: Bug report » Task
Status: Needs review » Postponed
Issue tags: -rc target triage

I think we can do this in contrib for 8.0.x, and target it for core in 8.1.x. It's not really a bug exactly, just one of several bits of functionality in our REST support that should be improved. Thanks for surfacing the issue!

dawehner’s picture

swentel’s picture

Status: Postponed » Active
dawehner’s picture

Status: Active » Needs review

.

dawehner’s picture

Note: git diff -M doesn't reduce the patch size.

Wim Leers’s picture

Status: Needs review » Needs work

This patch is still pretty sloppy. Needs a lot of minor touchups.

  1. +++ b/core/modules/image/config/schema/image.schema.yml
    @@ -139,6 +139,17 @@ field.formatter.settings.image:
    +  label: 'Image URL format settings'
    

    s/format/formatter/

  2. +++ b/core/modules/image/config/schema/image.schema.yml
    @@ -139,6 +139,17 @@ field.formatter.settings.image:
    +      label: 'Link image to'
    

    ?

  3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +   * Constructs an ImageFormatter object.
    

    ImageFormatterBase

  4. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +   *   The formatter label display setting.
    

    s/label display/label/ ?

  5. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +   *   Any third party settings settings.
    

    s/settings settings/settings/

  6. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +    return array(
    ...
    +    $link_types = array(
    ...
    +    $element['image_link'] = array(
    ...
    +    $summary = array();
    ...
    +    $link_types = array(
    

    s/array()/[]/

    (This is highly inconsistent in this patch.

  7. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +      $this->t('Configure Image Styles'),
    

    I'm not sure about this capitalization.

  8. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +      '#description' => $description_link->toRenderable() + [
    +          '#access' => $this->currentUser->hasPermission('administer image styles')
    +        ],
    

    Wrong indentation.

    And more importantly: this lacks cacheability metadata. Use:

    '#access' => AccessResult::allowedIfHasPermission($this->currentUser, 'administer image styles'),
    
  9. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +      '#empty_option' => t('Nothing'),
    ...
    +      $summary[] = t('Image style: @style', array('@style' => $image_styles[$image_style_setting]));
    ...
    +      'content' => t('Linked to content'),
    

    s/t()/$this->t()/

  10. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +    // Styles could be lost because of enabled/disabled modules that defines
    +    // their styles in code.
    

    Eh?

  11. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -50,4 +192,19 @@ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $item
    +    // Make sure to include 3rd party dependencies.
    

    s/3rd/third/

  12. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -50,4 +192,19 @@ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $item
    +    // Check for image style.
    ...
    +      // Add the image style dependencies as well.
    

    Need improved language.

  13. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -50,4 +192,19 @@ protected function getEntitiesToView(EntityReferenceFieldItemListInterface $item
    +  }
     }
    

    Missing newline.

  14. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,82 @@
    + * @file
    + *   Contains \Drupal\image\Plugin\Field\FieldFormatter\ImageUrlFormatter.
    

    Indentation problem

  15. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,82 @@
    +    // Url to be linked to.
    ...
    +      // Set the link url if settings require such.
    ...
    +      // Add a link if we have a valid link url.
    

    s/Url/URL/

  16. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,82 @@
    +        $cacheable_metadata = $cacheable_metadata->merge(CacheableMetadata::createFromObject($image_style));
    

    s/merge()/addCacheableDependency()/

mglaman’s picture

Status: Needs work » Needs review
FileSize
20.34 KB

Re-roll based on review in #55

mglaman’s picture

FileSize
8.84 KB

Sorry :/ forgot interdiff, here it is.

Status: Needs review » Needs work

The last submitted patch, 56: add_a_url_formatter_for-2517030-56.patch, failed testing.

amateescu’s picture

  1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatterBase.php
    @@ -7,14 +7,156 @@
    +      $this->$this->t('Configure image styles'),
    

    :P

  2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,95 @@
    +    // The URL formatter does not support image_link.
    +    unset($element['image_link']);
    

    Doesn't this mean we need to remove it from the default settings as well?

dawehner’s picture

+++ b/core/modules/image/config/schema/image.schema.yml
@@ -141,11 +141,8 @@ field.formatter.settings.image:
-    image_link:
-      type: string
-      label: 'Link image to'
     image_style:

This was not meant to be removed ... I'm sorry wim, but this is exactly how the label is defined for the other image formatter ...

dawehner’s picture

Doesn't this mean we need to remove it from the default settings as well?

Good point.

claudiu.cristea’s picture

Notes:

  • It's senseless to link a URL, pointing to an image, to a node. So, if there's a clickable link, it should go to the image (original or image style). Other special cases should be treated in contrib or custom modules.
  • Clickable links need a cutoff setting
  • There must be possible the get the URL as plain text.
claudiu.cristea’s picture

FileSize
23.69 KB
1.49 KB

Fix t() for code stolen from link.module

The last submitted patch, 62: 2517030-62.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 63: 2517030-63.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
23.12 KB
1.14 KB

Let's not add in this patch the dependency computing, that is not related and it's a more complex task (it needs update path, resolving dependencies onRemovalDependencies() and so on). That is resolved in #2479487: ImageStyles can be deleted while having dependent configuration.. Let's not mix them.

dawehner’s picture

Thank you for taking it over @claudiu.cristea
Do you like the patch as it is now?

claudiu.cristea’s picture

@dawehner, yes, but this needs a good review from the field team - @swentel, @amateescu? :)

jibran’s picture

Looks ready to me.

amateescu’s picture

Title: Add a URL formatter for the image field. » Add a URL formatter for the image field
Status: Needs review » Reviewed & tested by the community

Looks ready to me as well.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 66: 2517030-66.patch, failed testing.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
23.29 KB

Reroll.

claudiu.cristea’s picture

Status: Needs review » Reviewed & tested by the community

And back to RTBC as per #70, if passes.

dawehner’s picture

Back to RTBC

mondrake’s picture

Shouldn't ImageFormatter::calculateDependencies() and ImageFormatter::onDependencyRemoval() just introduced in #2479487: ImageStyles can be deleted while having dependent configuration. be also moved to ImageFormatterBase? Otherwise ImageUrlFormatter will again miss the image style dependency?

mondrake’s picture

Status: Reviewed & tested by the community » Needs work
amateescu’s picture

Status: Needs work » Needs review
FileSize
26.43 KB
4.49 KB

Yup :)

claudiu.cristea’s picture

Status: Needs review » Needs work

Unfortunately, now we need to take care also of \Drupal\responsive_image\Plugin\Field\FieldFormatter\ResponsiveImageFormatter because that, by a bad design decision, has replaced the setting 'image_style' with 'responsive_image_style'. This is messy also for ResponsiveImageFormatter::defaultSettings(), see how is implemented, without inheriting anything from parent.

Shouldn't' we return to 'image_style' there in a followup? Of course, by keeping BC and implementing an upgrade path.

claudiu.cristea’s picture

Assigned: Jaesin » Unassigned
claudiu.cristea’s picture

Forget everything I said in #78. I was wrong. In fact responsive image formatter not having the 'image_style' setting, will not implement any dependency. However, we need to implement dependencies for responsive image formatter thus I opened an new issue for that: #2657110: ResponsiveImageFormatter should define dependencies to responsive image style entity.

Setting back to NR for #77. Any reviewers?

mondrake’s picture

Looks pretty much OK now, the only thing I feel suggesting is to implement methods for ::calculateDependencies and ::onDependencyRemoval in ResponsiveImageFormatter, with just a @todo referring to #2657110: ResponsiveImageFormatter should define dependencies to responsive image style entity to stop inherithing the behavior from ImageFormatterBase which is not wanted there.

claudiu.cristea’s picture

@mondrake, that will not inherit dependencies from ImageFormatterBase. More clear, dependencies calculated by the parent ImageFormatterBase are an empty array because ResponsiveImageFormatter has no 'image_style' settings and, as effect, will not add dependencies. ImageFormatterBase adds dependencies only IF 'image_style' exists and it's valid style entity. The same happens in onDependencyRemoval. BTW, you can take a look also to #2657110: ResponsiveImageFormatter should define dependencies to responsive image style entity :)

mondrake’s picture

OK, back to RTBC then. Was RTBC in #70 and #74, the changes in #77 add image style dependencies also for the new ImageUrlFormatter.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I don't think the refactor of ImageFormatterBase is in the spirit of our BC promise - any module extending this abstract class would have to make extensive changes to do the current patch - how about we just make ImageUrlFormatter extend ImageFormatter and you'll get most of the code reuse you want without have to make changes to the responsive image formatter as well. Yes it would be nice but abstract base classes are specifically mentioned as not being @internal by design.

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
@@ -0,0 +1,139 @@
+    /**
+     * @var \Drupal\file\Entity\File[] $images
+     * @var \Drupal\Core\Field\EntityReferenceFieldItemListInterface $items
+     */

This looks super weird this is against coding standards for inline comments lets just do:
/* @var.. */.

nketchum’s picture

Should the image url formatter output match the how file url formats are structured, which ONLY include the internal drupal path, without the protocol/domain/subdir prefixed to it?

I can see, and currently have, many use-cases where excluding the host/domain portion "https://my.domain.com/drupal-subdir/" in file/image URLs definitely help simplify development.

As it stands now:
Image URL formatter output: http://my.drupal.site/drupal-subdir/sites/default/files/myfile.jpg
File URL formatter gives me this: /sites/default/files/myfile.jpg

Thoughts? Should the output be structured similarly?

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.8 KB
22.3 KB

So what something more like this?

Status: Needs review » Needs work

The last submitted patch, 87: 2517030-87.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dinarcon’s picture

Hello,

The https://www.drupal.org/project/image_url_formatter module seems to be solving this in contrib for now. It would be great to have this functionality built into core though.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

michaellenahan’s picture

I notice that #87 removes the 'url_link' and 'trim_length' functionality (originally added in #62) from the ImageUrlFormatter settings.

Is this intentional? Or should we add it back?

e0ipso’s picture

We are using a workaround in the JSON API module that will add a computed-without-storage field that will provide the download URL of any file entity.

See #2793809: [FEATURE] Provide direct download URLs for files and images for more details.

mattferderer’s picture

Just adding another pro reason for this. I'm trying to implement a responsive image block that uses multiple image fields. It uses multiple image fields because the desktop is a wide ratio & the mobile is not as wide. I can't create a responsive image style that uses multiple image fields & I don't want one image to just be cropped. Ideally this functionality would allow me to grab the url path from the selected image style & output that into a view block field that I rewrite the output to be an img srcset with multiple image fields placed in.

Fortunately the Image URL Formatter module handles this use case for me perfectly.

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.87 KB
3.48 KB

Ok I was getting a few different test fails locally.
This patch

  1. Adds logic to output as a link if Provide Link option is checked
  2. Adds trim_length setting. This seems to me to only make sense if you are using Provide Link. So I hid the option unless that is checked. This was already being tested for so it made those pass for me.
  3. Replaced t() with $this->t()
  4. Changed to short array syntax in few places
tedbow’s picture

This patch

  1. Updated \Drupal\image\Plugin\Field\FieldFormatter\ImageUrlFormatter::settingsSummary to output all settings.
  2. Removed unneeded @file block
  3. Code formatting clean up
joelpittet’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
55.53 KB
39.83 KB

Minor thing but could it be 'URL of image' instead of 'URL to image' would be easier to understand? Could be just me reading it like casting from this to that.

And there is a small nitpick double ';;' in the formatter class and mixed short and long array syntax around array('@limit' =>.

Those could be fixed on commit unless you want a bit more clean-up.

jump
jump

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs change record

This does not provide the image style URL(s) for an \Drupal\image\Plugin\Field\FieldType\ImageItem field when serialized to REST.

So I don't see how this is true:

This enables more people to use REST without additional work

I also am not sure this needs to be exposed as a new formatter. If people are sure, then please provide use cases.

tstoeckler’s picture

@Wim Leers: Because formatters are available in Views, this allows exposing the Image style URL with a Rest Export view.

mglaman’s picture

#99 is the main use case. In working on my book I hit this issue and had to reference this issue node for people wanting to use Views to build a REST export that has images which aren't' just chunks of HTML.

tstoeckler’s picture

Issue summary: View changes
Wim Leers’s picture

Issue tags: +API-First Initiative

I think I need to take another look at how REST export displays work in Views. Because this would mean that the JSON for e.g. a node read via /node/123?_format=json versus /view-listing-nodes?_format=json would be completely different, which is bad for RX.

Isn't anybody else concerned about that?

tstoeckler’s picture

@Wim Leers: Regardless of whether one thinks this is a good pattern or not, the entire point of the Views Rest Exports is to be able to click together a custom collection of (possibly aliased) fields to - in theory - serve as a backend for e.g. a frontend app without ever having to touch code. I'm not the biggest advocate of this in the world, but I do think it can be useful in some cases. And for those people using it, this issue - among others - makes the feature a lot more useful.

Wim Leers’s picture

Status: Needs review » Needs work

#103: ok.

  1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,143 @@
    + *   label = @Translation("URL to image"),
    

    Agreed with joelpittet that this reads/sounds weird. "URL of image" would be better". IMO it can be just "Image URL"

  2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,143 @@
    +    $element['url_link'] = [
    +      '#type' => 'checkbox',
    +      '#title' => $this->t('Provide link'),
    +      '#description' => $this->t('If checked, the URL to image will get a click-able link otherwise, the plain URL will be displayed.'),
    +      '#default_value' => $this->getSetting('url_link'),
    +    ];
    +    $element['trim_length'] = [
    +      '#type' => 'number',
    +      '#title' => $this->t('Trim link text length'),
    +      '#field_suffix' => $this->t('characters'),
    +      '#default_value' => $this->getSetting('trim_length'),
    +      '#min' => 1,
    +      '#description' => $this->t('Leave blank to allow unlimited link text lengths.'),
    +      '#states' => [
    +        'invisible' => [
    +          ":input[name=\"fields[{$this->fieldDefinition->getName()}][settings_edit_form][settings][url_link]\"]" => ['checked' => FALSE],
    +        ],
    +      ],
    +    ];
    

    What's the value of these settings, if this formatter is only intended for use with REST?

  3. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,143 @@
    +      // Add cacheable metadata from the image and image style.
    

    Nit: s/cacheable/cacheability/

  4. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,143 @@
    +      $cacheable_metadata = CacheableMetadata::createFromObject($image);
    

    Nit: s/$cacheable_metadata/$cacheability/

tedbow’s picture

Status: Needs work » Needs review
FileSize
8.01 KB
1.42 KB
83.82 KB

@Wim Leers thanks for review
1. I think that we should leave as "URL to image" because that is what \Drupal\file\Plugin\Field\FieldFormatter\UrlPlainFormatter uses, "URL to file". If you are on manage display or in Views and you want to have both your file field and image field have the same type of URL formatter it is reasonable to think similar wording in the UI will have similar output. If the wording is that confusing we should have follow up issue to fix it in both places.

2. I wrote this part because the tests and schema were already created in the patch. I could argue for keeping them because I could see cases where you would actually want to use this formatter in your site outside of REST and want to make the Image url a link.

3,4 fixed nits.

Relate to point 1 above. I was playing around with this formatter and the UrlPlainFormatter for files on the same content type.

As you can see the file formatter does not include the host and the image formatter does.
It seems this should be the same. Especially if we are thinking we want to expose this for REST.
Should we make the image URL not include the host?

dawehner’s picture

Yeah, the more I think about it, the less it really makes sense to have these options

  • If you use the formatter in the context of REST, you don't want HTML, you want the raw data, so you don't need link/trimming
  • If you use the formatter in the context of HTML, IMHO people should just use less configuration but rather do more things on the template level

Given that it is probably better to remove those options.

Wim Leers’s picture

Status: Needs review » Needs work

#106++ — that's exactly what I was thinking.

Less configuration options = less potential for bugs = easier to maintain.

If the need arises, we can still add those options.

tedbow’s picture

Status: Needs work » Needs review
FileSize
5.08 KB
7.14 KB

Ok because 105,106 removed the extra options and testing of those options.

@Wim Leers, @dawehner or others any opinion on #105 point that image url includes host but file url does not.

Wim Leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,95 @@
    +  public function settingsSummary() {
    +    $summary = [];
    +
    +    $image_styles = image_style_options(FALSE);
    +    // Unset possible 'No defined styles' option.
    +    unset($image_styles['']);
    +    // Styles could be lost because of enabled/disabled modules that defines
    +    // their styles in code.
    +    $image_style_setting = $this->getSetting('image_style');
    +    if (isset($image_styles[$image_style_setting])) {
    +      $summary[] = $this->t('Image style: @style', ['@style' => $image_styles[$image_style_setting]]);
    +    }
    +    else {
    +      $summary[] = $this->t('Original image');
    +    }
    +
    +    return $summary;
    +  }
    

    AFAICT this can be updated to call the parent implementation and remove the last item in the received $summary array.

    Less code to maintain here.

  2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,95 @@
    +      $url = $image_style ? $image_style->buildUrl($image_uri) : file_create_url($image_uri);
    

    If this is not going to use file_url_transform_relative(), then it should be documented why. This probably should use that though.

  3. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
    @@ -199,6 +200,23 @@ function _testImageFieldFormatters($scheme) {
    +        'url_link' => FALSE,
    +        'trim_length' => 80,
    

    These can be deleted.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
2.36 KB

1. Fixed. Unless the $settings is corrupt we should just be able to return parent::settingsSummary(). Because $this->getSetting('image_link') should always be empty. But just to careful we should only return the [$summary[0]]
2. Fixed. I think we should use file_url_transform_relative() because my final point in #105 the url should be relative just like the file url formatter.
3. Fixed

tedbow’s picture

I found a problem with using this formatter through the browser. I know this meant for REST but we can't exclude that people will use this for site display.


Here you can see that because of css selector

.node .field--type-image {
    float: left;
    margin: 0 1em 0 0;
}

The image field is floated even if it is not an image element.

Don't know if this import enough to fix but thought I would point it out.

Status: Needs review » Needs work

The last submitted patch, 110: 2517030-110.patch, failed testing.

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.61 KB
1.04 KB

in #110 I forgot to add file_url_transform_relative to the tests.

Fixed

joelpittet’s picture

re #111 @tedbow that's a good point luckily it's in bartik only from a quick grep. Bartik is non API so we can fix that. Could you open a follow-up issue for that against the bartik theme?

tedbow’s picture

dawehner’s picture

I like how this looks now. Thank you @tedbow
I'm confused that https://www.drupal.org/node/2517030#comment-10254713 mentions a change record, but I'm not able to find it. Again my browser messes up the project association again :)

tedbow’s picture

Issue summary: View changes

Added the change record: https://www.drupal.org/node/2824621

Also just wanted to note in #108 I removed the option to make this formatter a link.
but then in looking to make a change record I saw this in the issue summary:

Sometimes an image field will be used as an image in one view mode (full) and as a link in a view or another view mode (teaser).

Seems like a link was on of original intention of this issue. Checked the history and this has been there since the beginning.

Maybe it is okay to take out but just wanted point that out incase anyone thought it was mistake. I updated the Issue Summary to take a out the reference to "link"

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

I agree this now looks good.

  1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
    @@ -0,0 +1,82 @@
    +    /** @var \Drupal\file\Entity\File[] $images */
    

    Nit: should be typehinted to \Drupal\file\FileInterface.

  2. +++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
    @@ -88,6 +88,7 @@ function _testImageFieldFormatters($scheme) {
    +    /** @var \Drupal\node\Entity\Node $node */
    

    \Drupal\node\NodeInterface

Wim Leers’s picture

alexpott’s picture

Saving issue credits

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 10fead7 and pushed to 8.3.x. Thanks!

diff --git a/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
index 8c8ff78..1dea1f8 100644
--- a/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageUrlFormatter.php
@@ -61,7 +61,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
 
     /** @var \Drupal\image\ImageStyleInterface $image_style */
     $image_style = $this->imageStyleStorage->load($this->getSetting('image_style'));
-    /** @var \Drupal\file\Entity\File[] $images */
+    /** @var \Drupal\file\FileInterface[] $images */
     foreach ($images as $delta => $image) {
       $image_uri = $image->getFileUri();
       $url = $image_style ? $image_style->buildUrl($image_uri) : file_create_url($image_uri);
diff --git a/core/modules/image/src/Tests/ImageFieldDisplayTest.php b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
index d7e6623..f3ce858 100644
--- a/core/modules/image/src/Tests/ImageFieldDisplayTest.php
+++ b/core/modules/image/src/Tests/ImageFieldDisplayTest.php
@@ -88,7 +88,6 @@ function _testImageFieldFormatters($scheme) {
     // Save node.
     $nid = $this->uploadNodeImage($test_image, $field_name, 'article', $alt);
     $node_storage->resetCache(array($nid));
-    /** @var \Drupal\node\Entity\Node $node */
     $node = $node_storage->load($nid);
 
     // Test that the default formatter is being used.

Fixes made on commit.

  • alexpott committed 10fead7 on 8.3.x
    Issue #2517030 by Jaesin, dawehner, tedbow, claudiu.cristea, mglaman,...
Wim Leers’s picture

Published CR.

Status: Fixed » Closed (fixed)

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

Jaesin’s picture