Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
This should be taken from the 'Textimage Text' effect of the Textimage module.
Proposed resolution
Provide patch and test coverage.
Comment | File | Size | Author |
---|---|---|---|
#23 | 2655044-23.patch | 109.73 KB | mondrake |
#23 | interdiff_21-23.txt | 8.28 KB | mondrake |
#21 | 2655044-21.patch | 111.37 KB | mondrake |
Comments
Comment #2
slashrsm CreditAttribution: slashrsm at Examiner.com commentedComment #3
mondrakePostponed on #2662872: Remove code moved to Image Effects (2)
Comment #4
mondrakeTextimage 8.x-3-0-alpha2 has been released, this can be worked on now.
Comment #5
mondrakeSome initial work, missing tests + several todos still.
Comment #7
mondrakeComment #8
mondrakeComment #9
mondrakeComment #10
mondrakeSome progress, but still lots to do.
Comment #11
mondrakeComment #13
mondrakepatch with
--binary
optionComment #14
mondrakeComment #15
mondrakeSo, first reviewable patch. Interdiff would be bigger than patch itself, so not attached.
The 'Text overlay' effect allows to overlay text on the image. Text can contain tokens, but only global ones can be processed by core functionalities. Textimage module add some support for node, file and user tokens but requires using its own field formatter. Also, Textimage extends the effect by allowing a preview of the text overlay, like the 'Textimage text' provides. When this goes in, the 'Textimage text' effect can be dropped from Textimage.
ImageMagick support is only partial, like in Textimage - the image toolkit operation actually creates a temporary wrapper for the text overlay as a GD image, flushes to disk, then watermarks it via ImageMagick commands.
Comment #16
mondrakeRerolled after commit of #2667562: Add "Set transparent color" image effect.
Comment #17
slashrsm CreditAttribution: slashrsm at Examiner.com commentedFew comments:
s/IMG/img
@file is not required any more in this kind of cases:
https://www.drupal.org/node/2304909
Explain the reason for this decision.
$matches it not returned nor it seems we do anything with it after this point.
This should be converted to proper PHPDoc comment.
I prefer short array syntax. But this is more of a personal preference.
We can remove ---. Applies to all occurrences.
Can we remove this empty function?
This condition looks strange.
$image_width and $image_height are not used many times in this function. We could entirely replace them with $this->info counterparts.
Indentation looks strange.
We could simplify this with something like:
Comment #18
mondrakeAssigning
Comment #19
mondrakeThanks for review @slashrsm!
1. Done
2. AFAICS that's still RTBC, not finally live. In any case the entire module would require to be cleaned up eventually. I suggest to open an issue when the coding standard is finalised, for the module overall.
3. Added comments.
4. Actually $matches is passed in by reference to the method, so it is somehow 'returned'. See the tests in ImageEffectsTextUtilityTest.
5. Done
6. Me too. But here I am trying to move the code from Textimage with as little change as possible. Follow up for converting array() to [] for the entire module in a later stage?
7. Done
8. Done
9. Added a comment to explain why
10. Actually no, because $this->info can be changed by other methods while processing and we need to store starting width/height.
11. Fixed
12. Done
Comment #21
mondrakeI keep forgetting --binary option in git diff :(
Comment #22
mondrakeCreated issue #2684181: @file is not required for classes, interfaces and traits re #17.2
If that one lands before this one, this one will require reroll.
Comment #23
mondrakeRemoved @file docblocks from traits/classes. Patch with --binary option (just to remind myself)
Comment #25
mondrakeSo I thought to move ahead and commit this -
a) we're still in alpha
b) little chance to have more reviews for a 100k+ patch, better have it in and address bugs from reports
c) this is not new code (mostly) but the port of the 'Textimage text' effect from the Textimage module, which has already been in alpha for a few months with some use from statistics and no bugs reported
Comment #27
jo1ene CreditAttribution: jo1ene commented"Textimage module add some support for node, file and user tokens but requires using its own field formatter."
How does this work in the context of an Image Style?