Replace theme_more_link() and replace with #type 'more_link'

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

code

first go

missed one

Checked on following blocks

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

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

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.

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

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

Yes let's do #12.

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.

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

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

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.

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

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

Here's #12 done, then.

Missed one in forum.module...

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?

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?

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.

update code

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

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

I think getting #2042285: #theme_wrappers should be able to have a unique set of variables per wrapper hook in would be good to make this simpler.

.

okay, postponing until #2042285: #theme_wrappers should be able to have a unique set of variables per wrapper hook is in

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

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.

Issue summary needs to be updated.

Also needs a reroll.

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

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

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

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

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

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

Tagging for reroll.

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?

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.

Here's the re-roll for #47

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

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

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.

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.

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

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

How about this?

66: 2031301-66-type-more-link.patch queued for re-testing.

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.

66: 2031301-66-type-more-link.patch queued for re-testing.

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.

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

66: 2031301-66-type-more-link.patch queued for re-testing.

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

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

Here's the xpath stuff with another test.

81: 2031301-type-more-link-81.patch queued for re-testing.

81: 2031301-type-more-link-81.patch queued for re-testing.

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"

Proofs

81: 2031301-type-more-link-81.patch queued for re-testing.

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.

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

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

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.

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

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

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?

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?

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

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

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?

Just tagging for reroll.

Try this one

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?

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

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

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

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

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

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

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

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

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

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.

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

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

Sorry for the long way around...

You need to update drupal_common_theme() too

Tested manually - no regressions

Just need 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&...

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

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

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!

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.

render to drupal_render as per #139 Thanks @Cottser.

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

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

If the bot likes it so do I!

Nice clean-up, and test coverage, too!

Committed and pushed to 8.x. Thanks!

This breaks #1136680: #type 'more_link' - previously theme_more_link() - should have more context, which seem to still be relevant. Adding reference to it.