Problem/Motivation

We want to get rid of theme_system_compact_link() one way or another.

Proposed resolution

Convert it to a render element #type. A Twig template is overkill. The output can be tested by navigating to /admin/config, /admin/index, /admin/people/permissions.

The markup changes slightly at /admin/people/permissions. Because the element is inside a form the link wrapper has a form-wrapper class added. This doesn't affect the functionality or appearance.

Remaining tasks

None

User interface changes

n/a

API changes

theme_system_compact_link() removed.

Before:

$variables['system_compact_link'] = array(
  '#theme' => 'system_compact_link',
);

After:

$variables['system_compact_link'] = array(
  '#type' => 'system_compact_link',
);

Original report by @jenlampton

Original code:

function theme_system_compact_link() {
  $output = '';

  return $output;
}

Render array that will achieve the same thing:

$system_compact_link = array(
    '#type' => 'link',
    '#href' => 'admin/compact/off',
    '#title' => t('Show descriptions'),
    '#attributes' => array(
      'title' => t('Expand layout to include descriptions.'),
    ),
    '#options' => array('query' => drupal_get_destination()),
    '#theme_wrappers' => array('container'),
    '#wrapper_container_attributes' => array(
      'class' => array('compact-link'),
    ),
  );
  if (!system_admin_compact_mode()) {
    $system_compact_link['#href'] = 'admin/compact/on';
    $system_compact_link['#title'] = t('Hide descriptions');
    $system_compact_link['#attributes']['title'] = t('Compress layout by hiding descriptions.');
  }

  $output .= drupal_render($system_compact_link);

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because parent item #1757550: [Meta] Convert core theme functions to Twig templates is a task
Issue priority Major because parent item #1757550: [Meta] Convert core theme functions to Twig templates is Major.
Prioritized changes The main goal of this issue is to follow up on #1987410: [meta] system.module - Convert theme_ functions to Twig. These issues were recently moved to Major, but since this issue was already closed it likely was not updated.
Disruption This change should not be considered disruptive because markup and functionality are not changing, just the method used to generate the markup.
Files: 
CommentFileSizeAuthor
#151 Screen Shot 2014-11-25 at 3.56.54 PM.png20.5 KBlauriii
#151 Screen Shot 2014-11-25 at 3.56.32 PM.png56.05 KBlauriii
#151 Screen Shot 2014-11-25 at 3.56.22 PM.png47.11 KBlauriii
#151 Screen Shot 2014-11-25 at 3.56.03 PM.png35.87 KBlauriii
#151 Screen Shot 2014-11-25 at 3.55.52 PM.png57.09 KBlauriii
#148 interdiff-1833932-146-148.txt1.43 KBlanchez
#148 convert-1833932-148.patch8.38 KBlanchez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,742 pass(es). View
#146 convert-1833932-146.patch8.53 KBlanchez
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,723 pass(es). View
#139 convert-1833932-139.patch8.41 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,312 pass(es), 107 fail(s), and 44 exception(s). View
#137 interdiff.txt1.55 KBiMiksu
#137 convert-1833932-137.patch70.19 KBiMiksu
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,316 pass(es), 107 fail(s), and 45 exception(s). View
#127 interdiff-1833932-123-127.txt1002 byteslauriii
#127 convert-1833932-127.patch8.53 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,408 pass(es). View
#104 interdiff-1833932-95-104.txt4.4 KBlauriii
#51 interdiff.txt1.75 KBjoelpittet
#51 type_system_compact_link-1833932-51.patch6.12 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,073 pass(es). View
#46 remove-1833932-46.patch5.6 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,035 pass(es), 34 fail(s), and 7 exception(s). View
#38 1833932-38.patch5.3 KBlokapujya
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,324 pass(es). View
#20 interdiff.txt1.54 KBEric_A
#20 core-remove_theme_system_compact_link-1833932-20.patch5.81 KBEric_A
PASSED: [[SimpleTest]]: [MySQL] 57,091 pass(es). View
#15 core-remove_theme_system_compact_link-1833932-13.patch5.78 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 56,301 pass(es). View
#15 interdiff.txt1.16 KBjenlampton
#10 core-remove_theme_system_compact_link-1833932-9.patch5.78 KBericrdb
PASSED: [[SimpleTest]]: [MySQL] 56,612 pass(es). View
#7 core-remove_theme_system_compact_link-1833932-4.patch5.78 KBericrdb
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-remove_theme_system_compact_link-1833932-4.patch. Unable to apply patch. See the log in the details link for more information. View
#3 core-remove_theme_system_compact_link-1833932-3.patch5.79 KBjenlampton
PASSED: [[SimpleTest]]: [MySQL] 56,374 pass(es). View

