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()

CommentFileSizeAuthor
#50 interdiff.txt1.43 KBEric_A
#50 interdiff-42-50.txt4.84 KBEric_A
#50 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
#50 replace-theme-with-drupal_render-image_formatter-2010126-50.patch6.12 KBEric_A
#48 interdiff.txt783 bytesEric_A
#48 interdiff-42-48.txt2.76 KBEric_A
#48 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
#48 replace-theme-with-drupal_render-image_formatter-2010126-48.patch6.01 KBEric_A
#47 interdiff.txt933 bytesEric_A
#47 interdiff-42-47.txt2.76 KBEric_A
#47 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
#47 replace-theme-with-drupal_render-image_formatter-2010126-47.patch6.01 KBEric_A
#46 interdiff-42-46.txt2.75 KBEric_A
#46 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
#46 replace-theme-with-drupal_render-image_formatter-2010126-46.patch6 KBEric_A
#45 interdiff.txt1.25 KBEric_A
#45 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
#45 replace-theme-with-drupal_render-image_formatter-2010126-45.patch5 KBEric_A
#42 interdiff.txt1.31 KBEric_A
#42 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
#42 replace-theme-with-drupal_render-image_formatter-2010126-42.patch3.85 KBEric_A
#40 interdiff.txt443 bytesEric_A
#40 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-38.patch2.34 KBEric_A
#40 replace-theme-with-drupal_render-image_formatter-2010126-40.patch3.29 KBEric_A
#38 interdiff.txt1.31 KBEric_A
#38 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-38.patch2.34 KBEric_A
#38 replace-theme-with-drupal_render-image_formatter-2010126-38.patch2.79 KBEric_A
#36 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-36.patch2.34 KBEric_A
#36 replace-theme-with-drupal_render-image_formatter-2010126-36.patch2.79 KBEric_A
#33 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-33.patch2.22 KBEric_A
#33 replace-theme-with-drupal_render-image_formatter-2010126-33.patch2.67 KBEric_A
#24 replace-theme-with-drupal_render-image_formatter-2010126-24.patch461 bytesEric_A
#21 replace-theme-with-drupal_render-image_formatter-2010126-21.patch461 bytesEric_A
#17 replace-theme-with-drupal_render-image_formatter-2010126-17.patch1.8 KBjuanolalla
#17 interdiff.txt439 bytesjuanolalla
#15 replace-theme-with-drupal_render-image_formatter-2010126-15.patch2.07 KBjuanolalla
#15 interdiff.txt1.15 KBjuanolalla
#12 replace-theme-with-drupal_render-image_formatter-2010126-12.patch1.98 KBjuanolalla
#10 replace-theme-with-drupa_render-image_formatter-2010126-10.patch1.39 KBjuanolalla
#5 2010126-5.patch1.58 KBalphawebgroup
#3 2010126-3.patch1.37 KBalphawebgroup
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Samvel’s picture

Title: Replace theme() with drupal_render() in image module for » Replace theme() with drupal_render() in image module for theme_image_formatter()

Title changing

alphawebgroup’s picture

Assigned: Unassigned » alphawebgroup
Issue tags: +CodeSprintUA

starting

alphawebgroup’s picture

Status: Active » Needs review
FileSize
1.37 KB
alphawebgroup’s picture

Will update that patch soon.

alphawebgroup’s picture

FileSize
1.58 KB
podarok’s picture

Status: Needs review » Reviewed & tested by the community

#5 looks good for me

catch’s picture

Status: Reviewed & tested by the community » Needs work

It shouldn't be necessary to pull properties out of the theme registry like that, if it is, then something serious is wrong.

thedavidmeister’s picture

Assigned: alphawebgroup » Unassigned

this has been sitting idle for a week. @alweb, feel free to re-assign if you're still working on this.

juanolalla’s picture

Assigned: Unassigned » juanolalla

I'm working on this from Drupal Dev Days Dublin

juanolalla’s picture

Assigned: juanolalla » Unassigned
Status: Needs work » Needs review
FileSize
1.39 KB

I didn't use the previous patch at #5, I started a new fresh approach following the concern catch posted at #7

Status: Needs review » Needs work

The last submitted patch, replace-theme-with-drupa_render-image_formatter-2010126-10.patch, failed testing.

juanolalla’s picture

Status: Needs work » Needs review
FileSize
1.98 KB

Uploaded again the path fixing the errors reported by the failing tests

