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.
Related Issues
#2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class
Comment | File | Size | Author |
---|---|---|---|
#64 | image-2033669-64.patch | 114.27 KB | tim.plunkett |
#64 | interdiff.txt | 1.71 KB | tim.plunkett |
#50 | image-2033669-50.patch | 114.34 KB | tim.plunkett |
#44 | image-class-2033669.44.interdiff.txt | 10.68 KB | larowlan |
#44 | image-class-2033669.44.patch | 113.88 KB | larowlan |
Comments
Comment #1
tim.plunkettHere it is combined with the image effects patch.
Comment #3
larowlan+1
Comment #4
claudiu.cristeaRelated #204497: Make the background fill color configurable where image styles result in blank space.
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
?Comment #5
tim.plunkettThat's code from the other issue, and it's just moved around, not added there either.
Comment #6
claudiu.cristeaI know, but it's a chance to clean.
Comment #7
tim.plunkettMore progress. Still contains the blocker.
Comment #9
tim.plunkettOkay. We might be done here.
Still waiting on the blocker.
Comment #12
tim.plunkettComment #13
andypostRelated issue #1987712-71: Convert file_download() to a new style controller about image generation
Comment #14
andypostWithout injection?
This method should have controller after #2027423: Make image style system flexible
Comment #15
tim.plunkettIf 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.
Comment #16
tim.plunkettI'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.
Comment #17
tim.plunkettHere 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.
Comment #19
tim.plunkettThat 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
Comment #20
tim.plunkettFirst blocker went in, pretty happy so far with the second (last) blocker, so here's a reroll.
Will repostpone again later.
Comment #21
tim.plunkettimage.inc is finally dead! No more patches for now until the blocker is in, even if this fails.
Comment #22
tim.plunkettBest line in the patch, IMO :)
Comment #25
tim.plunkettRerolled, this has no more blockers.
Comment #26
jibranAwesome work @tim.plunkett. It is green. I want to RTBC it but let others have a look.
Comment #27
tim.plunkettneclimdul 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.Comment #28
tim.plunkettI'll try to fill one in soon.
Comment #29
larowlanShouldn't we be sure to clear $this->resource when setting a new source?
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.
Comment #30
tim.plunkettI'm not sure. The previous code didn't do that.
Unit tests aren't my specialty. Any volunteers?
Comment #31
larowlanI 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
Comment #32
larowlanI really think we should be type-hinting an interface here, which means we need to add one.
Comment #33
larowlanUnless of course something that extends Image is fine, in which case ignore me
Comment #34
tim.plunkettI 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.
Comment #35
tim.plunkettHad a long talk with msonnabaum, Crell, and katbailey about calling
new Image()
everywhere, and this is what I came up with.Obviously OO code is using the injected version.
It directly mimics the config.factory service.
Comment #37
tim.plunkettMissed a use statement, and msonnabaum pointed out that the Drupal::image wrapper was overkill.
Comment #38
tim.plunkett#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...
Comment #39
tim.plunkett.
Comment #40
larowlanSo 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.
Comment #41
jibranExtra \
Can we inject here?
@inheritdoc
@inheritdoc
Is there any other way to write this?
Comment #42
tim.plunkett@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.
Comment #43
larowlanwe 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
Comment #44
larowlanFixes 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
Comment #45
jibranThanks @larowlan. I think it is ready to fly. Let's wait for bot ;). This now needs issue summary update :).
Comment #45.0
jibranUpdated issue summary.
Comment #46
tim.plunkettUpdated.
Also, for the eventual change notice, this is a perfect hunk to show off the changes:
Comment #47
jibran@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.
Comment #48
xjm#44: image-class-2033669.44.patch queued for re-testing.
Comment #50
tim.plunkettReroll, it just conflicted with #2033383: Provide a default plugin bag on some
use
statements and no actual code, so setting back to RTBC.Comment #51
webchickSince 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
Comment #52
alexpott@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.
Comment #53
fietserwinimage_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.
Comment #54
tim.plunkettConsidering 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.
Comment #55
tim.plunkett#50: image-2033669-50.patch queued for re-testing.
Comment #57
tim.plunkett#50: image-2033669-50.patch queued for re-testing.
Comment #58
larowlantests passed
Comment #59
webchickOk, 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. :)
Comment #60
alexpottThe use is not used and the @todo needs to be done.
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)
These are unused variables - let's remove them.
Comment #61
larowlanWe're testing that if resource isn't set but source is, that a new resource is created ala this code
Ie we're forcing hasResource() to return false
Comment #62
alexpottSo then perhaps we should either add a comment in the test or improve the documentation here.
Comment #63
alexpottSo then perhaps we should either add a comment in the test or improve the documentation here.
Comment #64
tim.plunkettComment #65
larowlanThanks Tim and Alex
Assuming bit agrees, trivial changes so back to Rtbc.
Comment #66
alexpottCommitted 8cfc089 and pushed to 8.x. Thanks!
Comment #67
claudiu.cristeaRelated: #2063373: Cannot save image created from scratch with a failing test in #3.
Comment #68
tim.plunkettI'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.
Comment #69
claudiu.cristeaRelated #2066219: Decouple image type from image extension.
Comment #70
ParisLiakos CreditAttribution: ParisLiakos commentedComment #71
catch#2083415: [META] Write up all outstanding change notices before release.
Comment #72
larowlanworking on change notice
Comment #73
larowlanDraft change notice is here: https://drupal.org/node/2084547 please review
Comment #74
tim.plunkettI love the formatting! Looks great, very thorough.
Comment #75.0
(not verified) CreditAttribution: commentedUpdated