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\Utility\File class

Files: 
CommentFileSizeAuthor
#64 image-2033669-64.patch114.27 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,701 pass(es).
[ View ]
#64 interdiff.txt1.71 KBtim.plunkett
#50 image-2033669-50.patch114.34 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,979 pass(es).
[ View ]
#44 image-class-2033669.44.interdiff.txt10.68 KBlarowlan
#44 image-class-2033669.44.patch113.88 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-class-2033669.44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 57,590 pass(es).
[ View ]
#40 coverage.html_.txt114.14 KBlarowlan
#38 image-2033669-38.patch102.17 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,523 pass(es).
[ View ]
#38 interdiff.txt8.01 KBtim.plunkett
#37 image-2033669-37.patch106.71 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#37 interdiff.txt8.26 KBtim.plunkett
#35 image-2033669-35.patch106.81 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#35 interdiff.txt65.83 KBtim.plunkett
#27 image-2033669-27.patch98.07 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,507 pass(es).
[ View ]
#27 interdiff.txt46.52 KBtim.plunkett
#25 image-2033669-23.patch93.35 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,161 pass(es).
[ View ]
#21 image-2033669-21.patch193.23 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-2033669-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#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
PASSED: [[SimpleTest]]: [MySQL] 56,534 pass(es).
[ View ]
#17 image-2033669-17.patch221.89 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,924 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#17 image-2033669-17-standalone-do-not-test.patch120.81 KBtim.plunkett
#12 image-2033669-12.patch177.22 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 57,189 pass(es).
[ View ]
#9 image-2033669-9.patch177.22 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 57,043 pass(es), 67 fail(s), and 103 exception(s).
[ View ]
#9 interdiff.txt13.57 KBtim.plunkett
#7 image-2033669-7.patch174.19 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,777 pass(es), 100 fail(s), and 31 exception(s).
[ View ]
#7 interdiff.txt32.78 KBtim.plunkett
#1 image-2033669-1-combined.patch162 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 56,829 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#1 image-2033669-1-do-not-test.patch52.49 KBtim.plunkett

Comments

Status:Active» Needs review
StatusFileSize
new52.49 KB
new162 KB
FAILED: [[SimpleTest]]: [MySQL] 56,829 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

Here it is combined with the image effects patch.

Status:Needs review» Needs work

+1

Related #204497: Configurable background color when cropping to larger dimensions.

+++ 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?

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

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

Status:Needs work» Needs review
StatusFileSize
new32.78 KB
new174.19 KB
FAILED: [[SimpleTest]]: [MySQL] 56,777 pass(es), 100 fail(s), and 31 exception(s).
[ View ]

More progress. Still contains the blocker.

Status:Needs review» Needs work
StatusFileSize
new13.57 KB
new177.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,043 pass(es), 67 fail(s), and 103 exception(s).
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new177.22 KB
PASSED: [[SimpleTest]]: [MySQL] 57,189 pass(es).
[ View ]

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

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.

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.

StatusFileSize
new120.81 KB
new221.89 KB
FAILED: [[SimpleTest]]: [MySQL] 56,924 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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.

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

Status:Postponed» Needs review
StatusFileSize
new175.67 KB
PASSED: [[SimpleTest]]: [MySQL] 56,534 pass(es).
[ View ]
new75.03 KB

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

StatusFileSize
new24.7 KB
new193.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-2033669-21.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

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

Best line in the patch, IMO :)

StatusFileSize
new93.35 KB
PASSED: [[SimpleTest]]: [MySQL] 57,161 pass(es).
[ View ]

Rerolled, this has no more blockers.

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

StatusFileSize
new46.52 KB
new98.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,507 pass(es).
[ View ]

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.

I'll try to fill one in soon.

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

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

Unit tests aren't my specialty. Any volunteers?

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

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

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

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.

StatusFileSize
new65.83 KB
new106.81 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

StatusFileSize
new8.26 KB
new106.71 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new8.01 KB
new102.17 KB
PASSED: [[SimpleTest]]: [MySQL] 57,523 pass(es).
[ View ]

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

Issue tags:+Kill includes

.

StatusFileSize
new114.14 KB
new110.62 KB
PASSED: [[SimpleTest]]: [MySQL] 57,590 pass(es).
[ View ]
new9.68 KB

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.

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?

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.

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

StatusFileSize
new128.14 KB
new113.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch image-class-2033669.44.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new10.68 KB

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\Utility\File class
Screen Shot 2013-07-29 at 6.04.41 PM.png

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

Issue summary:View changes

Updated issue summary.

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);
     }

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.

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new114.34 KB
PASSED: [[SimpleTest]]: [MySQL] 57,979 pass(es).
[ View ]

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

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

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.

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.

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.

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.

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

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

Status:Needs review» Reviewed & tested by the community

tests passed

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

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.

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

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

Ie we're forcing hasResource() to return false

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

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

Status:Needs work» Needs review
StatusFileSize
new1.71 KB
new114.27 KB
PASSED: [[SimpleTest]]: [MySQL] 57,701 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

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

Title:Image file objects should be classedChange 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!

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

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.

Priority:Normal» Critical

Assigned:Unassigned» larowlan

working on change notice

Status:Active» Needs review

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

Title:Change notice: Image file objects should be classedImage 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.

Issue summary:View changes

Updated