siccababes’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/image/image.field.incundefined
@@ -424,24 +424,34 @@ function theme_image_widget($variables) {
+function theme_image_formatter($variables) {  ¶

Additional spaces at end of line..

+++ b/core/modules/image/image.field.incundefined
@@ -424,24 +424,34 @@ function theme_image_widget($variables) {
+  else {
+    $image['#uri'] = $item['uri'];
+  }

THis seems out of scope why are we doing this?

+++ b/core/modules/image/image.field.incundefined
@@ -424,24 +424,34 @@ function theme_image_widget($variables) {
+  foreach (array('width', 'height', 'alt', 'attributes') as $key) {
+    if (isset($item[$key])) {
+      $image["#$key"] = $item[$key];
+    }
   }

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

$image['#width'] = $item['width']
...
juanolalla’s picture

Status: Needs work » Needs review
FileSize
1.15 KB
2.07 KB

Thank you for the corrections, I've fixed it.

+++ b/core/modules/image/image.field.incundefined
@@ -424,24 +424,34 @@ function theme_image_widget($variables) {
+  else {
+    $image['#uri'] = $item['uri'];
+  }

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.

Status: Needs review » Needs work
juanolalla’s picture

Status: Needs work » Needs review
FileSize
439 bytes
1.8 KB

I 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()

patoshi’s picture

Status: Needs review » Reviewed & tested by the community

patch applied clean, created new image field. working good!

benjifisher’s picture

I did not test, but I reviewed the code. The changes are a little complex, but I agree with them.

+1 for RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 169b445 and pushed to 8.x. Thanks!

Eric_A’s picture

Status: Fixed » Needs review
FileSize
461 bytes

I 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.)

Status: Needs review » Needs work
benjifisher’s picture

+    if (array_key_exists($item[$key])) {...}

I think you want

  if (array_key_exists($key, $item)) {...}
Eric_A’s picture

Status: Needs work » Needs review
FileSize
461 bytes

@benjifisher, absolutely.

Status: Needs review » Needs work
stevector’s picture

Status: Needs work » Needs review

That failing test passes locally for me. The bot should re-run it.

Eric_A’s picture

Yeah, I'm going to re-test. This is from the PIFR report for reference.

The test did not complete due to a fatal error. Completion check TermTest.php 291 Drupal\taxonomy\Tests\TermTest->testTermInterface()

Theme fast initialization 12 passes, 0 fails, and 0 exceptions
exception 'Exception' with message '
To start over, you must empty your existing database, delete your active configuration, and copy default.settings.php over settings.php.
To install to a different database, edit the appropriate settings.php file in the sites folder.
To locate your active configuration, view the appropriate settings.php file in the sites folder.
To upgrade an existing installation, proceed to the update script.
View your existing site.
' in /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc:1031
Stack trace:
#0 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(480): install_verify_completed_task()
#1 /var/lib/drupaltestbot/sites/default/files/checkout/core/includes/install.core.inc(90): install_begin_request(Array)
#2 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php(774): install_drupal(Array)
#3 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TaxonomyTestBase.php(26): Drupal\simpletest\WebTestBase->setUp()
#4 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/TermTest.php(26): Drupal\taxonomy\Tests\TaxonomyTestBase->setUp()
#5 /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php(745): Drupal\taxonomy\Tests\TermTest->setUp()
#6 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(484): Drupal\simpletest\TestBase->run()
#7 /var/lib/drupaltestbot/sites/default/files/checkout/core/scripts/run-tests.sh(22): simpletest_script_run_one_test('619', 'Drupal\taxonomy...')
#8 {main}FATAL Drupal\taxonomy\Tests\TermTest: test runner returned a non-zero error code (1).
html.tpl.php html and body attributes 5 passes, 0 fails, and 0 exceptions

Eric_A’s picture

adamcowboy’s picture

Eric_A’s picture

Category: task » bug
Priority: Normal » Major

Changing category and escalating priority because of the regression described in #21.

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Considering we broke this it looks like we need a test so we don't break this again.

Eric_A’s picture

I tried something in \Drupal\image\Tests\Image\ThemeFunctionTest. (Couldn't find an existing equivalent assertion for theme_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() or render() in theme level testing.

Eric_A’s picture

Oops, 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.

Status: Needs review » Needs work
Eric_A’s picture

Status: Needs review » Needs work
Eric_A’s picture

Status: Needs review » Needs work
Eric_A’s picture

Have we found a similar bug in theme(), perhaps?

Status: Needs review » Needs work
Eric_A’s picture

Eric_A’s picture

The patch fixes the alt issue for theme_image_formatter() and theme_image_style() which have their test assertions in the same class. The bug in theme() was preventing us to fix the alt issue for theme_image_style() in drupal_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 in theme() made another assertion in this same test class pass in #33 when it shouldn't.

    // Link the image to a fragment on the page, and not a full URL.
    $fragment = $this->randomName();
    $element['#path']['path'] = '';
    $element['#path']['options'] = array(
      'external' => TRUE,
      'fragment' => $fragment,
    );
    $rendered_element = render($element);
    $expected_result = '<a href="#' . $fragment . '"><img class="image-style-test" src="' . $url . '" alt="" /></a>';

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.

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Cool. Everything looks good to me. I think the issues are similar enough that we can keep it here.

Eric_A’s picture

Added documentation for theme_image_style() following the example of theme_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..)

Eric_A’s picture

Ok, so for theme_image() the documentation is available both at theme_image() and drupal_common_theme(). Added the documentation at image_theme() as well.
Interdiff (with a little extra context) is between #42 and #46.

Eric_A’s picture

The drupal_common_theme() documentation had two references to the theme function...

Eric_A’s picture

Eric_A’s picture

@@ -978,7 +978,7 @@ function theme($hook, $variables = array()) {
     $variables = array();
     if (isset($info['variables'])) {
       foreach (array_keys($info['variables']) as $name) {
-        if (isset($element["#$name"])) {
+        if (array_key_exists("#$name", $element)) {
           $variables[$name] = $element["#$name"];
         }
       }

Assuming that the NULL value case is a rare one we would help performance if we did:

-        if (isset($element["#$name"])) {
+        if (isset($element["#$name"]) || array_key_exists("#$name", $element)) {

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().

Eric_A’s picture

Implemented #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.

pplantinga’s picture

Status: Needs review » Reviewed & tested by the community

Looks even better.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice work Eric_A

Committed d4fba77 and pushed to 8.x. Thanks!

Eric_A’s picture

Eric_A’s picture

Category: bug » task
Priority: Major » Normal

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