Updated: Comment #46

Problem/Motivation

Image objects are currently a stdClass with arbitrary properties and arrays of metadata, and a handful of procedural functions in image.inc.

Proposed resolution

Provide an interface for image objects, with getters and setters and real methods.

Remaining tasks

N/A

User interface changes

N/A

API changes

image_load() had been replaced by Drupal::service('image.factory')
image_get_info() and the properties it returned as an array now have specific getters and setters.

#2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class

CommentFileSizeAuthor
#64 image-2033669-64.patch114.27 KBtim.plunkett
#64 interdiff.txt1.71 KBtim.plunkett
#50 image-2033669-50.patch114.34 KBtim.plunkett
#44 image-class-2033669.44.interdiff.txt10.68 KBlarowlan
#44 image-class-2033669.44.patch113.88 KBlarowlan
#44 Screen Shot 2013-07-29 at 6.04.41 PM.png128.14 KBlarowlan
#40 image-class-2033669.39.interdiff.txt9.68 KBlarowlan
#40 image-class-2033669.39.patch110.62 KBlarowlan
#40 coverage.html_.txt114.14 KBlarowlan
#38 image-2033669-38.patch102.17 KBtim.plunkett
#38 interdiff.txt8.01 KBtim.plunkett
#37 image-2033669-37.patch106.71 KBtim.plunkett
#37 interdiff.txt8.26 KBtim.plunkett
#35 image-2033669-35.patch106.81 KBtim.plunkett
#35 interdiff.txt65.83 KBtim.plunkett
#27 image-2033669-27.patch98.07 KBtim.plunkett
#27 interdiff.txt46.52 KBtim.plunkett
#25 image-2033669-23.patch93.35 KBtim.plunkett
#21 image-2033669-21.patch193.23 KBtim.plunkett
#21 interdiff.txt24.7 KBtim.plunkett
#20 image-2033669-20-standalone-do-not-test.patch75.03 KBtim.plunkett
#20 image-2033669-20.patch175.67 KBtim.plunkett
#17 image-2033669-17.patch221.89 KBtim.plunkett
#17 image-2033669-17-standalone-do-not-test.patch120.81 KBtim.plunkett
#12 image-2033669-12.patch177.22 KBtim.plunkett
#9 image-2033669-9.patch177.22 KBtim.plunkett
#9 interdiff.txt13.57 KBtim.plunkett
#7 image-2033669-7.patch174.19 KBtim.plunkett
#7 interdiff.txt32.78 KBtim.plunkett
#1 image-2033669-1-combined.patch162 KBtim.plunkett
#1 image-2033669-1-do-not-test.patch52.49 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
52.49 KB
162 KB

Here it is combined with the image effects patch.

larowlan’s picture

Status: Needs review » Needs work

+1

claudiu.cristea’s picture

Related #204497: Make the background fill color configurable where image styles result in blank space.

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/RotateImageEffect.phpundefined
@@ -0,0 +1,132 @@
+  /**
+   * Validates to ensure a hexadecimal color value.
+   */
+  public function validateColorEffect(array $element, array &$form_state) {
+    if ($element['#value'] != '') {
+      if (!preg_match('/^#[0-9A-F]{3}([0-9A-F]{3})?$/', $element['#value'])) {
+        form_error($element, t('!name must be a hexadecimal color value.', array('!name' => $element['#title'])));
+      }
+    }
+  }

You should use \Drupal\Core\Utility\Color::validateHex() for validation. I agree that this is faster :)
Off topic: Shouldn't \Drupal\Core\Utility\Color be under \Drupal\Component?

tim.plunkett’s picture

That's code from the other issue, and it's just moved around, not added there either.

claudiu.cristea’s picture

I know, but it's a chance to clean.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
32.78 KB
174.19 KB

More progress. Still contains the blocker.

tim.plunkett’s picture

Status: Needs review » Needs work
FileSize
13.57 KB
177.22 KB

Okay. We might be done here.
Still waiting on the blocker.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
177.22 KB
andypost’s picture

andypost’s picture

