Problem/Motivation

We removed theme_link() #1985470: Remove theme_link() as part of a push to stop dedicating theme functions to tiny, single tag chunks of markup because it's overkill and would only get worse if we were to create a Twig template.

For links in particular, there have been many theme functions that are almost identical sort of piling up #1595614: [meta] Remove all the theme functions and templates in core that simply output a link. Replace with #type 'link' render arrays.

theme_more_link() is one of these functions that should not be converted to a Twig template.

Proposed resolution

Create #type 'more_link' that works much as #type 'link', but with a few extra default attributes set.

Remaining tasks

Commit patch.

User interface changes

n/a

API changes

Use #type 'more_link' instead of #theme 'more_link' in renderable arrays. The render array keys have changed as well to be in line with #type 'link'.

Original report by @thedavidmeister

Here is the current code for theme_more_link():

function theme_more_link($variables) {
  return '';
}

In order to reproduce this with #type = link, we need to make a render array with the following structure:

$more_link = array(
  '#type' => 'link',
  '#href' => 'admin/content',
  '#title' => t('More'),
  '#attributes' => array(
    '#title' => t('Show more content'),
  ),
  '#theme_wrappers' => array('container'),
  '#wrapper_container_attributes' => array(
    'class' => array('more-link'),
  ),
);

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

Related:
#1939098: Convert theme_more_link() to Twig

Files: 
CommentFileSizeAuthor
#142 interdiff.txt1.32 KBjoelpittet
#142 2031301-type-more-link-142.patch8.95 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,879 pass(es). View
#140 interdiff.txt854 bytesjoelpittet
#140 2031301-type-more-link-140.patch8.93 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,701 pass(es). View
#136 type-more-link-2031301-136.patch8.93 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,660 pass(es). View
#136 interdiff.txt6.88 KBjoelpittet
#132 Выделение_015.png4.34 KBandypost
#132 2031301-twig-theme-more-link-132.patch1.71 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,275 pass(es). View
#132 interdiff.txt554 bytesandypost
#130 2031301-twig-theme-more-link-130.patch1.32 KBjoelpittet
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,967 pass(es), 5 fail(s), and 0 exception(s). View

Comments

jenlampton’s picture

We may need to get this done before July 1st.

thedavidmeister’s picture

I'm just about to go to bed. @jenlampton, would you mind rolling a patch for this?

thedavidmeister’s picture

Issue summary: View changes

rela

jenlampton’s picture

Issue summary: View changes

code

jenlampton’s picture

Status: Active » Needs review
FileSize
6.21 KB
FAILED: [[SimpleTest]]: [MySQL] 56,358 pass(es), 1 fail(s), and 0 exception(s). View

first go

Status: Needs review » Needs work

The last submitted patch, core-remove_theme_more_link-2031301-3.patch, failed testing.

jenlampton’s picture

Status: Needs work » Needs review
FileSize
7.75 KB
PASSED: [[SimpleTest]]: [MySQL] 56,392 pass(es). View

missed one

ericrdb’s picture

Status: Needs review » Reviewed & tested by the community

Checked on following blocks

  • Aggregator Feed after pulling in content
  • Recent Forum topics
  • Recent content

There is a "More" link on all blocks. Works.

Cottser’s picture

Status: Reviewed & tested by the community » Needs work

