Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom’s picture

Status: Active » Needs review
FileSize
2.81 KB

Here's an initial attempt. I'm sure this line needs to be done in a different way:

require __DIR__ . "/../../../../../includes/image.inc";
ParisLiakos’s picture

Title: Convert Drupal\image\Tests\ImageDimensionsScaleUnitTest to phpunit » Convert image.inc to a service
Issue tags: +PHPUnit, +PHPUnit Blocker

duh...image_dimensions_scale().....i thought this was testing a class...but it is for a function:/
we need to do something with image.inc
i think it should be a service since it needs the image.toolkit service
and while we are at it convert the tests to phpunit as well

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/tests/Drupal/Tests/Component/Image/ImageDimensionsScaleUnitTest.phpundefined
@@ -2,17 +2,19 @@
+ * Definition of Drupal\Tests\Component\Image\ImageDimensionsScaleUnitTest.

The new core standards talks about "Core \...".

I don't think we need per se a new service, as we could inject the image.toolkit service into the scale object , if you need it.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
8.06 KB

Here's an initial attempt at moving the logic of image_dimensions_scale() into a class. I haven't addressed #3 yet, as I don't fully understand the standard (I was following the example of #1935908: Convert Graph tests to phpunit which uses Component instead of Core, see also https://drupal.org/node/2001218#comment-7445204).

Moving back to needs review as I figure we can discuss the new Image class while sorting out the proper namespacing.

ParisLiakos’s picture

Title: Convert image.inc to a service » Convert image.inc to an Image component

Thanks, looks good!

+++ b/core/lib/Drupal/Component/Image/Image.phpundefined
@@ -0,0 +1,63 @@
+  public static function imageDimensionsScale(array &$dimensions, $width = NULL, $height = NULL, $upscale = FALSE) {

you can remove the image prefix for the method:)

How about moving more of the functions though from image.inc to this component?
and then turn the imageScaleUnitTests to just ImageTest, that unit tests each of its methods? something similar like String and Unicode:)

I guess we can leave functions depending on image.toolkit service out for now, or introduce a method setImageToolkit, where we inject the toolkit right after the container is built

dawehner’s picture

Title: Convert image.inc to an Image component » Convert image.inc to a service

As long your code is independent from anything in Drupal\Core it should live in Component. Something like image certainly makes sense.

dawehner’s picture

Title: Convert image.inc to a service » Convert image.inc to an Image component
jhedstrom’s picture

This patch converts to use dataProvider instead of an array in the test method itself, and also simplifies the method/class names to remove redundancy.

jhedstrom’s picture

Forgot to clean up the new component names in previous patch.

jhedstrom’s picture

Realized we should probably be adding @deprecated messages on these wrapper functions.

Status: Needs review » Needs work
Issue tags: -PHPUnit, -PHPUnit Blocker

The last submitted patch, image-rescale-phpunit-1996868-10.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit, +PHPUnit Blocker
jhedstrom’s picture

.

dawehner’s picture

Looks great!

+++ b/core/lib/Drupal/Component/Image/Image.phpundefined
@@ -0,0 +1,63 @@
+  public static function dimensionsScale(array &$dimensions, $width = NULL, $height = NULL, $upscale = FALSE) {

what about scaleDimensions ... seems way easier to understand what the method is doing.

+++ b/core/lib/Drupal/Component/Image/Image.phpundefined
@@ -0,0 +1,63 @@
+  }

This needs a newline at the end.

