Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
follow up #2009580: Replace theme() with drupal_render() in image module
Need replace theme('image_style', $item) and theme('image', $item) with drupal_render() in theme_image_formatter()
Comment | File | Size | Author |
---|---|---|---|
#50 | interdiff.txt | 1.43 KB | Eric_A |
#50 | interdiff-42-50.txt | 4.84 KB | Eric_A |
#50 | replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch | 2.34 KB | Eric_A |
#50 | replace-theme-with-drupal_render-image_formatter-2010126-50.patch | 6.12 KB | Eric_A |
#48 | interdiff.txt | 783 bytes | Eric_A |
Comments
Comment #1
Samvel CreditAttribution: Samvel commentedTitle changing
Comment #2
alphawebgroupstarting
Comment #3
alphawebgroupComment #4
alphawebgroupWill update that patch soon.
Comment #5
alphawebgroupComment #6
podarok#5 looks good for me
Comment #7
catchIt shouldn't be necessary to pull properties out of the theme registry like that, if it is, then something serious is wrong.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedthis has been sitting idle for a week. @alweb, feel free to re-assign if you're still working on this.
Comment #9
juanolalla CreditAttribution: juanolalla commentedI'm working on this from Drupal Dev Days Dublin
Comment #10
juanolalla CreditAttribution: juanolalla commentedI didn't use the previous patch at #5, I started a new fresh approach following the concern catch posted at #7
Comment #12
juanolalla CreditAttribution: juanolalla commentedUploaded again the path fixing the errors reported by the failing tests
Comment #13
siccababes CreditAttribution: siccababes commentedI had an image on my site and I changed the image style from large to thumbnail to medium. The image changed sizes from what it was to what I changed it to be. This patch works just fine. Changing to RTBC.
Comment #14
alexpottAdditional spaces at end of line..
THis seems out of scope why are we doing this?
This is 4 lines of code... you could do this with 4 sets... so we have additional complexity for no gain... ie this could be
Comment #15
juanolalla CreditAttribution: juanolalla commentedThank you for the corrections, I've fixed it.
I need to do that because sometimes there's no entity defined at $item variable, for example when a link is created below in the function.
Comment #17
juanolalla CreditAttribution: juanolalla commentedI upload the patch with the interdiff from the previous patch #12. Please forget the last patch at #15, I made a mistake.
I've just removed the extra spaces. I've left the foreach because it doesn't work in just 4 lines with simple sets, the isset() checking condition is needed (otherwise tests that don't define these variables would fail), so the code without the foreach would be 12 lines. A similar foreach is implemented at theme_image()
Comment #18
patoshi CreditAttribution: patoshi commentedpatch applied clean, created new image field. working good!
Comment #19
benjifisherI did not test, but I reviewed the code. The changes are a little complex, but I agree with them.
+1 for RTBC.
Comment #20
alexpottCommitted 169b445 and pushed to 8.x. Thanks!
Comment #21
Eric_A CreditAttribution: Eric_A commentedI don't know if core image field formatters ever pass $item['alt'] with a NULL value, but there is a regression here in that NULL values are not passed through anymore to theme_image() or theme_image_style(). Both hooks default alt to the empty string, but allow you to pass in NULL to remove the alt attribute. (The latter is only documented for theme_image(), though.)
Comment #23
benjifisherI think you want
Comment #24
Eric_A CreditAttribution: Eric_A commented@benjifisher, absolutely.
Comment #26
stevectorThat failing test passes locally for me. The bot should re-run it.
Comment #27
Eric_A CreditAttribution: Eric_A commentedYeah, I'm going to re-test. This is from the PIFR report for reference.
Comment #28
Eric_A CreditAttribution: Eric_A commented#24: replace-theme-with-drupal_render-image_formatter-2010126-24.patch queued for re-testing.
Comment #29
adamcowboy CreditAttribution: adamcowboy commentedComment #30
Eric_A CreditAttribution: Eric_A commentedChanging category and escalating priority because of the regression described in #21.
Comment #31
pplantinga CreditAttribution: pplantinga commentedLooks good to me.
Comment #32
alexpottConsidering we broke this it looks like we need a test so we don't break this again.
Comment #33
Eric_A CreditAttribution: Eric_A commentedI tried something in
\Drupal\image\Tests\Image\ThemeFunctionTest
. (Couldn't find an existing equivalent assertion fortheme_image()
)I started with copying and pasting from existing code in this class. Personally I'm not fond at all of coupling with
drupal_render()
orrender()
in theme level testing.Comment #34
Eric_A CreditAttribution: Eric_A commentedOops, the first element is reused later in the test, so this won't work. I'll come back to this later today, unless somebody beats me to it.
Comment #36
Eric_A CreditAttribution: Eric_A commentedThis one should be better.
Comment #38
Eric_A CreditAttribution: Eric_A commentedComment #40
Eric_A CreditAttribution: Eric_A commentedHave we found a similar bug in
theme()
, perhaps?Comment #42
Eric_A CreditAttribution: Eric_A commentedFixed a bug in the test and fixed a bug in theme_image_style() that was introduced in #2010134: Replace theme() with drupal_render() in image module for theme_image_style().
Comment #43
Eric_A CreditAttribution: Eric_A commentedThe patch fixes the alt issue for
theme_image_formatter()
andtheme_image_style()
which have their test assertions in the same class. The bug intheme()
was preventing us to fix the alt issue fortheme_image_style()
indrupal_render()
context. For clarity we could leave out these parts and make them a follow-up for #2010134: Replace theme() with drupal_render() in image module for theme_image_style(). Note that the bug intheme()
made another assertion in this same test class pass in #33 when it shouldn't.Because of the bug in
theme()
top level properties with NULL values get lost and then defaults are applied to the variables array. So this can affect other tests as well at some point.Comment #44
pplantinga CreditAttribution: pplantinga commentedCool. Everything looks good to me. I think the issues are similar enough that we can keep it here.
Comment #45
Eric_A CreditAttribution: Eric_A commentedAdded documentation for
theme_image_style()
following the example oftheme_image()
.(To get the clearest interdiff I first re-rolled #42 to get rid of the offset for theme.inc caused by #2031319: The documentation for theme() should be clearer about saying not to call it directly..)
Comment #46
Eric_A CreditAttribution: Eric_A commentedOk, so for
theme_image()
the documentation is available both attheme_image()
anddrupal_common_theme()
. Added the documentation atimage_theme()
as well.Interdiff (with a little extra context) is between #42 and #46.
Comment #47
Eric_A CreditAttribution: Eric_A commentedThe
drupal_common_theme()
documentation had two references to the theme function...Comment #48
Eric_A CreditAttribution: Eric_A commentedRemoved the extra space in the documentation. Sorry for the noise.
Comment #49
Eric_A CreditAttribution: Eric_A commentedAssuming that the NULL value case is a rare one we would help performance if we did:
If this holds then it applies to all added instances of
array_key_exists()
in this patch.Core already uses this pattern. See for example
drupal_static()
.Comment #50
Eric_A CreditAttribution: Eric_A commentedImplemented #49. The patch is still fixing the bug, still looks absolutely sane, and the risk of hurting performance is gone.
@pplantinga, do you still like the patch? If we're going to add tests then might as well add API docs, right? The current wording is just being consistent with what we have in core now. Let's just finish this follow-up ASAP.
Comment #51
pplantinga CreditAttribution: pplantinga commentedLooks even better.
Comment #52
alexpottNice work Eric_A
Committed d4fba77 and pushed to 8.x. Thanks!
Comment #53
Eric_A CreditAttribution: Eric_A commentedSame follow-up needs review in #2009662: [REGRESSION] Replace theme() with drupal_render() in picture module.
Comment #54
Eric_A CreditAttribution: Eric_A commented