Convert theme_system_compact_link() into a #type link

updating the title.

This is part of #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays

add twig example

To test:
From the admin/index page, click on "Show Descriptions" and verify that the text displays. After page reloads, click "Hide Descriptions" and verify that the descriptor text is removed.

Also, on admin/people/permissions, perform above show/hide description action and verify text shows and hides accordingly.

Both tests worked.

The patch looks good. I do feel like the three instances of this link could be provided by a central system_build_compact_link() function, but I'm sure that could be a followup issue as it's not required in order to remove theme_system_compact_link().

Minor touch-up needed:

+++ b/core/modules/system/system.admin.incundefined
@@ -995,15 +995,37 @@ function theme_admin_page($variables) {
+    '#children' =>  drupal_render($system_compact_link),

@@ -1047,8 +1069,30 @@ function theme_system_admin_index($variables) {
+    '#children' =>  drupal_render($system_compact_link),

+++ b/core/modules/user/user.admin.incundefined
@@ -236,9 +236,29 @@ function theme_user_admin_permissions($variables) {
+    '#children' =>  drupal_render($system_compact_link),

Two spaces between => and drupal_render(), remove one.

OK, spaces removed. First time uploading a patch, I'm guessing the file needed to be renamed.

Will this need further review?

@ericrdb - thanks! The patch from #3 no longer applies and will first need a reroll.

Also, you should not edit patch files directly. Instead, apply the patch locally and create a new patch :)

This proved helpful guide for naming patches: https://drupal.org/patch/submit

Trying again.

#10: core-remove_theme_system_compact_link-1833932-9.patch queued for re-testing.

Nice work @ericrdb! I just hit re-test, it looks like we had a random test failure.

Cool - thanks for the patch links too @Cottser.

Patch in #10 has one more fixed up drupal_render() line :)

+ '#children' => drupal_render($system_compact_link),

double space before drupal_render

+ $output = drupal_render($system_compact);

double space before =

We should not render the container child immediately, but lazy-render by using a child element like 'system_compact_link' instead of the "#children" property. See remarks by @Fabianx and @catch in #2031301: Replace theme_more_link() and replace with #type 'more_link'.

Since theme_container() doesn't render child elements we then need to build our container render array with '#theme_wrappers' instead of '#theme'.

Addresses #18 and #19.

This patch needs to be re-rolled; it doesn't apply anymore. The things jenlampton mentioned should also be fixed.

rerolling patch.

Rerolled the patch so that it works again, as well as, added the refactoring suggested by jenlampton.

Thanks @mikemiles86!

Patch works fine. Thank you!

Nice clean-up. How come there are no hunks to remove the old theme function though?

Some changes were removed in the last patch from the patch in #20 with no explanation. What about the changes in system.module (that remove the old function), system.admin.inc, and user.admin.inc?

The changes in user.admin.inc, if they are still part of this patch, will need to be rerolled due to #1938888: Convert user_admin_permissions to a new-style Form object.

Needs work based on the above.

Here is a re-roll of #20. I'll follow up by integrating the changes from #24.

The patch in #30 was messed up. This one is #20 re-rolled, plus the changes from #24.

Renaming the last patch.

lokapujya, thanks for re-rolling. sorry everyone for my messed up re-roll, it was my first time attempting one.

33: 1833932-33.patch queued for re-testing.

reroll.

Look into using #route_name instead of #href.

#2151107: Convert theme_system_compact_link() to Twig has been going on in parallel, I am not sure which is the best approach here. #38 adds quite a lot of code duplication to be able to remove the theme function, and it looks like whatever we do will conflict and need refactoring when #2151095: Convert theme_admin_page() to Twig and #2151105: Convert theme_system_admin_index() to Twig get in.

Sorry I'm late to the party here but could we go with something like:
#type 'system_compact_link'?

And if it's feasible to remove the wrapper div and just use CSS like this: #2031301: Replace theme_more_link() and replace with #type 'more_link'

I agree that we should remove the duplication. Unassigned because I don't understand how it should integrate with twig yet.

Can we do "suggestions" with #type? Like #type = link__system_compact ?

I don't think so but it would be worth a stab...

Going to try this using the same approach as #2031301: Replace theme_more_link() and replace with #type 'more_link'

This seems to work well. Needs a markup comparison.
No need for profiling, it only ever shows up once on a page per admin.

Couldn't use $this->t() because it is a static preRender call, maybe there is a better way to deal with that?

Anyways... enjoy.

Huh, I should have known better I introduced a bug in MoreLink unintentionally, I've included the fix in this patch as well.

'class' needs to be an array.

Looking really good overall. I manually tested at /admin/config (added to issue summary).

1. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,77 @@
   +   * Pre-render callback: Renders a lin k into #markup.
   

   Minor: link has an extra space in there.

2. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,77 @@
   +   *   - One of the following
   +   *     - #route_name and (optionally) a #route_parameters array; The route
   +   *       name and route parameters which will be passed into the link
   +   *       generator.
   +   *     - #href: The system path or URL to pass as argument to l().
   

   The first line needs to end in a colon so that the following lines become a list. Ref. https://www.drupal.org/node/1354#lists

3. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,77 @@
   +      $element['#title'] = t('Show descriptions');
   ...
   +        'title' => t('Expand layout to include descriptions.'),
   ...
   +      $element['#title'] = t('Hide descriptions');
   ...
   +        'title' => t('Compress layout by hiding descriptions.'),
   

   Can we use $this->t()?

Nevermind 3 I guess, didn't see that it was a static method.

You know what, if those are the only changes then here's a patch :) RTBC from me.

Actually, this was missing the title attribute after doing a proper markup comparison/diff. Here's the before and after with the attached patch:

Before:

  <div class="compact-link"><a href="/admin/compact/on?destination=admin/config" title="Compress layout by hiding descriptions.">Hide descriptions</a></div>

After (including twig_debug):

<!-- THEME DEBUG -->
<!-- CALL: _theme('container') -->
<!-- BEGIN OUTPUT from 'core/modules/system/templates/container.html.twig' -->
<div class="compact-link"><a href="/admin/compact/on?destination=admin/config" title="Compress layout by hiding descriptions.">Hide descriptions</a></div>

<!-- END OUTPUT from 'core/modules/system/templates/container.html.twig' -->

Any chance of introducing tests for the expected markup while we're here?

Added some tests there.

:<

Rerolled

Ok I'm gonna fix this mess

Hopefully I fixed my own mess. Forget last 2 patches

Fixing, thanks for the test!

The system compact link was missing from patch #65

At admin/people/permissions, the wrapper and the system compact link elements have the same ID (unlike D7). Is this the way form API is supposed to work or do we need a new issue? Otherwise, this is RTBC for me.

Oh dang it seems container = form-wrapper for some confusing reason and it does lots of things it shouldn't...

I'm a bit unsure how to proceed here, maybe just #prefix/#suffix hard coded... argh. Thoughts?

system_compact_link isn't a form type. Back in #38, we used #markup and set it with drupal_render(). Not sure why it changed?

@lokapujya It's not intended to be a form type, it's just a Render Element type element like Link, MoreLink, Table.

It doesn't extend FormElement, but unfortunately the theme_wrapper does extend form wrapper and it's super weird/unintuitive that theme_wrapper => 'container' acts like a form element with it's processContainer and preRenderGroup...

@laurii thanks for that patch though that is a bit confusing why that would work... don't you think so?

I think we may need to go the route in #72, but I opened an issue for investigating an alternative. If you continue from #68 and apply this patch #2347209: Render regular element in a form should not duplicate container ID., the IDs are removed.

+++ b/core/modules/user/src/Form/UserPermissionsForm.php
@@ -96,8 +96,9 @@
+    $system_compact_link = array('#type' => 'system_compact_link');
     $form['system_compact_link'] = array(
-      '#type' => 'system_compact_link',
+      '#markup' => drupal_render($system_compact_link),

The array can just go directly inside drupal_render().

First, let's figure out if we want drupal_render() or Form API. EDIT: A third option may be #prefix.

The array can just go directly inside drupal_render().

I'm fairly sure that generates an error, see https://www.drupal.org/node/2006152#reviewer-notes :)

Rerolled #72.

I am trying to understand this issue but I don't understand why we would want to drupal_render() early.

@cilefen we don't that was to prove a point I assume. #type inside a form element seem to get a bunch of extra cruft on the container and the element as well.

View source on admin/people/permissions to see the extra ids and stuff on the markup.

1. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,77 @@
   +      $element['#href'] = 'admin/compact/off';
   ...
   +      $element['#href'] = 'admin/compact/on';
   

   Please use #route_name and #route_parameters, we're trying to kill #href.

2. +++ b/core/modules/system/system.module
   @@ -1163,24 +1160,6 @@ function system_time_zones($blank = NULL) {
   -    $output .= \Drupal::l(t('Show descriptions'), new Url('system.admin_compact_page', array('mode' => 'off'), array('attributes' => array('title' => t('Expand layout to include descriptions.')), 'query' => drupal_get_destination())));
   ...
   -    $output .= \Drupal::l(t('Hide descriptions'), new Url('system.admin_compact_page', array('mode' => 'on'), array('attributes' => array('title' => t('Compress layout by hiding descriptions.')), 'query' => drupal_get_destination())));
   

   Look, they used to be routes! Should be easy.

Fixed #80. #73 has to be still fixed.

+++ b/core/modules/system/src/Tests/Common/RenderElementTypesTest.php
@@ -194,7 +194,7 @@
+        'expected' => '//div[@class="compact-link"]/a[contains(@href, "admin/compact/on?") and text()="Hide descriptions"]',

That's not a reasonable change to the expectations. Please revert.

Removed .swp from the patch

Sorry, I misunderstood the change, I didn't realize this was a brand new test.

Need to check this out again to see if the ID's that got tagged on to the container are still there or not. It's so weird that types act different depending on whether they are used in a form or not... I hope that goes away.

Tested 1833932-87.patch on admin/config and permissions page as described in #4.

Unit tests pass.

The container of the link has the compact-link class.

@xen thanks for a review!

Everything else looks good for me but I found out there's still duplicate ID's on permissions form

I removed the ID completely since there's no ID in any other use cases than permission form. This also fixes the issue mentioned on #90 and #92.

Manually tested at these three paths, the only difference is that the link on /admin/people/permissions has an extra class of "form-wrapper" added.

• /admin/config/index
• /admin/index
• /admin/people/permissions

+++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
@@ -0,0 +1,79 @@
+/**
+ * Provides a link render element.
+ *
+ * @RenderElement("system_compact_link")
+ */
+class SystemCompactLink extends Link {

This description should be updated, it's the same as the Link render element.

Other than that this is looking good!

Updated the comment and the one for More Link.

Cool, testbot is having some issues but will try our luck at RTBC and see how long that sticks. Thanks @lokapujya!

1. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,79 @@
   +   *   A structured array whose keys form the arguments to l():
   +   *   - #title: The link text to pass as argument to l().
   ...
   +   *     - #href: The system path or URL to pass as argument to l().
   +   *   - #options: (optional) An array of options to pass to l() or the link
   ...
   +    // By default, link options to pass to l() are normally set in #options.
   

   l() no longer exists.

2. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,79 @@
   +      $element['#title'] = t('Show descriptions');
   ...
   +        'attributes' => array('title' => t('Expand layout to include descriptions.')),
   ...
   +      $element['#title'] = t('Hide descriptions');
   ...
   +        'attributes' => array('title' => t('Compress layout by hiding descriptions.')),
   

   $this->t() not t()

Also we need a change record to explain that the theme function is going away and how to replace it.

@alexpott, for the t() how do we use $this->t() inside a static or is there another way to do the processing?

@joelpittet may be this can help http://stackoverflow.com/a/2286720/1233922

I can't reproduce the install error from #104 and I am not sure why Url is considered a double-use. This is an experiment using the url static.

Lets try this out..

1. +++ b/core/includes/common.inc
   @@ -2994,6 +2994,7 @@ function drupal_render(&$elements, $is_root_call = FALSE) {
   diff --git a/core/lib/Drupal/Core/Render/Element/MoreLink.php b/core/lib/Drupal/Core/Render/Element/MoreLink.php
   
   diff --git a/core/lib/Drupal/Core/Render/Element/MoreLink.php b/core/lib/Drupal/Core/Render/Element/MoreLink.php
   index 4f7e90c..fbdc62f 100644
   
   index 4f7e90c..fbdc62f 100644
   --- a/core/lib/Drupal/Core/Render/Element/MoreLink.php
   
   --- a/core/lib/Drupal/Core/Render/Element/MoreLink.php
   +++ b/core/lib/Drupal/Core/Render/Element/MoreLink.php
   
   +++ b/core/lib/Drupal/Core/Render/Element/MoreLink.php
   +++ b/core/lib/Drupal/Core/Render/Element/MoreLink.php
   @@ -8,7 +8,7 @@
   
   @@ -8,7 +8,7 @@
    namespace Drupal\Core\Render\Element;
    
    /**
   - * Provides a link render element.
   + * Provides a link render element for a "more" link, like those used in blocks.
     *
     * @RenderElement("more_link")
     */
   @@ -24,7 +24,7 @@ public function getInfo() {
   
   @@ -24,7 +24,7 @@ public function getInfo() {
          '#title' => $this->t('More'),
          '#theme_wrappers' => array(
            'container' => array(
   -          '#attributes' => array('class' => 'more-link'),
   +          '#attributes' => array('class' => array('more-link')),
            ),
          ),
        ) + $info;
   

   More Link changes - does this belongs here? Introduced in #59.

2. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,84 @@
   +   *     - #route_name and (optionally) a #route_parameters array; The route
   

   #router_parameters should be in new line for better readability...

3. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,84 @@
   +   *     - #href: The system path or URL to pass as argument to Drupal::l().
   

   .. and not sure about #href, like stated above this should #href should be killed.

4. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,84 @@
   +    // By default, link options to pass to l() are normally set in #options.
   

   Note from #99 still not implemented. l() no longer exists.

Everything else seems fine and patch applies cleanly against current HEAD. Tested on pages:

• /admin/config
• /admin/index
• /admin/people/permissions

I guess the more link changes doesnt belong to this issue. Dunno where they're from.

Ups found the actual comment #51. Maybe we can include the bug fix there.

+++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
@@ -0,0 +1,84 @@
+   *     - #href: The system path or URL to pass as argument to Drupal::l().

#href is not used, we should delete it from here, and I do not see #route_name and #route_parameters used.

Yes, there is one occurence of the l() function in documentation, that should be changed to Drupal::l()

Alltogether, it looks fine. It seems we do not even need the #route_name and #route_parameters, we do not use the #href.
The documentation part should be updated by someone who understands what is going on.

@mikispeed

Re: #112.1 Technically it should be a quick fix follow-up. Would you mind creating that new issue, I can move the patch over to it if you'd prefer or you can tackle that too?

@joelpittet I will now create new issue and post a patch there.

Created new issue from #112.1 and posted a patch in #2368957: Set class on MoreLink as array instead as string.

@mikispeed thanks we can take it out of this patch now. I've RTBCd that morelink followup.

Ok, so first a reroll (I see it needed per #111) and MoreLink part taken out.

Lets see what test bot says :)

Another quick style review:

1. diff --git a/core/includes/common.inc b/core/includes/common.inc
   index 4163da2..895e0e4 100644
   --- a/core/includes/common.inc
   +++ b/core/includes/common.inc
   @@ -3008,6 +3008,7 @@ function drupal_render(&$elements, $is_root_call = FALSE) {
    
      $elements['#printed'] = TRUE;
      $elements['#markup'] = SafeMarkup::set($elements['#markup']);
   +
      return $elements['#markup'];
    }
    
   

   An extra empty line got added in #104 - not sure why?

2. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,84 @@
   +   *   - #options: (optional) An array of options to pass to Drupal::l() or the link
   

   Comments should wrap at 80 characters

3. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,84 @@
   +      $element['#url'] = BaseUrl::fromRoute('system.admin_compact_page', array('mode' => 'off'));
   

   Short arrays should use the compact array style (array('mode' => 'off') becomes ['mode' => 'off']).

Thanks a lot @akalata for your review! I fixed points 1. and 2. on the patch and they were good catch but I'm not very sure about 3. point because there's no policy for which style we should use and in the other parts of the file we are using the array() syntax. I personally prefer shorter syntax but because there's no clear policy which one to use I would leave it as is. Are you ok with this or should we change the whole file to follow that style?

@lauriii, I'm fine with that, I've just been following @joelpittet's lead on short array style for small arrays that we can put all on one line. No "official" reason to change, I don't think.

Manually reviewed the latest patch for code style and out-of-scope changes. Everything looks okay to me.

According to @Cottser, no change record for theme_ function removal is necessary - which makes sense in this case since this is only used on the admin side.

This issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

1. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,84 @@
   +      $element['#title'] = \Drupal::translation()->translate('Show descriptions');
   ...
   +        'attributes' => array('title' => \Drupal::translation()->translate('Expand layout to include descriptions.')),
   ...
   +      $element['#title'] = \Drupal::translation()->translate('Hide descriptions');
   ...
   +        'attributes' => array('title' => \Drupal::translation()->translate('Compress layout by hiding descriptions.')),
   

   This should be using the plugin's t() method - ie. $this->t()

2. +++ b/core/lib/Drupal/Core/Render/Element/SystemCompactLink.php
   @@ -0,0 +1,84 @@
   +    $element['#markup'] = \Drupal::l($element['#title'], $element['#url']->setOptions($options));
   

   We should inject the link_generator instead of using \Drupal::l(). But I think we should open a followup to do this since we should inject this into the parent class (Link) since that needs it too.

The method is static, therefore using static::t() instead.

Whoops, sorry. Some unexpected files came in to the patch. Rerolling.

There you go. Previous interdiff still applies.

To use static::t we need to have static t method in the class.

After a short discussion on IRC with GaborHojtsy and alexpott we decided to use just simply t() instead of \Drupal::translation()->translate or static::t() there.

So that means the patch in #127 looks good. Sorry for derailing the issue. Perhaps someone can re-upload that to see if it still passes testbot and to make it the latest patch.

I'll test that out.

I tested the patch and that applied fine. The patch is the exact same as in 127 but just renamed.

Actually, the darned patch did it wrong....so i'll make another one.

So once more with t() functions :)

Reviewed against #113. It seems to work and fixes the issue described.

We need screenshots and html comparisons to prove this issue has not broken anything - with compact mode enabled and disabled.

Here's also markup from the permissions page. No ID's as you can see:

Committed 9e163fc and pushed to 8.0.x. Thanks!

Thanks making the screenshots and for adding the beta evaluation for to the issue summary.