Part of #2006152: [meta] Don't call theme() directly anywhere outside drupal_render().

CommentFileSizeAuthor
#17 drupal-2008982-16.patch2.12 KBc4rl
#16 twig-2008982-16.patch985 byteshussainweb
#13 twig-2008982-13.patch4.64 KBadamcowboy
#13 interdiff.txt1.12 KBadamcowboy
#11 twig-2008982-10.patch674 bytesadamcowboy
#5 drupal-core-replace_theme_config_module-2008982-5.patch746 bytesxxAlHixx
#3 drupal-core-replace_theme_config_module-2008982-3.patch753 bytesxxAlHixx

Comments

thedavidmeister’s picture

thedavidmeister commented
Issue tags: +Novice
xxAlHixx’s picture

xxAlHixx commented
Assigned: Unassigned » xxAlHixx

WDG (Ukraine,Kharkov) want to implement this on CodeSprintUA.

xxAlHixx’s picture

xxAlHixx commented
Status: Active » Needs review
Issue tags: +CodeSprintUA
StatusFileSize
new drupal-core-replace_theme_config_module-2008982-3.patch753 bytes
podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Status: Needs review » Needs work 
+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.phpundefined
@@ -77,8 +77,14 @@ public function diff($config_file) {
+    $table = array(
+      '#theme' => 'table',   ¶
...
+    );
+    ¶
     $output['diff'] = array(
...

code styling errors
https://drupal.org/coding-standards#indenting
needs work

xxAlHixx’s picture

xxAlHixx commented
Status: Needs work » Needs review
StatusFileSize
new drupal-core-replace_theme_config_module-2008982-5.patch746 bytes

I've fixed code styling errors

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Status: Needs review » Reviewed & tested by the community

#5 looks good

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

We can assign the variables directly to $table instead of assigning to $variables then later to $table.

thedavidmeister’s picture

thedavidmeister commented
Assigned: xxAlHixx » Unassigned
adamcowboy’s picture

adamcowboy commented
Assigned: Unassigned » adamcowboy

Dibs!

eromero1’s picture

eromero1 commented
Assigned: adamcowboy » eromero1

Dibs!!

adamcowboy’s picture

adamcowboy commented
Assigned: eromero1 » adamcowboy
Status: Needs work » Needs review
StatusFileSize
new twig-2008982-10.patch674 bytes

Done!

Status: Needs review » Needs work

The last submitted patch, twig-2008982-10.patch, failed testing.

adamcowboy’s picture

adamcowboy commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1.12 KB
new twig-2008982-13.patch4.64 KB

Take 2!

jenlampton’s picture

jenlampton
she / her
commented
Assigned: adamcowboy » Unassigned

Great job adam :)

thedavidmeister’s picture

thedavidmeister commented
Status: Needs review » Needs work 
diff --git a/core/modules/config/lib/Drupal/config/Controller/ConfigController.php.orig b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php.orig
new file mode 100644
index 0000000..5709feb
--- /dev/null
+++ b/core/modules/config/lib/Drupal/config/Controller/ConfigController.php.origundefined
@@ -0,0 +1,116 @@
+<?php
+
+/**
+ * @file
+ * Contains \Drupal\config\Controller\ConfigController
+ */
+
+namespace Drupal\config\Controller;
+
+use Drupal\Core\Controller\ControllerInterface;
+use Drupal\Core\Config\StorageInterface;
+use Drupal\Component\Archiver\ArchiveTar;
+use Drupal\Core\Ajax\AjaxResponse;
+use Drupal\Core\Ajax\OpenModalDialogCommand;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+
+/**
+ * Returns responses for config module routes.
+ */
+class ConfigController implements ControllerInterface {
+
+  /**
+   * The target storage.
+   *
+   * @var \Drupal\Core\Config\StorageInterface;
+   */
+  protected $targetStorage;
+
+  /**
+   * The source storage.
+   *
+   * @var \Drupal\Core\Config\StorageInterface;
+   */
+  protected $sourceStorage;
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static($container->get('config.storage'), $container->get('config.storage.staging'));
+  }
+
+  /**
+   * Constructs a ConfigController object.
+   *
+   * @param \Drupal\Core\Config\StorageInterface $target_storage
+   *   The target storage.
+   * @param \Drupal\Core\Config\StorageInterface $source_storage
+   *   The source storage
+   */
+  public function __construct(StorageInterface $target_storage, StorageInterface $source_storage) {
+    $this->targetStorage = $target_storage;
+    $this->sourceStorage = $source_storage;
+  }
+
+  /**
+   * Downloads a tarball of the site configuration.
+   */
+  public function downloadExport() {
+    $archiver = new ArchiveTar(file_directory_temp() . '/config.tar.gz', 'gz');
+    $config_dir = config_get_config_directory();
+    $config_files = array();
+    foreach (\Drupal::service('config.storage')->listAll() as $config_name) {
+      $config_files[] = $config_dir . '/' . $config_name . '.yml';
+    }
+    $archiver->createModify($config_files, '', config_get_config_directory());
+    return file_download('temporary', 'config.tar.gz');
+  }
+
+  /**
+   * Shows diff of specificed configuration file.
+   *
+   * @param string $config_file
+   *   The name of the configuration file.
+   *
+   * @return string
+   *   Table showing a two-way diff between the active and staged configuration.
+   */
+  public function diff($config_file) {
+    // Add the CSS for the inline diff.
+    $output['#attached']['css'][] = drupal_get_path('module', 'system') . '/css/system.diff.css';
+
+    $diff = config_diff($this->targetStorage, $this->sourceStorage, $config_file);
+    $formatter = new \DrupalDiffFormatter();
+    $formatter->show_header = FALSE;
+
+    $variables = array(
+      'header' => array(
+        array('data' => t('Old'), 'colspan' => '2'),
+        array('data' => t('New'), 'colspan' => '2'),
+      ),
+      'rows' => $formatter->format($diff),
+    );
+
+    $output['diff'] = array(
+      '#markup' => theme('table', $variables),
+    );
+
+    $output['back'] = array(
+      '#type' => 'link',
+      '#attributes' => array(
+        'class' => array(
+          'dialog-cancel',
+        ),
+      ),
+      '#title' => "Back to 'Synchronize configuration' page.",
+      '#href' => 'admin/config/development/sync',
+    );
+
+    // @todo Remove use of drupal_set_title() when
+    //   http://drupal.org/node/1871596 is in.
+    drupal_set_title(t('View changes of @config_file', array('@config_file' => $config_file)), PASS_THROUGH);
+
+    return $output;
+  }
+}