+++ b/core/lib/Drupal/Core/Image/ImageFile.phpundefined
@@ -0,0 +1,439 @@
+  protected function getToolkit() {
+    if (!$this->toolkit) {
+      $this->toolkit = \Drupal::service('image.toolkit');

Without injection?

+++ b/core/modules/image/image.moduleundefined
@@ -476,11 +481,11 @@ function image_style_deliver($style, $scheme) {
-    $image = image_load($derivative_uri);
-    $uri = $image->source;
+    $image = new ImageFile($derivative_uri);
+    $uri = $image->getSource();
...
+      'Content-Type' => $image->get('mime_type'),
+      'Content-Length' => $image->get('file_size'),
...
     return new BinaryFileResponse($uri, 200, $headers);

This method should have controller after #2027423: Make image style system flexible

tim.plunkett’s picture

If you want to inject your own, you can call setToolkit(). Some code already does that.

Yeah, there are 3 or 4 issues that will all conflict, but only in the patch-sense, not the architecture-sense.

tim.plunkett’s picture

I've rerolled this on top of #2027423: Make image style system flexible as well, since it all touched the same code and that was RTBC.

tim.plunkett’s picture

Here it is containing both of those other issues.
Also attached is a standalone patch for anyone wanting to take a look at only this issue's changes.

tim.plunkett’s picture

Status: Needs review » Postponed

That last fail was fixed in the blocker issue used in that patch.
Please don't bother rerolling this as a patch, I'm working on this in http://drupalcode.org/sandbox/tim.plunkett/1698392.git/shortlog/refs/hea... and rerolling it on top of #2027423: Make image style system flexible and #1821854: Convert image effects into plugins in a very specific order

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
175.67 KB
75.03 KB

First blocker went in, pretty happy so far with the second (last) blocker, so here's a reroll.
Will repostpone again later.

tim.plunkett’s picture

FileSize
24.7 KB
193.23 KB

image.inc is finally dead! No more patches for now until the blocker is in, even if this fails.

tim.plunkett’s picture

+++ b/core/includes/common.incundefined
@@ -3083,7 +3083,6 @@ function _drupal_bootstrap_code() {
-  require_once __DIR__ . '/image.inc';

Best line in the patch, IMO :)

tim.plunkett’s picture

FileSize
93.35 KB

Rerolled, this has no more blockers.

jibran’s picture

Awesome work @tim.plunkett. It is green. I want to RTBC it but let others have a look.

tim.plunkett’s picture

FileSize
46.52 KB
98.07 KB

neclimdul didn't like the name ImageFile, suggested Image
I didn't want to conflict with \Drupal\Component\Image\Image
I realized it had only 1 method, which is static, and is only used twice, once by ImageFile

I moved the static method into ImageFile itself
I deleted \Drupal\Component\Image\Image
I renamed \Drupal\Core\Image\ImageFile to \Drupal\Core\Image\Image

This changed one Image::scaleDimensions() call to a static::scaleDimensions(), and the other change was in a file that already use'd ImageFile.

tim.plunkett’s picture

I'll try to fill one in soon.

larowlan’s picture

+++ b/core/lib/Drupal/Core/Image/Image.phpundefined
@@ -0,0 +1,529 @@
+    $this->source = $source;

Shouldn't we be sure to clear $this->resource when setting a new source?

+++ b/core/lib/Drupal/Core/Image/Image.phpundefined
@@ -0,0 +1,529 @@
+    $this->toolkit = $toolkit;

Same, if we change the toolkit, shouldn't we clear the resource, which could have been from a different toolkit?

Also I was expecting to see a new unit test for the Image class.

tim.plunkett’s picture

I'm not sure. The previous code didn't do that.

Unit tests aren't my specialty. Any volunteers?

larowlan’s picture

I might take a crack at it tonight/tomorrow, been looking for an excuse to tinker with http://phpunit.de/manual/3.7/en/skeleton-generator.html

larowlan’s picture

+++ b/core/modules/image/lib/Drupal/image/ImageEffectInterface.phpundefined
@@ -17,13 +18,13 @@
+  public function applyEffect(Image $image);

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/CropImageEffect.phpundefined
@@ -24,17 +25,17 @@ class CropImageEffect extends ResizeImageEffect {
+  public function applyEffect(Image $image) {

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/DesaturateImageEffect.phpundefined
@@ -31,9 +32,9 @@ public function transformDimensions(array &$dimensions) {
+  public function applyEffect(Image $image) {

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ResizeImageEffect.phpundefined
@@ -26,9 +27,9 @@ class ResizeImageEffect extends ImageEffectBase implements ConfigurableImageEffe
+  public function applyEffect(Image $image) {

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/RotateImageEffect.phpundefined
@@ -26,7 +27,7 @@ class RotateImageEffect extends ImageEffectBase implements ConfigurableImageEffe
+  public function applyEffect(Image $image) {

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ScaleAndCropImageEffect.phpundefined
@@ -24,9 +25,9 @@ class ScaleAndCropImageEffect extends ResizeImageEffect {
+  public function applyEffect(Image $image) {

+++ b/core/modules/image/lib/Drupal/image/Plugin/ImageEffect/ScaleImageEffect.phpundefined
@@ -25,7 +25,7 @@ class ScaleImageEffect extends ResizeImageEffect {
+  public function applyEffect(Image $image) {

+++ b/core/modules/image/tests/modules/image_module_test/lib/Drupal/image_module_test/Plugin/ImageEffect/NullTestImageEffect.phpundefined
@@ -24,7 +25,7 @@ class NullTestImageEffect extends ImageEffectBase {
+  public function applyEffect(Image $image) {

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.phpundefined
@@ -50,33 +51,34 @@ public function settingsFormSubmit($form, &$form_state) {
+  public function rotate(Image $image, $degrees, $background = NULL) {

@@ -84,88 +86,91 @@ public function rotate($image, $degrees, $background = NULL) {
+  public function crop(Image $image, $x, $y, $width, $height) {
...
+  public function desaturate(Image $image) {
...
+  public function load(Image $image) {

@@ -174,7 +179,7 @@ public function load($image) {
+  public function save(Image $image, $destination) {

@@ -214,9 +219,9 @@ public function save($image, $destination) {
+  public function getInfo(Image $image) {

@@ -245,16 +250,16 @@ public function getInfo($image) {
+  public function createTmp(Image $image, $width, $height) {

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkitInterface.phpundefined
@@ -42,15 +72,13 @@ function settingsFormSubmit($form, &$form_state);
+  function resize(Image $image, $width, $height);

@@ -64,15 +92,13 @@ function resize($image, $width, $height);
+  function rotate(Image $image, $degrees, $background = NULL);

@@ -89,56 +115,50 @@ function rotate($image, $degrees, $background = NULL);
+  function crop(Image $image, $x, $y, $width, $height);
...
+  function desaturate(Image $image);
...
+  function load(Image $image);
...
+  function save(Image $image, $destination);

@@ -149,9 +169,9 @@ function save($image, $destination);
+  function getInfo(Image $image);

+++ b/core/modules/system/lib/Drupal/system/Tests/Image/ToolkitGdTest.phpundefined
@@ -79,15 +80,15 @@ function colorsAreEqual($color_a, $color_b) {
+  function getPixelColor(Image $image, $x, $y) {

+++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.phpundefined
@@ -38,7 +39,7 @@ public function settingsFormSubmit($form, &$form_state) {}
+  public function getInfo(Image $image) {

@@ -46,7 +47,7 @@ public function getInfo($image) {
+  public function load(Image $image) {

@@ -54,7 +55,7 @@ public function load($image) {
+  public function save(Image $image, $destination) {

@@ -64,7 +65,7 @@ public function save($image, $destination) {
-  public function crop($image, $x, $y, $width, $height) {
+  public function crop(Image $image, $x, $y, $width, $height) {

@@ -72,7 +73,7 @@ public function crop($image, $x, $y, $width, $height) {
+  public function resize(Image $image, $width, $height) {

@@ -80,7 +81,7 @@ public function resize($image, $width, $height) {
+  public function rotate(Image $image, $degrees, $background = NULL) {

@@ -88,7 +89,7 @@ public function rotate($image, $degrees, $background = NULL) {
+  public function desaturate(Image $image) {

I really think we should be type-hinting an interface here, which means we need to add one.

larowlan’s picture

Unless of course something that extends Image is fine, in which case ignore me

tim.plunkett’s picture

I didn't bother with an interface because we hardcode the class anyway.

If we wanted a factory service that's one thing, but that's way more flexibility than image.inc gives us now, so I didn't bother.

tim.plunkett’s picture

FileSize
65.83 KB
106.81 KB

Had a long talk with msonnabaum, Crell, and katbailey about calling new Image() everywhere, and this is what I came up with.

-  $original_image = image_get_info($original_path);
+  $original_image = Drupal::image($original_path);

Obviously OO code is using the injected version.
It directly mimics the config.factory service.

tim.plunkett’s picture

FileSize
8.26 KB
106.71 KB

Missed a use statement, and msonnabaum pointed out that the Drupal::image wrapper was overkill.

tim.plunkett’s picture

FileSize
8.01 KB
102.17 KB

#2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class made me realize that moving the scaleDimensions method in was stupid. Moving it back...

tim.plunkett’s picture

Issue tags: +Kill includes

.

larowlan’s picture

So here's the unit tests.
Now I spent about 30 mins tearing my hair out as to why the testCropWidth and testCropHeight methods didn't pass and finally figured out it was the code in Drupal\Core\Image\Image, not my test.
So I wonder if this passes.
If it does, it means we've found a bug thanks to unit-testing.

Also attached code coverage report.

jibran’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Image/Image.phpundefined
@@ -0,0 +1,347 @@
+use \Drupal\Component\Utility\Image as ImageUtility;

Extra \

+++ b/core/modules/image/lib/Drupal/image/Plugin/Core/Entity/ImageStyle.phpundefined
@@ -277,7 +277,8 @@ public function createDerivative($original_uri, $derivative_uri) {
+    $image = \Drupal::service('image.factory')->get($original_uri);

Can we inject here?

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.phpundefined
@@ -50,33 +51,34 @@ public function settingsFormSubmit($form, &$form_state) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::resize().
...
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::rotate().

@@ -84,88 +86,91 @@ public function rotate($image, $degrees, $background = NULL) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::crop().
...
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::desaturate().
...
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::load().

@@ -174,7 +179,7 @@ public function load($image) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::save().

@@ -214,9 +219,9 @@ public function save($image, $destination) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::getInfo().

@inheritdoc

+++ b/core/modules/system/tests/modules/image_test/lib/Drupal/image_test/Plugin/ImageToolkit/TestToolkit.phpundefined
@@ -38,15 +39,30 @@ public function settingsFormSubmit($form, &$form_state) {}
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::load().

@@ -54,7 +70,7 @@ public function load($image) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::save().

@@ -64,7 +80,7 @@ public function save($image, $destination) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::crop().

@@ -72,7 +88,7 @@ public function crop($image, $x, $y, $width, $height) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::resize().

@@ -80,7 +96,7 @@ public function resize($image, $width, $height) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::rotate().

@@ -88,7 +104,7 @@ public function rotate($image, $degrees, $background = NULL) {
    * Implements \Drupal\system\Plugin\ImageToolkitInterface::desaturate().

@inheritdoc

+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.phpundefined
@@ -0,0 +1,293 @@
+    $source = __DIR__ . '/../../../../../misc/druplicon.png';
...
+    $source = __DIR__ . '/../../../../../misc/grippie.png';

Is there any other way to write this?

tim.plunkett’s picture

Status: Needs work » Needs review

@larowlan thanks for the tests! That logic comes straight from image_crop(), and those lines had never been changed until now (added in #373613: Create "Image" Objects, Operate on Images By Resource).

@jibran thanks for the review!
I don't touch any of the Implements ImageToolkitInterface lines, not even to move them, so I'm not going to make the patch bigger by changing them.
If you grep for "__DIR__ . '/../", that's apparently a pattern used elsewhere.

We can't inject into entity classes yet.

I'll fix the leading \ in the use statement in the next patch (or @larowlan can), I'll wait for another review.

larowlan’s picture

+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.phpundefined
@@ -0,0 +1,293 @@
+    $toolkit = $this->getMockBuilder('Drupal\system\Plugin\ImageToolkit\GDToolkit')
+      ->disableOriginalConstructor()
+      ->getMock();
+    // This will fail if save() method isn't called on the toolkit.
+    $toolkit->expects($this->once())

we can use $this->toolkit here and ditch the first hunk. And if we can configure 'save' method to return TRUE we'll improve the coverage

larowlan’s picture

Fixes issues from #43 and #41 except for the inject (can't do) and the ../../ which is an existing pattern.
New code coverage attached. 100% excluding the chmod method which is temporary until such time as we get #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class
Screen Shot 2013-07-29 at 6.04.41 PM.png

jibran’s picture

Thanks @larowlan. I think it is ready to fly. Let's wait for bot ;). This now needs issue summary update :).

jibran’s picture

Issue summary: View changes

Updated issue summary.

tim.plunkett’s picture

Updated.

Also, for the eventual change notice, this is a perfect hunk to show off the changes:

     if ($success) {
-      $image = image_load($derivative_uri);
-      $uri = $image->source;
+      $image = $this->imageFactory->get($derivative_uri);
+      $uri = $image->getSource();
       $headers += array(
-        'Content-Type' => $image->info['mime_type'],
-        'Content-Length' => $image->info['file_size'],
+        'Content-Type' => $image->getMimeType(),
+        'Content-Length' => $image->getFileSize(),
       );
       return new BinaryFileResponse($uri, 200, $headers);
     }
jibran’s picture

Status: Needs review » Reviewed & tested by the community

@tim.plunkett Thanks for updating issue summary. I think it is safe to RTBC an issue on which @tim.plunkett and @larowlan has been working.

xjm’s picture

Issue tags: -Kill includes

#44: image-class-2033669.44.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +Kill includes

The last submitted patch, image-class-2033669.44.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
114.34 KB

Reroll, it just conflicted with #2033383: Provide a default plugin bag on some use statements and no actual code, so setting back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Since we're past API freeze, and this doesn't seem to be resolving any kind of critical issue, the BC layer needs to be left in (as one-liner wrapper functions or so) and marked @deprecated: http://buytaert.net/drupal-8-api-freeze

alexpott’s picture

Issue tags: +API change

@webchick - @catch and I have been discussing this change - should have posted here earlier. And yes whilst this is not resolving a "critical issue" in terms of bugs, for my perspective it is solving an important issue with consistency. Using stdClass to create objects on the fly in Drupal 8 really ought to be frowned upon. Making image file object have an interface make this sub system much easier to change in the future and to develop upon in the present.

Also in this changes favour is that as @catch has said "It's another one where I can't really see any modules using the API that's getting changed..."

For example:
http://drupalcontrib.org/api/drupal/drupal%21includes%21image.inc/functi... - 5 contrib modules in D7 using image_get_info()
http://drupalcontrib.org/api/drupal/drupal%21includes%21image.inc/functi... - 3 contrib modules in D7 using image_load()

If we leave the backwards compatibility layer in we also remove of the advantages of this patch - we're no longer calling require_once on image.inc on every request - even those that are never going to manipulate an image.

So I would be in favour with this patch going in without the bc layer. Leaving at "needs work" pending further discussion.

fietserwin’s picture

image_load() also gets called by the imagecache_actions contrib module, so the search above does not seem to find all uses. But (as a co-maintainer of that module) I'm completely happy with the change as proposed, without any BC measures.

Looking at the recent overhaul of the image, image style and image effect functionality, I think that maintainers of modules that do any image handling should be preprared for a serious, non-trivial port of (that part of) their code anyway.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Considering that the maintainer of one of the major image-related modules that would touch this code (imagecache_actions) is okay with this sort of change, and 2 committers agree, resetting to RTBC. Still applies clearly, will retest to demonstrate.

tim.plunkett’s picture

Issue tags: -API change, -Kill includes

#50: image-2033669-50.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, image-2033669-50.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Kill includes

#50: image-2033669-50.patch queued for re-testing.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

tests passed

webchick’s picture

Issue tags: +Approved API change

Ok, marking as an approved API change, per #52. Since it sounds like alex/catch are more familiar with this, leaving it for one of them. :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/lib/Drupal/Core/Image/ImageInterface.phpundefined
@@ -0,0 +1,247 @@
+use Drupal\system\Plugin\ImageToolkitInterface;
+
+/**
+ * @todo.
+ */
+interface ImageInterface {

The use is not used and the @todo needs to be done.

+++ b/core/tests/Drupal/Tests/Core/Image/ImageTest.phpundefined
@@ -0,0 +1,356 @@
+  public function testSetResource() {
+    $resource = fopen($this->image->getSource(), 'r');
+    $this->image->setResource($resource);
+    $this->assertEquals($this->image->getResource(), $resource);
+    $this->image->setResource(FALSE);
+    $this->assertNotNull($this->image->getResource());

What are we testing by setting the resource to FALSE and then asserting the response of getResource is not null? Also passing FALSE to setResource is weird because it expects a parameter of type resource (allow you can't typehint on it)

+++ b/core/modules/system/lib/Drupal/system/Plugin/ImageToolkit/GDToolkit.phpundefined
@@ -48,35 +49,36 @@ public function settingsFormSubmit($form, &$form_state) {
-    $width = $image->info['width'];
-    $height = $image->info['height'];
+    $width = $image->getWidth();
+    $height = $image->getHeight();

These are unused variables - let's remove them.

larowlan’s picture

What are we testing by setting the resource to FALSE and then asserting the response of getResource is not null? Also passing FALSE to setResource is weird because it expects a parameter of type resource (allow you can't typehint on it)

We're testing that if resource isn't set but source is, that a new resource is created ala this code

public function getResource() {
    if (!$this->hasResource()) {
      $this->processInfo();
      $this->toolkit->load($this);
    }
    return $this->resource;
  }

Ie we're forcing hasResource() to return false

alexpott’s picture

+++ b/core/lib/Drupal/Core/Image/ImageInterface.phpundefined
@@ -0,0 +1,247 @@
+  /**
+   * Sets the image file resource.
+   *
+   * @param resource $resource
+   *   The image file handle.
+   *
+   * @return self
+   *   Returns this image file.
+   */
+  public function setResource($resource);

So then perhaps we should either add a comment in the test or improve the documentation here.

alexpott’s picture

+++ b/core/lib/Drupal/Core/Image/ImageInterface.phpundefined
@@ -0,0 +1,247 @@
+  /**
+   * Sets the image file resource.
+   *
+   * @param resource $resource
+   *   The image file handle.
+   *
+   * @return self
+   *   Returns this image file.
+   */
+  public function setResource($resource);

So then perhaps we should either add a comment in the test or improve the documentation here.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
114.27 KB
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Thanks Tim and Alex
Assuming bit agrees, trivial changes so back to Rtbc.

alexpott’s picture

Title: Image file objects should be classed » Change notice: Image file objects should be classed
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 8cfc089 and pushed to 8.x. Thanks!

claudiu.cristea’s picture

Related: #2063373: Cannot save image created from scratch with a failing test in #3.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

I'm told that no one will volunteer to write a change notice if the issue is assigned, and there's a sprint coming up.
Feel free to ping me if any questions arise.

claudiu.cristea’s picture

ParisLiakos’s picture

Priority: Normal » Critical
catch’s picture

larowlan’s picture

Assigned: Unassigned » larowlan

working on change notice

larowlan’s picture

Status: Active » Needs review

Draft change notice is here: https://drupal.org/node/2084547 please review

tim.plunkett’s picture

Title: Change notice: Image file objects should be classed » Image file objects should be classed
Priority: Major » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

I love the formatting! Looks great, very thorough.

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

Anonymous’s picture

Issue summary: View changes

Updated