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

Files: 
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
FAILED: [[SimpleTest]]: [MySQL] 57,500 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#50 replace-theme-with-drupal_render-image_formatter-2010126-50.patch6.12 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,826 pass(es).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] 57,300 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#48 replace-theme-with-drupal_render-image_formatter-2010126-48.patch6.01 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,732 pass(es).
[ View ]
#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
FAILED: [[SimpleTest]]: [MySQL] 57,627 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#47 replace-theme-with-drupal_render-image_formatter-2010126-47.patch6.01 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,745 pass(es).
[ View ]
#46 interdiff-42-46.txt2.75 KBEric_A
#46 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,639 pass(es), 13 fail(s), and 2 exception(s).
[ View ]
#46 replace-theme-with-drupal_render-image_formatter-2010126-46.patch6 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,961 pass(es).
[ View ]
#45 interdiff.txt1.25 KBEric_A
#45 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,656 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#45 replace-theme-with-drupal_render-image_formatter-2010126-45.patch5 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,818 pass(es).
[ View ]
#42 interdiff.txt1.31 KBEric_A
#42 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-42.patch2.34 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,921 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#42 replace-theme-with-drupal_render-image_formatter-2010126-42.patch3.85 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,300 pass(es).
[ View ]
#40 interdiff.txt443 bytesEric_A
#40 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-38.patch2.34 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,866 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#40 replace-theme-with-drupal_render-image_formatter-2010126-40.patch3.29 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,636 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#38 interdiff.txt1.31 KBEric_A
#38 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-38.patch2.34 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,779 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#38 replace-theme-with-drupal_render-image_formatter-2010126-38.patch2.79 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,810 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#36 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-36.patch2.34 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,628 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#36 replace-theme-with-drupal_render-image_formatter-2010126-36.patch2.79 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,853 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#33 replace-theme-with-drupal_render-image_formatter-2010126-tests-only-33.patch2.22 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,698 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#33 replace-theme-with-drupal_render-image_formatter-2010126-33.patch2.67 KBEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,712 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#24 replace-theme-with-drupal_render-image_formatter-2010126-24.patch461 bytesEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,531 pass(es).
[ View ]
#21 replace-theme-with-drupal_render-image_formatter-2010126-21.patch461 bytesEric_A
FAILED: [[SimpleTest]]: [MySQL] 57,185 pass(es), 14 fail(s), and 205 exception(s).
[ View ]
#17 replace-theme-with-drupal_render-image_formatter-2010126-17.patch1.8 KBjuanolalla
PASSED: [[SimpleTest]]: [MySQL] 56,393 pass(es).
[ View ]
#17 interdiff.txt439 bytesjuanolalla
#15 replace-theme-with-drupal_render-image_formatter-2010126-15.patch2.07 KBjuanolalla
FAILED: [[SimpleTest]]: [MySQL] 56,333 pass(es), 14 fail(s), and 190 exception(s).
[ View ]
#15 interdiff.txt1.15 KBjuanolalla
#12 replace-theme-with-drupal_render-image_formatter-2010126-12.patch1.98 KBjuanolalla
PASSED: [[SimpleTest]]: [MySQL] 57,919 pass(es).
[ View ]
#10 replace-theme-with-drupa_render-image_formatter-2010126-10.patch1.39 KBjuanolalla
FAILED: [[SimpleTest]]: [MySQL] 58,634 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#5 2010126-5.patch1.58 KBalweb
PASSED: [[SimpleTest]]: [MySQL] 56,074 pass(es).
[ View ]
#3 2010126-3.patch1.37 KBalweb
FAILED: [[SimpleTest]]: [MySQL] 55,850 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

Comments

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

Title changing

Assigned:Unassigned» alweb
Issue tags:+CodeSprintUA

starting

Status:Active» Needs review
StatusFileSize
new1.37 KB
FAILED: [[SimpleTest]]: [MySQL] 55,850 pass(es), 0 fail(s), and 8 exception(s).
[ View ]

Will update that patch soon.

StatusFileSize
new1.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,074 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