+++ b/core/tests/Drupal/Tests/Component/Image/DimensionsScaleTest.phpundefined
@@ -2,29 +2,49 @@
+  public function provider() {

I guess a little bit more specific provider name would be cool.

ParisLiakos’s picture

+++ b/core/tests/Drupal/Tests/Component/Image/DimensionsScaleTest.phpundefined
@@ -2,29 +2,49 @@
+ * Definition of Drupal\Tests\Component\Image\DimensionsScaleTest.

also.should be Contains \...

jhedstrom’s picture

This should address #14 and #15.

ParisLiakos’s picture

alsmost there, just some more nitpicks

+++ b/core/tests/Drupal/Tests/Component/Image/ScaleDimensionsTest.phpundefined
@@ -2,29 +2,49 @@
+ * Contains Drupal\Tests\Component\Image\ScaleDimensionsTest.

it needs the full namespace aka
\Drupal\Tests\Component\Image\ImageTest
now

+++ b/core/tests/Drupal/Tests/Component/Image/ScaleDimensionsTest.phpundefined
@@ -2,29 +2,49 @@
+class ScaleDimensionsTest extends UnitTestCase {

Should be called ImageTest in case we add more methods/tests on Image component

+++ b/core/tests/Drupal/Tests/Component/Image/ScaleDimensionsTest.phpundefined
@@ -2,29 +2,49 @@
+      'name' => 'Drupal\Component\Image\Image::imageScaleDimensions()',

could be something better, eg 'Tests for Image component'

+++ b/core/tests/Drupal/Tests/Component/Image/ScaleDimensionsTest.phpundefined
@@ -2,29 +2,49 @@
+      'description' => 'Tests all control flow branches in Drupal\Component\Image\Image::imageDimensionsScale() which is called via image_dimensions_scale().',

which is called via image_dimensions_scale should be removed

+++ b/core/tests/Drupal/Tests/Component/Image/ScaleDimensionsTest.phpundefined
@@ -2,29 +2,49 @@
+    $this->assertEquals($output['dimensions']['width'], $input['dimensions']['width'], sprintf('Computed width (%s) equals expected width (%s)', $output['dimensions']['width'], $input['dimensions']['width']));
...
+    $this->assertEquals($output['dimensions']['height'], $input['dimensions']['height'], sprintf('Computed height (%s) equals expected height (%s)', $output['dimensions']['height'], $input['dimensions']['height']));
...
+    $this->assertEquals($output['return_value'], $return_value, 'Correct return value.');

messages should be reverse since they are only displayed upon failure

+++ b/core/tests/Drupal/Tests/Component/Image/ScaleDimensionsTest.phpundefined
@@ -2,29 +2,49 @@
+  public function scaleDimensionsProvider() {

we usually just prefix the methods name with provider.
Eg in this case:
providerTestImageDimensionsScale().

Also needs @return docs

Status: Needs review » Needs work

The last submitted patch, image-rescale-phpunit-1996868-16.patch, failed testing.

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
8.77 KB
3.53 KB

This should address ##1.

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Component/Image/ImageTest.phpundefined
@@ -2,29 +2,59 @@
+ * Contains Drupal\Tests\Component\Image\ImageTest.

Missing starting "\"

+++ b/core/tests/Drupal/Tests/Component/Image/ImageTest.phpundefined
@@ -2,29 +2,59 @@
+ * Tests Drupal\Component\Image\Image.
...
+class ImageTest extends UnitTestCase {

If you provide an @see you can use the IDE and click on the classname directly. It's just a handy feature.

ParisLiakos’s picture

edit: see above^

jhedstrom’s picture

I think this addresses #20.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

ParisLiakos’s picture

Title: Convert image.inc to an Image component » Start converting image.inc to an Image component
alexpott’s picture

Title: Start converting image.inc to an Image component » Change notice: Start converting image.inc to an Image component
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Committed 2bb2976 and pushed to 8.x. Thanks!

scor’s picture

Issue tags: +Needs change record

update tags (normalize to "Needs change notification")

tim.plunkett’s picture

Follow-ups (in this order):

#2027423: Make image style system flexible
#1821854: Convert image effects into plugins
#2033669: Image file objects should be classed

If all three get in, there will just be image_get_info() as a two line wrapper, and the @defgroup (which should likely be moved.

catch’s picture

jhedstrom’s picture

slashrsm’s picture

Assigned: Unassigned » slashrsm

Working on change record.

slashrsm’s picture

Status: Active » Needs review
slashrsm’s picture

Assigned: slashrsm » Unassigned
star-szr’s picture

Title: Change notice: Start converting image.inc to an Image component » Start converting image.inc to an Image component
Priority: Major » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Thanks @slashsrm! I think we can close this issue, edits can be made to the change record if needed but it looks pretty reasonable to me.

Status: Fixed » Closed (fixed)

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