Convert views/templates/views-more.tpl.php to twig

Separated from meta patch, first draft

Tagging.

Oh,... all of these issues do exist :)

If we do that, we should remove the original theme function in the same issue, or?

Trying to keep phptemplate engine running at the same time so try to keep the theme() working for now.

Thank you for that information!
Oh, twig doesn't hook into theme() yet (which afaik template engines should be already possible), never would have expected that.

From the actual template file this one looks perfect, though i have no idea of twig best practices etc.

The only ones I think need some love are the views templates I marked as Needs Work because I haven't gotten around to writing the preprocessor to provide the right variables to the template. But seeing as views is already using tpl.php files instead of theme() functions this should not be necessary just needs testing:)

Committed, thanks!

Moving to core queue

giving it a first try

Fix some twig standard issues

@chrisjlee the interdiff changes look good but I think the patch got messed, I have done that before too but I can't remember what I did.

It's probably because the files are added to the index but you didn't diff the cached index changes in git. So you need to use the --cached option with the diff, or use a branch to diff between that and 8.x. The failure is a random one too :)

Or because it's an empty patch. Doh!

I reattached, hopefully, a non-empty patch. Reuploading #12

That is what I was saying, the reasons you would have an empty patch.

+++ b/core/modules/views/templates/views-more.html.twigundefined
@@ -0,0 +1,18 @@
+ * Default theme implementation for a more link.

It's a views more link ... we should certainly name that in the @file.

@chrisjlee Nice work! It's green:)

I agree with @dawehner to add "views more link" or is it "views' more link" ? English v Technology!

Thanks for the feedback and for the positive encouragement (@joelpettit).

