Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Comment | File | Size | Author |
---|---|---|---|
#22 | interdiff.txt | 715 bytes | jhedstrom |
#22 | image-rescale-phpunit-1996868-22.patch | 8.8 KB | jhedstrom |
#19 | interdiff.txt | 3.53 KB | jhedstrom |
#19 | image-rescale-phpunit-1996868-18.patch | 8.77 KB | jhedstrom |
#16 | interdiff.txt | 3.93 KB | jhedstrom |
Comments
Comment #1
jhedstromHere's an initial attempt. I'm sure this line needs to be done in a different way:
Comment #2
ParisLiakos CreditAttribution: ParisLiakos commentedduh...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
serviceand while we are at it convert the tests to phpunit as well
Comment #3
dawehnerThe 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.
Comment #4
jhedstromHere'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.Comment #5
ParisLiakos CreditAttribution: ParisLiakos commentedThanks, looks good!
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
Comment #6
dawehnerAs long your code is independent from anything in Drupal\Core it should live in Component. Something like image certainly makes sense.
Comment #7
dawehnerComment #8
jhedstromThis patch converts to use dataProvider instead of an array in the test method itself, and also simplifies the method/class names to remove redundancy.
Comment #9
jhedstromForgot to clean up the new component names in previous patch.
Comment #10
jhedstromRealized we should probably be adding
@deprecated
messages on these wrapper functions.Comment #12
dawehner#10: image-rescale-phpunit-1996868-10.patch queued for re-testing.
Comment #13
jhedstrom.
Comment #14
dawehnerLooks great!
what about scaleDimensions ... seems way easier to understand what the method is doing.
This needs a newline at the end.
I guess a little bit more specific provider name would be cool.
Comment #15
ParisLiakos CreditAttribution: ParisLiakos commentedalso.should be Contains \...
Comment #16
jhedstromThis should address #14 and #15.
Comment #17
ParisLiakos CreditAttribution: ParisLiakos commentedalsmost there, just some more nitpicks
it needs the full namespace aka
\Drupal\Tests\Component\Image\ImageTest
now
Should be called ImageTest in case we add more methods/tests on Image component
could be something better, eg 'Tests for Image component'
which is called via image_dimensions_scale should be removed
messages should be reverse since they are only displayed upon failure
we usually just prefix the methods name with provider.
Eg in this case:
providerTestImageDimensionsScale().
Also needs @return docs
Comment #19
jhedstromThis should address ##1.
Comment #20
dawehnerMissing starting "\"
If you provide an @see you can use the IDE and click on the classname directly. It's just a handy feature.
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentededit: see above^
Comment #22
jhedstromI think this addresses #20.
Comment #23
dawehnerThank you!
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedComment #25
alexpottCommitted 2bb2976 and pushed to 8.x. Thanks!
Comment #26
scor CreditAttribution: scor commentedupdate tags (normalize to "Needs change notification")
Comment #27
tim.plunkettFollow-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.
Comment #28
catch#2083415: [META] Write up all outstanding change notices before release
Comment #29
jhedstromComment #30
slashrsm CreditAttribution: slashrsm commentedWorking on change record.
Comment #31
slashrsm CreditAttribution: slashrsm commentedChange record: https://drupal.org/node/2144561
Comment #32
slashrsm CreditAttribution: slashrsm commentedComment #33
star-szrThanks @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.