Needs some code style touchups, thanks for giving this a test @ericrdb :)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.phpundefined
@@ -69,11 +69,21 @@ public function build() {
+        '#children' =>  drupal_render($more_link),

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorFeedBlock.phpundefined
@@ -70,11 +70,21 @@ public function build() {
+        '#children' =>  drupal_render($more_link),

+++ b/core/modules/forum/forum.moduleundefined
@@ -636,7 +636,21 @@ function forum_block_view_pre_render($elements) {
+      '#children' =>  drupal_render($more_link),

+++ b/core/modules/node/node.moduleundefined
@@ -1544,11 +1544,21 @@ function theme_node_recent_block($variables) {
+        '#children' =>  drupal_render($more_link),

+++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.phpundefined
@@ -34,7 +34,22 @@ public function onRequest(GetResponseEvent $event) {
+        '#children' =>  drupal_render($more_link),

Extra space between => and drupal_render()

+++ b/core/modules/forum/forum.moduleundefined
@@ -636,7 +636,21 @@ function forum_block_view_pre_render($elements) {
+      '#href' => 'forum', ¶

+++ b/core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.phpundefined
@@ -34,7 +34,22 @@ public function onRequest(GetResponseEvent $event) {
+        '#href' => 'user', ¶

Trailing whitespace should be removed per http://drupal.org/coding-standards#indenting.

thedavidmeister’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3.43 KB
7.74 KB
PASSED: [[SimpleTest]]: [MySQL] 56,370 pass(es). View

minor whitespace cleanup only as requested from Cottser. RTBC as per #6.

thedavidmeister’s picture

As I mentioned in #1833932: Convert theme_system_compact_link() into a #type link, I do think there could be value in adding a central function that builds a render array for a "more link" but I think this should be a followup rather than something done here. It's probably something that would end up under #1804488: [meta] Introduce a Theme Component Library.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

Fabianx’s picture

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Block/AggregatorCategoryBlock.phpundefined
@@ -69,11 +69,21 @@ public function build() {
+        '#children' => drupal_render($more_link),
...
+      $read_more = drupal_render($container);

Uhm, why?

Doesn't that defeat our lazy-render purpose?

thedavidmeister’s picture

#children needs to be a string if you want it to be set before sending it to drupal_render().

+      $container = array(
+        '#theme' => 'container',
+        '#children' => drupal_render($more_link),
+        '#attributes' => array(
+          'class' => array('more-link'),
+        ),
+      );

Changing:

'#children' => drupal_render($more_link);

to

'more_link' => $more_link;

Would probably work equally well.

catch’s picture

Status: Reviewed & tested by the community » Needs work

Yes let's do #12.

Pancho’s picture

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

Yes but let's first get the API change in #8 committed before API freeze coming into effect, and do the rest in a followup.

catch’s picture

Status: Reviewed & tested by the community » Needs work

No that's OK. We've explicitly said that some theme function -> template conversions can happen for a bit longer, and minor one-off theme function removal I think is OK for a little while too. API freeze was yesterday so this already slipped technically.

According to drupalcontrib no contrib modules use this at all http://drupalcontrib.org/api/drupal/drupal%21includes%21theme.inc/functi...

Pancho’s picture

Issue tags: -Needs followup

OK, so removing tag.
It's really hard to figure out what is allowed for "a bit longer" and what isn't. :)

catch’s picture

With something like this it's going to have to be case by case. This specific issue has the 'RTBC July 1st' tag on it, so it's definitely OK for a bit longer regardless of anything else anyway.

Eric_A’s picture

'more_link' => $more_link;

Would probably work equally well.

Wouldn't implementing the container theme hook as a #theme_wrappers property be even better? Or did that property got discouraged recently?

Either way #2031305: Remove theme_more_help_link() and replace with a #type link render array needs a follow-up I guess.

Eric_A’s picture

Status: Needs work » Needs review
FileSize
7.99 KB
FAILED: [[SimpleTest]]: [MySQL] 56,668 pass(es), 2 fail(s), and 0 exception(s). View
2.75 KB

Ah, we have attributes on both the child element and the container. Thus no #theme_wrappers.

Here's #12 done, then.

Eric_A’s picture

FileSize
7.98 KB
FAILED: [[SimpleTest]]: [MySQL] 56,475 pass(es), 6 fail(s), and 0 exception(s). View
3 KB
414 bytes

Missed one in forum.module...

Eric_A’s picture

So in fact in #19 I accidentally touched the pre-existing $container variable in forum_help() instead of the new container in forum_block_view_pre_render(). This bonus one-liner fix is still there in #20, not sure if it should be. Any big issues in the queue touching forum_help() that are ready and which would break because of this line?

Eric_A’s picture

Nevermind, it pre-existed since yesterday, and in fact this one-line would be the mentioned follow-up for #2031305: Remove theme_more_help_link() and replace with a #type link render array. Might as well do it here?

Status: Needs review » Needs work

The last submitted patch, 2031301-20.patch, failed testing.

Eric_A’s picture

Status: Needs work » Needs review
FileSize
8.14 KB
PASSED: [[SimpleTest]]: [MySQL] 56,467 pass(es). View
3.28 KB
Eric_A’s picture

Yay, patch is green now.
Since theme_container() doesn't render child elements we do need to use '#theme_wrappers' instead of '#theme' when the container render array has child elements.
From drupal_render():

If #theme is not present and the element has children, each child is itself rendered by a call to drupal_render(), and the results are concatenated.

thedavidmeister’s picture

Status: Needs review » Needs work

Ugh. #theme_wrappers and #theme sharing #attributes so you can't use them together in the same element. so broken... that means you couldn't convert theme_more_link() into #type 'more_link' and set container and link attributes separately as defaults.

I mean, it's nice that we're cleaning up the theme system, but we're paying for it in duplicated code :/

Anyway, review of the patch. I applied it cleanly, coding standards look good.

-        '#theme' => 'container',
-        '#children' => drupal_render($more_help_link),
+        '#theme_wrappers' => array('container'),
+        'more_help_link' => $more_help_link,

Why are we touching this? I don't think we should do this here. You are right though, 'container' should be #theme_wrappers only and we should be putting our new link render arrays in child elements to workaround the conflict with #attributes rather than early-rendering into #children.

/**
 * Markup generated by theme_more_link().
 */
.more-link {
  text-align: right; /* LTR */
}
[dir="rtl"] .more-link {
  text-align: left;
}

Patch misses this comment in system.theme.css

Other than that, I think this looks good.

thedavidmeister’s picture

thedavidmeister’s picture

Issue summary: View changes

output

jenlampton’s picture

Issue summary: View changes

update code

Fabianx’s picture

This is really broken (the patch is nice, but theme wrappers will kill us more in the future than help),

We should have a #type => container, #content => 'inner render element' instead ...

jenlampton’s picture

Based on https://drupal.org/node/2042285#comment-7668681, what should we do here?

Fabianx’s picture

Fabianx’s picture

.

jenlampton’s picture

Status: Needs work » Postponed
thedavidmeister’s picture

Title: Remove theme_more_link() and replace with #type link render arrays » Remove theme_more_link() and replace with #type 'more_link'
Status: Postponed » Needs work

It's in, so CNW.

As I said in #26, I'd personally actually like to see #theme 'more_link' converted to #type 'more_link' as we new should be able to build everything required for a "more link" in a single element without conflict from #attributes or forced nesting of child elements.

thedavidmeister’s picture

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

This introduces #type 'more_link' and uses the new #theme_wrappers syntax. I think it's a lot cleaner than what we've previously had to do here.

Status: Needs review » Needs work
Issue tags: -Twig, -theme system cleanup, -RTBC July 1

The last submitted patch, 2031301-34.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review
Issue tags: +Twig, +theme system cleanup, +RTBC July 1

#34: 2031301-34.patch queued for re-testing.

jibran’s picture

Code looks good to me nice clean up. I like new #type more_link.
I think we can add tests to check the output.
Minor issue

+++ b/core/modules/system/system.module
@@ -540,6 +540,16 @@ function system_element_info() {
+  $types['more_link'] = $types['link'];
+  $types['more_link'] += array(

Can we make this one liner for readability?

Other then this is RTBC for me.

thedavidmeister’s picture

Status: Needs review » Needs work

I think tests should go in the file introduced here #2058761: PHP notice when #attributes is not set with #theme_wrappers 'container' - if/when it lands I'll do a re-roll here.

Fabianx’s picture

Issue summary needs to be updated.

Cottser’s picture

Issue tags: +Needs reroll

Also needs a reroll.

thedavidmeister’s picture

Issue tags: +Needs tests

we can totally add tests for this.

Sorry, I completely forgot this was postponed on that other issue. In my head it was waiting for a review >.<

trogels’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.2 KB
FAILED: [[SimpleTest]]: [MySQL] 58,872 pass(es), 2 fail(s), and 7 exception(s). View

Patch rerolled. Didn't do anything for test though.

danquah’s picture

FileSize
7.2 KB
PASSED: [[SimpleTest]]: [MySQL] 58,426 pass(es). View

I believe #42 accidentally used the wrong feed-id reference that got changed back in fd9804d88, attached updated patch should fix this.

joelpittet’s picture

@jibran re #37 I don't think a one-liner in that case makes this more readable, I could be wrong, would you mind showing an example snippet?

@thedavidmeister re #41 which tests would you expect of these, I'll gladly write some.

This patch is looking really good, thanks for all who are involved!

jibran’s picture

@joelpittet I think it should look like this.

+++ b/core/modules/system/system.module
@@ -538,6 +538,16 @@ function system_element_info() {
+  $types['more_link'] = $types['link'];
+  $types['more_link'] += array(

I think it should look like this

+++ b/core/modules/system/system.module
@@ -538,6 +538,16 @@ function system_element_info() {
+  $types['more_link'] = $types['link'] + array(
thedavidmeister’s picture

Status: Needs review » Needs work

see /core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php for examples for tests.

Needs work until "Needs issue summary update" and "Needs tests" are cleaned up.

robynlgreen’s picture

FileSize
568 bytes
7.17 KB
PASSED: [[SimpleTest]]: [MySQL] 58,046 pass(es). View
+++ b/core/modules/system/system.module
@@ -538,6 +538,16 @@ function system_element_info() {
+  $types['more_link'] = $types['link'] + array(

Change made in attached patch

robynlgreen’s picture

Status: Needs work » Needs review
thedavidmeister’s picture

Status: Needs review » Needs work

Still needs work as per #46, but thanks for that update @robynlgreen :)

Would you be able to have a shot at updating the issue summary?

thedavidmeister’s picture

Issue summary: View changes

output

joelpittet’s picture

Status: Needs work » Needs review

47: 2031301-47.patch queued for re-testing.

Cottser’s picture

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

Tagging for reroll.

BarisW’s picture

I'm pretty sure we don't need a wrapper div around the link. Please have a look here for that discussion: #2099289: Do we need a div around a more link?

joelpittet’s picture

I'd like to say that too, just add the class to the link and call it a day. display: block that class if you need it to be a block element. More flexible and less divs.

Anybody disagree? I'll do that after the reroll if not.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
7.05 KB
FAILED: [[SimpleTest]]: [MySQL] 58,966 pass(es), 2 fail(s), and 1 exception(s). View

Here's the re-roll for #47

joelpittet’s picture

Issue tags: -Needs tests
FileSize
1.25 KB
8.16 KB
FAILED: [[SimpleTest]]: [MySQL] 59,093 pass(es), 1 fail(s), and 0 exception(s). View

I've added a test, @thedavidmeister will this suffice or does this test not work?

joelpittet’s picture

FileSize
2.39 KB
8.5 KB
FAILED: [[SimpleTest]]: [MySQL] 58,985 pass(es), 2 fail(s), and 0 exception(s). View

@BarisW This is the change to get rid of the div... though the test I wrote clobbers the link's default attribute class. This is because the passed in definition of #attributes takes over in the array union. I could write a new pre_render and some workaround property but that feels like overkill... any suggestions to make what I have here work?

The last submitted patch, 54: 2031301-54-type-more-link-reroll.patch, failed testing.

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 56: 2031301-56-type-more-link.patch, failed testing.

The last submitted patch, 55: 2031301-55-type-more-link.patch, failed testing.

BarisW’s picture

Wow @joelpittet, that was quick! :)

The proposal in the related issue is to use the class 'more' instead of 'more-link' (as the a element is obviously a link already). I have no idea why the tests fail, will look at it later today.

thedavidmeister’s picture

This is because the passed in definition of #attributes takes over in the array union. I could write a new pre_render and some workaround property but that feels like overkill... any suggestions to make what I have here work?

Nowpe, you can't avoid doing that. It is indeed a limitation in what #type can be used achieve and this highlights that #type has the potential in the future to be a lot more useful than it currently is - you can't even use it to provide a few default CSS classes on an element alongside existing classes.

Adding a pre_render would be a perf hit and increase the complexity of "more links" a fair bit, so no, that's not any more desirable than keeping the wrapper IMO.

I suspect this limitation is more likely the reason we still have wrapper DIVs for links than the wrappers themselves being desirable HTML.

As I understand, to achieve useful more links in both the sense of a consistent "template" for the structure of the links without making them inflexible, we need the wrapper OR to modify/extend how the #type system works.

Eric_A’s picture

Merging your #attributes with the defaults can be done by leveraging element_info_property(), just like field_ui_field_edit_form() merges #pre_render properties.

Of course attributes is a deeper structure, so you'll need to choose the correct deep merging to get it right.

joelpittet’s picture

@Eric_A where would you do this? Could you provide an example or maybe give this a try?

joelpittet’s picture

@thedavidmeister, maybe if I had a #default_attributes that I merge in the drupal_pre_render_link?

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.2 KB
FAILED: [[SimpleTest]]: [MySQL] 59,390 pass(es), 2 fail(s), and 0 exception(s). View
7.31 KB
FAILED: [[SimpleTest]]: [MySQL] 59,331 pass(es), 2 fail(s), and 0 exception(s). View
2.29 KB

How about this?

The last submitted patch, 66: 2031301-66-reroll-type-more-link.patch, failed testing.

The last submitted patch, 66: 2031301-66-type-more-link.patch, failed testing.

joelpittet’s picture

The last submitted patch, 66: 2031301-66-type-more-link.patch, failed testing.

joelpittet’s picture

I've run these tests locally in the CLI and they both pass
"Drupal\system\Tests\Common\RenderElementTypesTest"
and "Drupal\image\Tests\ImageFieldDisplayTest"

So I'm going to retest again.

joelpittet’s picture

joelpittet’s picture

If this fails the only thing I can think is the attributes are merged out of order and may need to change that to xpath or something else.

The last submitted patch, 66: 2031301-66-type-more-link.patch, failed testing.

joelpittet’s picture

Ok can anybody else confirm test bot failures? Seems one of the failures went away from those retests but still two remain which I think has to do with what I wrote in #73

Cottser’s picture

The last submitted patch, 66: 2031301-66-type-more-link.patch, failed testing.

mariacha1’s picture

Issue tags: -Needs issue summary update, -RTBC July 1
FileSize
7.54 KB
FAILED: [[SimpleTest]]: [MySQL] 63,088 pass(es), 2 fail(s), and 0 exception(s). View
678 bytes

(Cleaning up the the tags that are no longer relevant.)

Tests are succeeding for me, although the patch didn't apply cleanly since the theme_node_recent_block function from node.module doesn't seem to exist anymore.

Rerolling the patch without that bit.

If this latest patch fails (which seems likely), I'll try the xpath method suggested in [#73].

Status: Needs review » Needs work

The last submitted patch, 78: 2031301-78-type-more-link.patch, failed testing.

joelpittet’s picture

@mariacha1 thanks for re-rolling this, yeah the attribute/class order is likely to blame. If you need a hand I can take another stab. Good luck:)

joelpittet’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
8.28 KB
FAILED: [[SimpleTest]]: [MySQL] 64,366 pass(es), 3 fail(s), and 0 exception(s). View

Here's the xpath stuff with another test.

Status: Needs review » Needs work

The last submitted patch, 81: 2031301-type-more-link-81.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 81: 2031301-type-more-link-81.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
joelpittet’s picture

I'm curious about the testbot here... this passes in the UI and CLI locally.

php ./core/scripts/run-tests.sh --url http://d8.dev --verbose --color --class "\Drupal\system\Tests\Common\RenderElementTypesTest"

joelpittet’s picture

Status: Needs review » Needs work

The last submitted patch, 81: 2031301-type-more-link-81.patch, failed testing.

thedavidmeister’s picture

The last submitted patch, 81: 2031301-type-more-link-81.patch, failed testing.

joelpittet’s picture

The last submitted patch, 81: 2031301-type-more-link-81.patch, failed testing.

tim.plunkett’s picture

I'm now getting a conflict on core/modules/system/tests/modules/theme_test/lib/Drupal/theme_test/EventSubscriber/ThemeTestSubscriber.php, and it seems to be failing me consistently locally.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.32 KB
FAILED: [[SimpleTest]]: [MySQL] 64,434 pass(es), 3 fail(s), and 0 exception(s). View
1.91 KB

Re-rolled and fixed ThemeTestSubscriber.php. Crossing fingers.

Status: Needs review » Needs work

The last submitted patch, 94: 2031301-type-more-link-94.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.36 KB
PASSED: [[SimpleTest]]: [MySQL] 64,499 pass(es). View
2.43 KB

Not testing the path system, likely testbot is on a subdir. Thanks @larowlan and @timplunkett for helping me.

tim.plunkett’s picture

Well we should have at least one with an internal path, and one with route names... Because it sounds like there is a real bug here.

joelpittet’s picture

@tim.plunkett we should probably add those, to type=>link tests if they don't exist already as this just wraps that, don't ya think? We could try to get those working here too as a test to see what's up.

I guess instead of xpath I could actually make a type=>link equivalent to this extension and just render and compare outputs. May be mildly faster maybe and better suited test for this.

joelpittet’s picture

FileSize
10.06 KB
PASSED: [[SimpleTest]]: [MySQL] 64,611 pass(es). View
3.83 KB

More tests, I think this should clear up any doubts of bugs. @tim.plunkett let me know.

joelpittet’s picture

Assigned: Unassigned » tim.plunkett

I think we covered our bases on the tests and the fails were likely just an oversight of how the tests were setup on my part originally but I'm assigning to @tim.plunkett to have a second look just incase he still sees that potential bug mentioned in #97 and let me know if we still need those extra tests or should I slim them back down and leave those to #type=>link tests?

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Needs review » Needs work

The coverage looks good, but it no longer applies.

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.php
@@ -266,4 +268,81 @@ function testMaintenancePage() {
+        'expected' => '//a[@href="' . \Drupal::urlGenerator()->generate('router_test.1') . '" and @class="more-link" and text()="More"]',

This can be \Drupal::url().

I don't see any added docs for #default_attributes, those should be added to hook_element_info().

Finally, this may have been discussed before, but why is this not just 'link__more' or something? Is the extra type worth it?

joelpittet’s picture

@tim.plunkett thanks for reviewing.

  1. Good point, I'll change that after reroll, thanks.
  2. Where do docs go for default_attributes? Sorry I've yet to document that kind of thing.
  3. I don't think it was discussed but the reason I believe is because there is no longer 'theme_link' and without the #theme, type's don't go through the _theme() for the template suggestions. Though that would have been nice to just do
    {% set attributes.class = attributes.class|merge(['more-link'])%}
    <a{{ attributes }}>{{ text|default('More') }}</a>

Removed here #1985470: Remove theme_link()

tim.plunkett’s picture

103.2, as I said, see hook_element_info.
103.3, that kinda sucks. Suggestions like that were really useful :(

joelpittet’s picture

103.2, I see it now, for some reason that was tricky to find... not sure why:S

103.3. Mostly a performance issue, though I agree it was useful, maybe we should look at adding it back? Would like to see @thedavidmeister's take on that and maybe @c4rl and @Cottser too. It has implication for suggestions here as well: #2151107: Convert theme_system_compact_link() to Twig which I was recommending going the route of this issue, but if theme_link was back and used for #type=>link we could use it though that would mean defining it like:

$more_link = array(
  '#type' => 'more_link',
  '#href' => 'admin/content',
),

Would need to be:

$more_link = array(
  '#type' => 'link',
  '#theme' => 'more__link',
  '#href' => 'admin/content',
),

Which feels a bit more janky? Or do #type's take the same __ suggestion syntax?

Cottser’s picture

Issue tags: +Needs reroll

Just tagging for reroll.

mark.labrecque’s picture

Assigned: Unassigned » mark.labrecque
mark.labrecque’s picture

FileSize
10.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2031301-type-more-link-100_0.patch. Unable to apply patch. See the log in the details link for more information. View

Try this one

mark.labrecque’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 108: 2031301-type-more-link-100.patch, failed testing.

mark.labrecque’s picture

Status: Needs work » Needs review
mark.labrecque’s picture

FileSize
8.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,048 pass(es), 3 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 112: 2031301-type-more-link-111.patch, failed testing.

thedavidmeister’s picture

Just in response to #103 and #105.

theme_link() was never intended to be called directly, if you look at the internals of l() in D7 it performs sanitisation that was never in the theme function #1187032: theme_link needs to be clearer about saying not to call it directly so using #theme in this case skips some important "preprocessing" (not actual theme preprocessing, because there is no preprocess for links, for performance reasons).

If you've ever used #theme => 'link' in a renderable array you've been "doing it wrong" ;)

There are also performance implications of using #theme over #type, and the performance hit for trying to create a Twig template for individual links has caused multiple issue threads to become very long.

#1922454: Gut l(), fix theme('link'), have a working framework for consistently generated links inside and outside of Twig also has some relevant discussion.

I opened #2272577: Make #base_type a standard way to extend #type in renderable arrays. as well, which is tangentially related to those two comments.

joelpittet’s picture

Assigned: mark.labrecque » Unassigned
Status: Needs work » Needs review
FileSize
9.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2031301-type-more-link-115.patch. Unable to apply patch. See the log in the details link for more information. View

Reroll, @thedavidmeister, mind giving this a review if it comes back green?

RenderElementTypesTest comes back green locally so crossing fingers. @mark.labrecque thanks for giving that a try, maybe you can jump on some other theme conversion reviews when we get to Austin?

thedavidmeister’s picture

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

Seems fine to me. I've updated the issue summary as well.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 115: 2031301-type-more-link-115.patch, failed testing.

xjm’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
9.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,749 pass(es). View

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 118: 2031301-psr4-reroll.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review

118: 2031301-psr4-reroll.patch queued for re-testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Cottser’s picture

Assigned: Unassigned » joelpittet
Issue tags: +Needs change record
FileSize
9.6 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,862 pass(es). View
390 bytes

Thanks for the PSR-4 reroll @xjm :D

Minor doc fixup and assigning to @joelpittet who I am sprinting with to write up a small change record :)

joelpittet’s picture

Assigned: joelpittet » Unassigned
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs change record +Needs screenshots, +Novice

Change record here. https://drupal.org/node/2281851

Needs a screenshot because we changed the markup by removing the div.

joelpittet’s picture

Issue tags: -Needs screenshots +CSS
FileSize
43.25 KB
48.35 KB
.sidebar .block:after {
    clear: both;
    content: "";
    display: table;
}

This clearfix would make this work but right now with the more-link without a container it's difficult to align it to the right without float: right;

I know mortendk loves killing divs. The div is killed so the CSS needs to be adjusted to keep it from zombie re-appearing to get this patch in.

andypost’s picture

Status: Needs work » Reviewed & tested by the community

Also there's core/modules/views/templates/views-more.html.twig with the same issue but without patch
because its content the same (without DIV)
<a href="{{ more_url }}" class="more-link">{{ link_text }}</a>

so css could be fixed in own issue

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

@andypost Those screenshots show a regression, that needs to be fixed here.

joelpittet’s picture

@BarisW or @andypost we can add the div back in. That shouldn't be a problem. But I'll let someone stab at that regression first before I hastily add that back in.

lokapujya’s picture

The more link needs to be a block element so that it's space is counted as part of the height of the gray background of the block. Since we are changing the text alignment from left to right, it actually needs to be inline but inside a block element, so we need the container div. We can't use float on the anchor tag because that removes it from the block height calculation.

joelpittet’s picture

@lokapujya thanks, I agree, too bad we couldn't get rid of that div. So we have a fork in the road. We could try to hack this into #type link with a wrapper or something...

Though I think more straight forward would be to just convert the theme function to twig template. What do you think?

joelpittet’s picture

Title: Remove theme_more_link() and replace with #type 'more_link' » Convert theme_more_link() to more-link.html.twig
Status: Needs work » Needs review
Issue tags: +Needs profiling, +Needs manual testing
FileSize
1.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,967 pass(es), 5 fail(s), and 0 exception(s). View

Ok so nobody has a solution, so going to plan B, straight conversion.

Sorry for the long way around...

Status: Needs review » Needs work

The last submitted patch, 130: 2031301-twig-theme-more-link-130.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs manual testing
FileSize
554 bytes
1.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,275 pass(es). View
4.34 KB

You need to update drupal_common_theme() too

Tested manually - no regressions

Just need profiling

joelpittet’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -CSS, -Needs profiling

Thanks for fixing that andypost!

Scenario:
11 more links on the page from 11 blocks. 1 Active forum topics and 10 aggregator feeds on the homepage.

=== 8.x..8.x compared (53d4352fc0c61..53d435bff29d8):

ct  : 112,018|112,018|0|0.0%
wt  : 732,455|735,114|2,659|0.4%
cpu : 725,266|728,265|2,999|0.4%
mu  : 50,849,232|50,849,232|0|0.0%
pmu : 50,932,488|50,932,488|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53d4352fc0c61&...

=== 8.x..2031301-twig-theme-more-link compared (53d4352fc0c61..53d436500e2ac):

ct  : 112,018|112,101|83|0.1%
wt  : 732,455|739,156|6,701|0.9%
cpu : 725,266|732,072|6,806|0.9%
mu  : 50,849,232|50,889,440|40,208|0.1%
pmu : 50,932,488|50,971,280|38,792|0.1%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53d4352fc0c61&...

alexpott’s picture

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

So the issue summary needs an update an maybe we need more consensus on converting to a twig template - which was introduced in #130

joelpittet’s picture

Issue summary: View changes

@thedavidmeister and the other followers, any objections to just theme to twig template? If so please provide some reason and know that we went hard to make this a #type until about #126 where that superfluous div became necessary from a theming structural perspective.

Leaving the Issue Summary until a few people review what transpired.

joelpittet’s picture

Title: Convert theme_more_link() to more-link.html.twig » Replace theme_more_link() and replace with #type 'more_link'
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
6.88 KB
8.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,660 pass(es). View

Rerolled #122 with a suggestion from @thedavidmeister to use #theme_wrappers.

And moved the definition to a class called MoreLink, which was pretty cool because I can extend Link!

thedavidmeister’s picture

I like the patch in #136, it's like an OO version of #34:)

thedavidmeister’s picture

Also, I'd like to re-iterate what I said in chat, which is that if we decide we've reached the point the only way to cleanly wrap a data structure in a div with an attribute is to write up a whole new Twig template, that's a Drupal-wtf fail.

Cottser’s picture

Status: Needs review » Needs work

Wow this looks really great to me. The OO render element stuff is awesome. One thing held me back from the RTBC button:

+++ b/core/modules/system/src/Tests/Common/RenderElementTypesTest.php
@@ -101,4 +103,85 @@ function testHtmlTag() {
+    foreach($elements as $element) {
+      $xml = new \SimpleXMLElement(render($element['value']));
+      $result = $xml->xpath($element['expected']);
+      $this->assertTrue($result, '"' . $element['name'] . '" input rendered correctly by drupal_render().');
+    }

drupal_render() instead of render() please.

Edit: Also I revised the change record to reflect the latest patch a bit better.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.93 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,701 pass(es). View
854 bytes

render to drupal_render as per #139 Thanks @Cottser.

Cottser’s picture

Issue summary: View changes

Thanks @joelpittet!

Nice test coverage and docs and I just updated the issue summary to reflect the latest patch. One more thing:

+++ b/core/lib/Drupal/Core/Render/Element/MoreLink.php
@@ -0,0 +1,33 @@
+      '#title' => t('More'),

+++ b/core/modules/forum/src/Plugin/Block/ForumBlockBase.php
@@ -26,9 +26,9 @@ public function build() {
+        '#attributes' => array('title' => t('Read the latest forum topics.')),

I missed these before, can these both use $this->t()?

joelpittet’s picture

FileSize
8.95 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,879 pass(es). View
1.32 KB

Hmm it should be able to... it extends from StringTranslationTrait:)

Cottser’s picture

Status: Needs review » Reviewed & tested by the community

If the bot likes it so do I!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Nice clean-up, and test coverage, too!

Committed and pushed to 8.x. Thanks!

  • webchick committed 946aee4 on 8.0.x
    Issue #2031301 by andypost, Cottser, xjm, mark.labrecque, mariacha1,...

Status: Fixed » Closed (fixed)

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

fgm’s picture