Comments

jenlampton’s picture

Title: Remove theme_system_admin_compact_mode() from core » Remove theme_system_admin_compact_link() from core

whoopsie, wrong theme func name

jenlampton’s picture

Issue summary: View changes

wring name fix

thedavidmeister’s picture

Title: Remove theme_system_admin_compact_link() from core » Remove theme_system_compact_link() and replace with a #type link render array
thedavidmeister’s picture

Issue summary: View changes

add twig example

jenlampton’s picture

Issue summary: View changes

code

jenlampton’s picture

Issue summary: View changes

more code

jenlampton’s picture

Issue summary: View changes

code

jenlampton’s picture

Issue summary: View changes

correct code

jenlampton’s picture

Issue summary: View changes

more code

jenlampton’s picture

Status: Active » Needs review
FileSize
5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 56,374 pass(es). View

here we go

ericrdb’s picture

Status: Needs review » Reviewed & tested by the community

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.

thedavidmeister’s picture

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

Cottser’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice

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.

ericrdb’s picture

Status: Needs work » Needs review
FileSize
5.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch core-remove_theme_system_compact_link-1833932-4.patch. Unable to apply patch. See the log in the details link for more information. View

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

Will this need further review?

Status: Needs review » Needs work

The last submitted patch, core-remove_theme_system_compact_link-1833932-4.patch, failed testing.

Cottser’s picture

Issue tags: +Needs reroll

@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 :)

ericrdb’s picture

Issue tags: -Needs reroll
FileSize
5.78 KB
PASSED: [[SimpleTest]]: [MySQL] 56,612 pass(es). View

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

Trying again.

ericrdb’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Novice, -theme system cleanup, -Theme Component Library

The last submitted patch, core-remove_theme_system_compact_link-1833932-9.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +theme system cleanup, +Theme Component Library
Cottser’s picture

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

jenlampton’s picture

FileSize
1.16 KB
5.78 KB
PASSED: [[SimpleTest]]: [MySQL] 56,301 pass(es). View

rerolled :)

ericrdb’s picture

Cool - thanks for the patch links too @Cottser.

Cottser’s picture

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

thedavidmeister’s picture

Status: Needs review » Needs work

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

double space before drupal_render

+ $output = drupal_render($system_compact);

double space before =

Eric_A’s picture

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

Eric_A’s picture

Status: Needs work » Needs review
FileSize
5.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,091 pass(es). View
1.54 KB

Addresses #18 and #19.

jenlampton’s picture

it also might be worth fixing theme_container so that it can render it's contents, but that's a follow-up issue.

If we're going to use a theme_wrapper, then we certainly don't need two arrays.

+  $system_compact_link = array(
+    '#type' => 'link',
+    '#href' => 'admin/compact/off',
+    '#title' => t('Show descriptions'),
+    '#attributes' => array(
+      'title' => t('Expand layout to include descriptions.'),
+    ),
+    '#options' => array('query' => drupal_get_destination()),
+  );
+  if (!system_admin_compact_mode()) {
+    $system_compact_link['#href'] = 'admin/compact/on';
+    $system_compact_link['#title'] = t('Hide descriptions');
+    $system_compact_link['#attributes']['title'] = t('Compress layout by hiding descriptions.');
+  }
+  $system_compact = array(
+    '#theme_wrappers' => array('container'),
+    'system_compact_link' => $system_compact_link,
+    '#attributes' => array(
+      'class' => array('compact-link'),
+    ),
+  );

How about something like this:

 $system_compact_link = array(
   '#type' => 'link',
   '#href' => 'admin/compact/off',
   '#title' => t('Show descriptions'),
   '#attributes' => array(
     'title' => t('Expand layout to include descriptions.'),
   ),
   '#options' => array('query' => drupal_get_destination()),
   '#theme_wrappers' => array('container'),
   '#wrapper_container_attributes' => array(
     'class' => array('compact-link'),
   ),
 );
 if (!system_admin_compact_mode()) {
   $system_compact_link['#href'] = 'admin/compact/on';
   $system_compact_link['#title'] = t('Hide descriptions');
   $system_compact_link['#attributes']['title'] = t('Compress layout by hiding descriptions.');
 }

Also, this patch should depend on #2042285: #theme_wrappers should be able to have a unique set of variables per wrapper hook

jenlampton’s picture

Issue summary: View changes

better code

jenlampton’s picture

Issue summary: View changes

