Replace theme() with drupal_render() in image module

add related issue

append related issue

attached

append related issue

attached new one

re-roll

I was trying to reroll this patch when I came across a few issues about which I am not entirely sure.

In block.admin.inc and HelpController.php, render arrays are replaced with calls to drupal_add_css. Such as:

    $output = array(
      '#attached' => array(
        'css' => array(drupal_get_path('module', 'help') . '/css/help.module.css'),
      ),
      '#markup' => '<h2>' . t('Help topics') . '</h2><p>' . t('Help is available on the following items:') . '</p>' . $this->helpLinksAsList(),
    );

This is replaced with:

    // Add CSS.
    drupal_add_css(drupal_get_path('module', 'help') . '/css/help.css');
    $output = '<h2>' . t('Help topics') . '</h2><p>' . t('Help is available on the following items:') . '</p>' . $this->helpLinksAsList();

I was in an impression that we are moving towards render arrays for everything. Please clarify.

Another issue I faced was in core/modules/node/node.admin.inc. The patch introduced a change in node_multiple_delete_confirm_submit() but it seems that this function (and many others) are completely removed. I checked the documentation and they seem to be removed.

Lastly, I don't see why this patch removes core/modules/system/lib/Drupal/system/Tests/InstallerTest.php and modifies scripts in core/scripts/generate-d7-content.sh and core/scripts/generate-d6-content.sh.

But in good faith, I have just gone ahead and rerolled the patch. Please set the issue to 'needs review' if you think the above issues are not problems.

Agreed on those issues. Also, many changes, while they could be good ones, were unrelated to this issue.

Here's a new patch. Just a quick question, do we care about theme() calls in tests? This patch only covers image.admin.inc and image.file.inc, not any test files.

Argh, decided I'd add an "alt" tag to preview images, didn't know that tests depended on not having it. Whoever reviews this should look at questions I had in #10.

Yeah, we should remove the theme calls in tests as well. Do you want to take a stab at adding those in?

Stuck on #2030243: Remove theme_file_upload_help() from the theme system

Once the aforementioned issue is in, then this patch should do it.

How is #2030243: Remove theme_file_upload_help() from the theme system not accommodated by, and thus duplicate of this present issue?

The name for the issue was not as clear as it could have been, since i came across it while working on this issue. I've just renamed it. It looks like a resolution is near for the issue, so it shouldn't matter that much either way.

Basically that issue is about a FAPI conflict in a file.module theme function, which appears in more places than just the image module. I felt it deserved a separate issue.

Re-roll patch, again assuming that #2030243: Remove theme_file_upload_help() from the theme system gets in.

@@ -47,7 +48,8 @@ function image_style_form($form, &$form_state, $style) {
   $form['preview'] = array(
     '#type' => 'item',
     '#title' => t('Preview'),
-    '#markup' => theme('image_style_preview', array('style' => $style)),
+    '#theme' => 'image_style_preview',
+    '#style' => $style,
   );

Mixing up elements like this is a bad idea. It just so happens that there (currently) is no conflict here between the properties of the two elements, but we should just separate these properly. Especially if we want to do #2035977: Introduce #type elements for base theme hooks in image.module.
Type item by default does not declare #theme and does declare #theme_wrappers, so we should try something like this:

   $form['preview'] = array(
     '#type' => 'item',
     '#title' => t('Preview'),
-    '#markup' => theme('image_style_preview', array('style' => $style)),
+    'image_style_preview' => array(
+      '#theme' => 'image_style_preview',
+      '#style' => $style,
+    ),
   );

It might not even work still. In many cases it will just work: the rendered children of the item will be concatenated with an empty #markup property string.
Safest would be to just populate the #markup property, with a #pre_render. EDIT2: #pre_render would perhaps be out of scope for these straight conversions I guess. Just a direct drupal_render() then. This would mean that the current patch is already taking an out of scope approach here.

You can use any #theme_wrapper you like. And combine it with any fitting #theme. The point is that you must not merge elements of different types into one level. That can always lead to conflicts. There is no way to solve that.

Yeah, I should have said:
Different elements cannot just be merged into one element on one level.
Every single element has its own properties (explicit or implicit as type defaults), be it #input, #markup, #theme, #theme_wrappers, #description. Merging elements will lead to conflicts.
So they must live as a siblings or a children. Of course, when early rendering into a string every type can be stuffed into #markup.

How would you convert this example?

$form['preview'] = array(
   '#type' => 'item',
   '#title' => t('Preview'),
   '#markup' => theme('image_style_preview', array('style' => $style)) . theme('more_link', array('url' => $url, 'title' => $title)),
);

I can come up with three options:

$form['preview'] = array(
   '#type' => 'item',
   '#title' => t('Preview'),
   '#markup' => drupal_render($image_style_preview) . drupal_render($more_link),
);

$form['preview'] = array(
   '#type' => 'item',
   '#title' => t('Preview'),
   '#pre_render' => array('please_populate_my_type_item_markup_property'),
);

$form['preview'] = array(
   '#type' => 'item',
   '#title' => t('Preview'),
   'image_style_preview' => $image_style_preview,
   'more_link' => $more_link,
);

In many ways the third seems best. But for alterable structures moving the contents of #markup to newly invented children is API breakage.
Even when there is only one theme function used to build #markup, I think you should still not try and merge it into another element. It's messy and many a time it won't even be possible.
Note that if there was a #theme property involved (explicit or implicit as type default) the children would not be rendered by drupal_render() itself. They would have to be taken care of by the theme function.

Updated patch.

d'oh.

Looks like test bot likes it. I forgot to mention that I addressed issue raised in #19.

Looks like this needs a reroll!

Rerolled...

Any reason this render array's variable name doesn't match the naming convention established in the parent issue?

Just a better variable name, more descriptive, more consistent and all that. I've changed it to $image in the latest patch though.

conversion to l() looks out of scope here

I've verified that the output is identical... does that help? Converting theme() to l() is indirectly changing theme() to drupal_render() since l() calls drupal_render(). That's my angle, at any rate. I've attached two patches, with and without that change.

The rest should be fixed as well.

#35: replace-theme-image-module-no-l-2009580-35.patch queued for re-testing.

It needed a re-roll

Related issue: #1898420: image.module - Convert theme_ functions to Twig.

+++ b/core/modules/image/image.field.incundefined
@@ -433,12 +437,12 @@ function theme_image_formatter($variables) {
-  if (isset($item['entity']) && empty($item['uri'])) {
-    $image['#uri'] = $item['entity']->getFileUri();
-  }
-  else {
+  if (!empty($item['uri'])) {
     $image['#uri'] = $item['uri'];
   }
+  elseif (isset($item['entity'])) {
+    $image['#uri'] = $item['entity']->getFileUri();
+  }

So this change makes it possible for $image['#uri'] to not be set. Why are we changing the logic here (in a conversion issue)

Re-rolled and updated with changes in #44

Images are loading correctly in the admin area and on a node. Ran the tests for image module locally with patch and came back green. Setting RTBC.

Committed 3f97e8c and pushed to 8.x. Thanks!