huh?

hussainweb’s picture

hussainweb
Primary language English
commented
Assigned: Unassigned » hussainweb
Status: Needs work » Needs review
StatusFileSize
new twig-2008982-16.patch985 bytes

Fixing the patch.

I also considered removing drupal_render as it seemed trivial. But didn't considering the guidelines in #2006152: [meta] Don't call theme() directly anywhere outside drupal_render(). This is just a plain fixed patch.

c4rl’s picture

c4rl commented
Assigned: hussainweb » Unassigned
StatusFileSize
new drupal-2008982-16.patch2.12 KB

Yeah, not sure I understand that last one. Let's try this one.

thedavidmeister’s picture

thedavidmeister commented

Yeh, the guidelines say:

If the patch does change anything else, it will need manual testing and therefore slow/stall what should be relatively simple tasks.

The patch where I changed things, #2006974: Replace theme() with drupal_render() in views_ui.module was the first patch to go up and it still hasn't been reviewed while every straight conversion patch has been committed within a week or so of it being cleared of any code style issues. Waiting for manual testing is slooow.

My vote would be for the patch in #16 to just get the theme() removal out of the way, with a followup issue to make it more like #17 after the commit to get rid of the "flat" #markup attribute.

thedavidmeister’s picture

thedavidmeister commented

#16: twig-2008982-16.patch queued for re-testing.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

#16 needs to be re-rolled. Don't re-roll #17. See #18.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Ignore #20. My bad.

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Status: Needs review » Reviewed & tested by the community

#16 looks good to me.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

This looks like it doesn't need #markup at all, could just leave the table element unrendered.

thedavidmeister’s picture

thedavidmeister commented
Issue tags: +Needs manual testing

Sure, well the patch in #17 still applies against HEAD so we just need some manual testing of the final markup.

hussainweb’s picture

hussainweb
Primary language English
commented
Assigned: Unassigned » hussainweb

I am doing a round of manual testing. Any particular steps I should follow?

hussainweb’s picture

hussainweb
Primary language English
commented