example code update

areke’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

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

mikemiles86’s picture

Assigned: Unassigned » mikemiles86

rerolling patch.

mikemiles86’s picture

Assigned: mikemiles86 » Unassigned
Status: Needs work » Needs review
FileSize
1.56 KB
PASSED: [[SimpleTest]]: [MySQL] 59,285 pass(es). View

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

Cottser’s picture

Issue tags: -Needs reroll

Thanks @mikemiles86!

parthipanramesh’s picture

Status: Needs review » Reviewed & tested by the community

Patch works fine. Thank you!

webchick’s picture

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

lokapujya’s picture

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.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Needs work based on the above.

lokapujya’s picture

Assigned: Unassigned » lokapujya
Status: Needs work » Needs review
FileSize
6.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,902 pass(es). View

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

lokapujya’s picture

FileSize
5.29 KB
FAILED: [[SimpleTest]]: [MySQL] 59,725 pass(es), 2 fail(s), and 1 exception(s). View

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

lokapujya’s picture

lokapujya’s picture

FileSize
5.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1833932-33.patch. Unable to apply patch. See the log in the details link for more information. View

Renaming the last patch.

The last submitted patch, 31: 1833932.31.patch, failed testing.

mikemiles86’s picture

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

lokapujya’s picture

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

Status: Needs review » Needs work

The last submitted patch, 33: 1833932-33.patch, failed testing.

lokapujya’s picture

Status: Needs work » Needs review
FileSize
5.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,324 pass(es). View

reroll.

lokapujya’s picture

Status: Needs review » Needs work

Look into using #route_name instead of #href.

longwave’s picture

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

joelpittet’s picture

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'

lokapujya’s picture

Assigned: lokapujya » Unassigned

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

Cottser’s picture

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

joelpittet’s picture

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

joelpittet’s picture

Assigned: Unassigned » joelpittet
joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Needs work » Needs review
Issue tags: +Needs markup review
FileSize
5.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,035 pass(es), 34 fail(s), and 7 exception(s). View

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.

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 46: remove-1833932-46.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 46: remove-1833932-46.patch, failed testing.

joelpittet’s picture

Title: Remove theme_system_compact_link() and replace with a #type link render array » Convert theme_system_compact_link() into a #type link
Status: Needs work » Needs review
FileSize
6.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,073 pass(es). View
1.75 KB

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.

Cottser’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: -Needs manual testing +Twig

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

Cottser’s picture

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

Cottser’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
6.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,255 pass(es). View
947 bytes

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

Cottser’s picture

Cottser’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
6.17 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,256 pass(es). View
1.12 KB

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' -->
thedavidmeister’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

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

lauriii’s picture

Issue tags: -Needs tests
FileSize
13.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1833932-58.patch. Unable to apply patch. See the log in the details link for more information. View

Added some tests there.

lauriii’s picture

Status: Needs work » Needs review

:<

Status: Needs review » Needs work

The last submitted patch, 58: 1833932-58.patch, failed testing.

Status: Needs work » Needs review

lauriii queued 58: 1833932-58.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 58: 1833932-58.patch, failed testing.

lauriii’s picture

FileSize
20.3 KB

Rerolled

lauriii’s picture

Assigned: Unassigned » lauriii

Ok I'm gonna fix this mess

lauriii’s picture

Status: Needs work » Needs review
FileSize
5.88 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,572 pass(es), 20 fail(s), and 1 exception(s). View

Hopefully I fixed my own mess. Forget last 2 patches

Status: Needs review » Needs work

The last submitted patch, 65: 1833932-65.patch, failed testing.

joelpittet’s picture

Assigned: lauriii » joelpittet

Fixing, thanks for the test!

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.42 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,713 pass(es). View

The system compact link was missing from patch #65

lokapujya’s picture

Status: Needs review » Reviewed & tested by the community

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.

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Reviewed & tested by the community » Needs work

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?

lokapujya’s picture

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?

lauriii’s picture

Status: Needs work » Needs review
FileSize
8.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,735 pass(es). View
608 bytes
joelpittet’s picture

@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?

lokapujya’s picture

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.

lokapujya’s picture

Status: Needs review » Needs work
+++ 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.

Cottser’s picture

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

cilefen’s picture

Status: Needs work » Needs review
FileSize
8.57 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,661 pass(es). View

Rerolled #72.

cilefen’s picture

FileSize
608 bytes
8.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,661 pass(es). View

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

joelpittet’s picture

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

tim.plunkett’s picture

Status: Needs review » Needs work
  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.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.12 KB