Also capitalized views (Views) as per (http://drupal.org/files/1843746-views-view-field-9.patch) does it there.

Perfect!

(Reviewing the second patch, http://drupal.org/files/1843742-convert-views-more-twig-20_0.patch)

+++ b/core/modules/views/templates/views-more.html.twigundefined
@@ -0,0 +1,18 @@
+ * - more_url: the url for the Views more link.
+ * - link_text: the text for the Views more link.

Please replace "the url" with "The URL", and "the text" with "The text", per http://drupal.org/node/1354#drupal.

Great work @chrisjlee and @joelpittet!

Can go back to RTBC after #24 is fixed :)

Thanks cottser i will fix this

It was brought up in #1843740-21: Convert views/templates/views-exposed-form.tpl.php to twig that the template should be changed from views-more.html.twig to views-view-more.html.twig.

@chrisjlee - can you take care of that too please? Just rename the file and update views_theme() I think.

There's also this CSS class in Drupal\views\Plugin\views\field\FieldPluginBase::render_text() that could be replaced like in #1843740-33: Convert views/templates/views-exposed-form.tpl.php to twig:

$more_link = l($more_link_text, $more_link_path, array('attributes' => array('class' => array('views-more-link'))));

Thanks!

Yes sure thing.

Attached are for changes 21-26 Did the docs changes.

Seperated out the patches.
Changes as requested on #27

And not sure if it really matters should we edit the issue summary title?

It doesn't seem like this template is even being used?

I just grepped and found another 'views_more' string in Drupal\views\Plugin\views\display\DisplayPluginBase::renderMoreLink(), and I think that's the answer to @steveoliver's question as well.

$theme = views_theme_functions('views_more', $this->view, $this->view->display_handler->display);
$path = check_url(url($path, $url_options));

return theme($theme, array('more_url' => $path, 'link_text' => check_plain($this->useMoreText()), 'view' => $this->view));

So one more string to be changed. Thanks for your work on this @chrisjlee!

Thanks Cottser for your help!

@steveoliver out of curiousity, how did you know the template wasn't running and providing a false positive?

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/field/FieldPluginBase.phpundefined
@@ -1265,7 +1265,7 @@ function render_text($alter) {
+        $more_link = l($more_link_text, $more_link_path, array('attributes' => array('class' => array('views-view-more-link'))));

That's not the place where the theme function is called. The place is in DisplayPluginBase::renderMoreLink(). This here is another kind of more link.

@dawehner - So that class altered in FieldPluginBase should be left alone then?

Is there a reason Views needs its own special template for this and can't just re-use http://api.drupal.org/api/drupal/core%21includes%21theme.inc/function/th... ?

theme_more_link doesn't have configurable text, just a title. Although massive this could be added to that?

I think we should absolutely do that consolidation (#37/#38), not yet though. Convert first.

Tagging.

This should probably just be a straight conversion, not renaming the files and such.

Try this on for size...

I agree with #41, let's do a follow-up if we want to rename this, but I think it makes sense as is - @webthingee put it well in #1843740-40: Convert views/templates/views-exposed-form.tpl.php to twig.

Beside from the "needs manual testing" tag, this seems fine.

review for #41:

This looks very close to being ready for manual testing (which we need instructions on how to do).

+ * Default theme implementation for a Views more link.

Is it a Views more link. or a Views "more" link.?

Also, there's no template_preprocess_views_more() function but we still want a @see template_preprocess() and an @ingroup themeable in the template documentation.

As a followup issue we might want to look at the case when more_url is NULL (since that's the default value) - this isn't handled in the original tpl.php so I don't think we should touch it here. It makes sense to me that the whole template should not render anything unless more_url is not empty.

Thanks @thedavidmeister, tagging the tweaks in #45 as a novice task.

If you'd like to work on this, please assign the issue to yourself. If you get stuck, drop by #drupal-twig on IRC or post your questions here. Thanks!

Implemented changes from #45 to patch from #41. Please Review.

Great, thanks!

+ * - more_url: The URL for the Views more link.
+ * - link_text: The text for the Views more link.

Missed a couple of "Views more link" references but this is looking pretty good from a docs perspective now.

This issue needs steps to manually test added to the issue summary.

Implemented #48. Please review.

#49 looks good to me. Cottser was saying in IRC earlier that + * @ingroup views_templates lines might be unwanted later, but for now I'm happy.

As I said in #48 this now needs manual test steps written in the summary and for somebody to actually do the markup testing.

@dawehner/@damiankloip - should we be keeping @ingroup views_templates? I think the Views and Views UI templates should definitely have @ingroup themeable but not sure if we're keeping the other group.

The functionality itself is broken, so we have to fix that: #1970136: Read more link isn't rendered

#52 - so you're saying this patch needs work, or that we can't manually test it until that linked issue is resolved?

Yeah the second point.

sure.

We need to get this in, whether the functionality behind it is broken or not. Let's remove @ingroup views_templates and lowercase 'View' and 'Views' please :)

If necessary we can do something like manually call the theme function in page.tpl to ensure the markup matches. theme('views_more', …) or create a render array and drupal_render() it.

Fixes from #56 - lowercase v and no @group views_templates

Seems fair. I'll do some manual testing and hopefully RTBC this.

alrighty then.

Steps to test:

1. Add two articles
2. Edit front page view, set pager settings to unconditionally show the "more" link and show 1 article per page
3. Go to the front page

HTML with patch in #58 applied:

      <div class="more-link">
  <a href="/d8html/node">
    
  </a>
</div>

Note that I set the 'more' text through the views UI to be "asdfasdf" and also left it blank and the output was not modified. Editing the template to replace {{ link_text }} does make something appear.

We need to get this in, whether the functionality behind it is broken or not.

This is pretty broken.

Give me a minute to check the functionality pre-patch...

Updated issue summary.

HTML from pre-patch with both empty "more" settings and "asdfasdf" for the more text:

<div class="more-link">
  <a href="/d8raw/node">
      </a>
</div>

so, the problem isn't being introduced by Twig. RTBC then? ;)

If necessary we can do something like manually call the theme function in page.tpl

FWIW I don't think this would help with the problem here, apparently the problem is actually the view calling the theme function and sending the wrong variables.

I think as long as it's broken in the same way. @dawehner?

Edit: read too quickly, I will do a page.tpl test as proposed.

#64 - I'm totally happy to have this RTBC since the breakage has zero to do with the Twiggy-ness of the template.

The template conversion itself is fine and doesn't need to be blocked on #1970136: Read more link isn't rendered, I agree with the RTBC.

Code added to page.tpl.php:

  print theme('views_more', array('more_url' => 'node/1', 'link_text' => 'other more link text'));
  print theme('views_more', array('more_url' => 'node/1'));

Before:

<div class="more-link">
  <a href="node/1">
    other more link text  </a>
</div>
    
<div class="more-link">
  <a href="node/1">
    more  </a>
</div>

After:

    <div class="more-link">
  <a href="node/1">
    other more link text
  </a>
</div>
    <div class="more-link">
  <a href="node/1">
    more
  </a>
</div>

Tagging for profiling.

Assigning to me for profiling.

#1970136: Read more link isn't rendered was fixed, so the read more link is working again.

Results from profiling a front page with full node listing, 1 per page and a read more link:

=== 8.x..8.x compared (519c0a969ad5d..519c0b8568388):

ct  : 27,516|27,516|0|0.0%
wt  : 177,407|176,677|-730|-0.4%
cpu : 176,010|176,011|1|0.0%
mu  : 13,196,304|13,196,304|0|0.0%
pmu : 13,302,040|13,302,040|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c0a969ad5d&...

=== 8.x..1970136-views-read-more compared (519c0a969ad5d..519c0c218ed51):

ct  : 27,516|27,575|59|0.2%
wt  : 177,407|177,756|349|0.2%
cpu : 176,010|172,010|-4,000|-2.3%
mu  : 13,196,304|13,223,536|27,232|0.2%
pmu : 13,302,040|13,328,392|26,352|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c0a969ad5d&run2=519c0c218ed51&source=drupal-perf-mrbaileys&extra=8.x..1970136-views-read-more

+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch

Committed 9414794 and pushed to 8.x. Thanks!