I followed instructions on Managing configuration in Drupal 8 and copied all .yml files from a previous D8 instance to a fresh HEAD installation. I clicked on 'View Differences' for one of the changes config files and this is what I got.

<div><div class="tableresponsive-toggle-columns"><button class="link tableresponsive-toggle" type="button" title="Show table cells that were hidden to make the table fit within a small screen." style="display: none;">Hide unimportant columns</button></div><table class="responsive-enabled tableresponsive-processed">
 <thead><tr><th colspan="2">Old</th><th colspan="2">New</th> </tr></thead>
<tbody>
 <tr class="odd"><td>&nbsp;</td><td class="diff-context">id: comment_unpublish_action</td><td>&nbsp;</td><td class="diff-context">id: comment_unpublish_action</td> </tr>
 <tr class="even"><td>&nbsp;</td><td class="diff-context">label: 'Unpublish comment'</td><td>&nbsp;</td><td class="diff-context">label: 'Unpublish comment'</td> </tr>
 <tr class="odd"><td class="diff-marker">-</td><td class="diff-context diff-deletedline">uuid: <span class="diffchange">0f7f169f</span>-<span class="diffchange">5f79</span>-<span class="diffchange">48f0</span>-<span class="diffchange">b900</span>-<span class="diffchange">a760b25712b5</span></td><td class="diff-marker">+</td><td class="diff-context diff-addedline">uuid: <span class="diffchange">9c00b317</span>-<span class="diffchange">5145</span>-<span class="diffchange">4d08</span>-<span class="diffchange">a800</span>-<span class="diffchange">b0668f765d17</span></td> </tr>
 <tr class="even"><td>&nbsp;</td><td class="diff-context">status: '1'</td><td>&nbsp;</td><td class="diff-context">status: '1'</td> </tr>
 <tr class="odd"><td>&nbsp;</td><td class="diff-context">langcode: en</td><td>&nbsp;</td><td class="diff-context">langcode: en</td> </tr>
</tbody>
</table>
<a class="dialog-cancel" href="/d8conf/admin/config/development/sync">Back to 'Synchronize configuration' page.</a></div>

I then installed the patch at #17. I cleared the cache and viewed the difference for the same config file. I verified the changes were in effect by temporarily altering some strings and seeing them in output. This is the HTML after the change:

<div><div class="tableresponsive-toggle-columns"><button class="link tableresponsive-toggle" type="button" title="Show table cells that were hidden to make the table fit within a small screen." style="display: none;">Hide unimportant columns</button></div><table class="responsive-enabled tableresponsive-processed">
 <thead><tr><th colspan="2">Old</th><th colspan="2">New</th> </tr></thead>
<tbody>
 <tr class="odd"><td>&nbsp;</td><td class="diff-context">id: comment_unpublish_action</td><td>&nbsp;</td><td class="diff-context">id: comment_unpublish_action</td> </tr>
 <tr class="even"><td>&nbsp;</td><td class="diff-context">label: 'Unpublish comment'</td><td>&nbsp;</td><td class="diff-context">label: 'Unpublish comment'</td> </tr>
 <tr class="odd"><td class="diff-marker">-</td><td class="diff-context diff-deletedline">uuid: <span class="diffchange">0f7f169f</span>-<span class="diffchange">5f79</span>-<span class="diffchange">48f0</span>-<span class="diffchange">b900</span>-<span class="diffchange">a760b25712b5</span></td><td class="diff-marker">+</td><td class="diff-context diff-addedline">uuid: <span class="diffchange">9c00b317</span>-<span class="diffchange">5145</span>-<span class="diffchange">4d08</span>-<span class="diffchange">a800</span>-<span class="diffchange">b0668f765d17</span></td> </tr>
 <tr class="even"><td>&nbsp;</td><td class="diff-context">status: '1'</td><td>&nbsp;</td><td class="diff-context">status: '1'</td> </tr>
 <tr class="odd"><td>&nbsp;</td><td class="diff-context">langcode: en</td><td>&nbsp;</td><td class="diff-context">langcode: en</td> </tr>
</tbody>
</table>
<a class="dialog-cancel" href="/d8conf/admin/config/development/sync">Back to 'Synchronize configuration' page.</a></div>

I ran above through diff and there was no difference. Visually too, there is no difference whatsoever.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Fixed

Committed/pushed #17 to 8.x, thanks!

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