8.58 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,796 pass(es), 1 fail(s), and 0 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 81: 1833932-81.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
8.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1833932-83.patch. Unable to apply patch. See the log in the details link for more information. View

Status: Needs review » Needs work

The last submitted patch, 83: 1833932-83.patch, failed testing.

Status: Needs work » Needs review

lauriii queued 83: 1833932-83.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 83: 1833932-83.patch, failed testing.

tim.plunkett’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
0 bytes
8.58 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,808 pass(es). View

Removed .swp from the patch

tim.plunkett’s picture

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

joelpittet’s picture

Issue tags: +Needs manual testing

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.

Xen’s picture

Status: Needs review » Reviewed & tested by the community

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.

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

@xen thanks for a review!

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
420 bytes
8.61 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,867 pass(es). View

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.

Cottser’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing

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!

lokapujya’s picture

Status: Needs work » Needs review
FileSize
8.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,030 pass(es). View
988 bytes

Updated the comment and the one for More Link.

Status: Needs review » Needs work

The last submitted patch, 95: 1833932-95.patch, failed testing.

Cottser’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community

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

alexpott queued 95: 1833932-95.patch for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record
  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.

joelpittet’s picture

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

subhojit777’s picture

Assigned: Unassigned » subhojit777
subhojit777’s picture

Issue tags: +Needs reroll
subhojit777’s picture

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

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
9.64 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 104: 1833932-104.patch, failed testing.

Status: Needs work » Needs review

cilefen queued 104: 1833932-104.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 104: 1833932-104.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
1.4 KB
9.62 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,363 pass(es), 96 fail(s), and 7 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, 108: convert-1833932-108.patch, failed testing.

lauriii’s picture

Assigned: subhojit777 » Unassigned
Status: Needs work » Needs review
FileSize
9.66 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,480 pass(es). View
1.42 KB

Lets try this out..

subhojit777’s picture

Issue tags: -Needs reroll
mikispeed’s picture

  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
lauriii’s picture

Status: Needs review » Needs work
balagan’s picture

Assigned: Unassigned » balagan
lauriii’s picture

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

lauriii’s picture

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

balagan’s picture

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

balagan’s picture

Assigned: balagan » Unassigned
joelpittet’s picture

@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?

mikispeed’s picture

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

mikispeed’s picture

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

joelpittet’s picture

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

mikispeed’s picture

FileSize
785 bytes
8.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,986 pass(es). View

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

lauriii’s picture

Status: Needs work » Needs review

Lets see what test bot says :)

akalata’s picture

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

akalata’s picture

Status: Needs review » Needs work
lauriii’s picture

Status: Needs work » Needs review
FileSize
8.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,408 pass(es). View
1002 bytes

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?

akalata’s picture

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

akalata’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Twig, -Needs change record

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.

lauriii’s picture

Issue tags: +Twig
alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

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.

akalata’s picture

akalata’s picture

Status: Needs work » Reviewed & tested by the community
akalata’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  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.

iMiksu’s picture

Assigned: Unassigned » iMiksu
iMiksu’s picture

Assigned: iMiksu » Unassigned
Status: Needs work » Needs review
FileSize
70.19 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,316 pass(es), 107 fail(s), and 45 exception(s). View
1.55 KB

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

iMiksu’s picture

Assigned: Unassigned » iMiksu
Status: Needs review » Needs work

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

iMiksu’s picture

Assigned: iMiksu » Unassigned
Status: Needs work » Needs review
FileSize
8.41 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,312 pass(es), 107 fail(s), and 44 exception(s). View

There you go. Previous interdiff still applies.

The last submitted patch, 137: convert-1833932-137.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 139: convert-1833932-139.patch, failed testing.

lauriii’s picture

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

lauriii’s picture

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.

alexpott’s picture

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.

lanchez’s picture

Assigned: Unassigned » lanchez

I'll test that out.

lanchez’s picture

Assigned: lanchez » Unassigned
Status: Needs work » Needs review
FileSize
8.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,723 pass(es). View

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

lanchez’s picture

Assigned: Unassigned » lanchez
Status: Needs review » Needs work

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

lanchez’s picture

Assigned: lanchez » Unassigned
Status: Needs work » Needs review
FileSize
8.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,742 pass(es). View
1.43 KB

So once more with t() functions :)

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs screenshots

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

lauriii’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs screenshots

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

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

  • alexpott committed 9e163fc on 8.0.x
    Issue #1833932 by lauriii, lokapujya, cilefen, Cottser, joelpittet,...

Status: Fixed » Closed (fixed)

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