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

starting

Will update that patch soon.

#5 looks good for me

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

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

I'm working on this from Drupal Dev Days Dublin

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

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

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.

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

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.

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

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.

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

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

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

I think you want

  if (array_key_exists($key, $item)) {...}

@benjifisher, absolutely.

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

#24: replace-theme-with-drupal_render-image_formatter-2010126-24.patch queued for re-testing.

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

Looks good to me.

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

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.

This one should be better.

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

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.

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

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

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.

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

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

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.

Looks even better.

Nice work Eric_A

Committed d4fba77 and pushed to 8.x. Thanks!

Same follow-up needs review in #2009662: [REGRESSION] Replace theme() with drupal_render() in picture module.