#5 looks good for me

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.

Assigned:alweb» Unassigned

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

Assigned:Unassigned» juanolalla

I'm working on this from Drupal Dev Days Dublin

Assigned:juanolalla» Unassigned
Status:Needs work» Needs review
StatusFileSize
new1.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,634 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.98 KB
PASSED: [[SimpleTest]]: [MySQL] 57,919 pass(es).
[ View ]

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

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.

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']
...

Status:Needs work» Needs review
StatusFileSize
new1.15 KB
new2.07 KB
FAILED: [[SimpleTest]]: [MySQL] 56,333 pass(es), 14 fail(s), and 190 exception(s).
[ View ]

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

The last submitted patch, replace-theme-with-drupal_render-image_formatter-2010126-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new439 bytes
new1.8 KB
PASSED: [[SimpleTest]]: [MySQL] 56,393 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

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

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

+1 for RTBC.

Status:Reviewed & tested by the community» Fixed

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

Status:Fixed» Needs review
StatusFileSize
new461 bytes
FAILED: [[SimpleTest]]: [MySQL] 57,185 pass(es), 14 fail(s), and 205 exception(s).
[ View ]

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

The last submitted patch, replace-theme-with-drupal_render-image_formatter-2010126-21.patch, failed testing.

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

I think you want

<?php
 
if (array_key_exists($key, $item)) {...}
?>

Status:Needs work» Needs review
StatusFileSize
new461 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,531 pass(es).
[ View ]

@benjifisher, absolutely.

Status:Needs review» Needs work

The last submitted patch, replace-theme-with-drupal_render-image_formatter-2010126-24.patch, failed testing.

Status:Needs work» Needs review

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

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

Category:task» bug
Priority:Normal» Major

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

Status:Needs review» Reviewed & tested by the community

Looks good to me.

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.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new2.67 KB
FAILED: [[SimpleTest]]: [MySQL] 57,712 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.22 KB
FAILED: [[SimpleTest]]: [MySQL] 57,698 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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.

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

The last submitted patch, replace-theme-with-drupal_render-image_formatter-2010126-33.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,853 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,628 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This one should be better.

Status:Needs review» Needs work

The last submitted patch, replace-theme-with-drupal_render-image_formatter-2010126-36.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,810 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,779 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.31 KB

Status:Needs review» Needs work

The last submitted patch, replace-theme-with-drupal_render-image_formatter-2010126-38.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.29 KB
FAILED: [[SimpleTest]]: [MySQL] 57,636 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,866 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new443 bytes

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

Status:Needs review» Needs work

The last submitted patch, replace-theme-with-drupal_render-image_formatter-2010126-40.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.85 KB
PASSED: [[SimpleTest]]: [MySQL] 57,300 pass(es).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,921 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.31 KB

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

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.

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.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new5 KB
PASSED: [[SimpleTest]]: [MySQL] 57,818 pass(es).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,656 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new1.25 KB

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

StatusFileSize
new6 KB
PASSED: [[SimpleTest]]: [MySQL] 57,961 pass(es).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,639 pass(es), 13 fail(s), and 2 exception(s).
[ View ]
new2.75 KB

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.

StatusFileSize
new6.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,745 pass(es).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,627 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.76 KB
new933 bytes

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

StatusFileSize
new6.01 KB
PASSED: [[SimpleTest]]: [MySQL] 57,732 pass(es).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,300 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new2.76 KB
new783 bytes

Removed the extra space in the documentation. Sorry for the noise.

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

StatusFileSize
new6.12 KB
PASSED: [[SimpleTest]]: [MySQL] 57,826 pass(es).
[ View ]
new2.34 KB
FAILED: [[SimpleTest]]: [MySQL] 57,500 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new4.84 KB
new1.43 KB

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.

Status:Needs review» Reviewed & tested by the community

Looks even better.

Status:Reviewed & tested by the community» Fixed

Nice work Eric_A

Committed d4fba77 and pushed to 8.x. Thanks!

Category:bug» task
Priority:Major» Normal

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