Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrisjlee’s picture

Will probably fail; but figures i should give it a shot.

Not sure what to do here with the template preprocess function:

function template_preprocess_feed_icon(&$variables) {
  if ($title) {
    $variables['title'] = $title;
    $variables['attributes']['alt'] = $title;
  }

  if ($url) {
    $variables['url'] = url();
  }
}
chrisjlee’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 1939096-convert-feed-icon-1.patch, failed testing.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
1.44 KB
3.05 KB

Try again. No idea if this will work.

Status: Needs review » Needs work

The last submitted patch, 1939096-convert-feed-icon-4.patch, failed testing.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
2.86 KB
2.28 KB

Status: Needs review » Needs work

The last submitted patch, 1939096-convert-feed-icon-6.patch, failed testing.

joelpittet’s picture

@chrisjlee great start! May need the t() function in twig to get the translations to pass.

chrisjlee’s picture

thanks joelpittet. I'll give that a try.

chrisjlee’s picture

FileSize
476 bytes
2.87 KB

Probably need to put 'subscribe to' in the translate as well but not sure if it's needed. We'll find out.

chrisjlee’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 1939096-10.patch, failed testing.

joelpittet’s picture

Totally do need "Subscribe to" in there too:) But you got it down to two fails instead of 4! Keep up the good work:)

Also, need some escaping to pass the other one. {{ variable_name|e }}

They will be autoescaped eventually... but that is a cleanup task... either escape it with check_plain() in the preprocess or in twig with |e, not sure which is best right now because they will be removed later. Maybe check_plain() would be better for now so the twig file can stay clean..er. You're call:)

chrisjlee’s picture

Another try based on joelpettit's feed . Thanks joel!

chrisjlee’s picture

Status: Needs work » Needs review

argh someday i'll get better at remembering to change the status.

Status: Needs review » Needs work

The last submitted patch, 1939096-convert-feed-icon-14.patch, failed testing.

joelpittet’s picture

oh my freedback didn't help much there, did it:( I think the t() needs the full string with the args just like the original... but I could be wrong. I could take a stab if you want, ping me on irc or here.

chrisjlee’s picture

Status: Needs work » Needs review
FileSize
2.9 KB
592 bytes

@joelpittet don't worry about :) i'm open to any ideas to get this patch green.

I'm going to go try something different.

Status: Needs review » Needs work

The last submitted patch, 1939096-convert-feed-icon-17.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
FileSize
4.75 KB
4.26 KB

I gave those tests a good look and in my travels I found that this was already attempted a twig conversion on the twig sandbox. It wasn't quite perfect but I used that as a base, so the interdiff will look a bit drastic. Not sure it will pass all tests but hopefully it will get things going in the right direction. I also tweaked a test because it was calling theme_feed_icon() directly which since it doesn't exist anymore is one of the fails.

the twig filenames should be feed-icon instead of feed_icon for consistency but that wouldn't have caused a fail.

*Crosses fingers*

joelpittet’s picture

@chrisjlee, could you review the patch for anything you think may be wrong or silly or spelling mistakes or the like? I never get things right the first time.

chrisjlee’s picture

Looks good to me. I wouldn't know any better though. Hope someone with more experience reviews this. :)

Only issue i see would be something small and nitpicky like this:

+ *   - url: An internal system path or a fully qualified external URL of the
+ *     feed.

Feed should probably be on the same line? Although, it'd be one character over the drupal character line limit (80) as per drupal documentation comment standards.

joelpittet’s picture

@chrisjlee you must be a designer too, "say no to widows" :)

star-szr’s picture

Indeed the widow must stay. Quick review:

+++ b/core/includes/theme.incundefined
@@ -3199,7 +3209,8 @@ function drupal_common_theme() {
-      'variables' => array('url' => NULL, 'title' => NULL),
+      'variables' => array('url' => NULL, 'title' => NULL, 'attributes' => array()),

Is adding 'attributes' here necessary? Can't we just add it in the preprocess function?

+++ b/core/modules/system/templates/feed-icon.html.twigundefined
@@ -0,0 +1,19 @@
+ * - attributes: An array of attributes for the feed link, with the following children:

This goes over 80 characters, and shouldn't use the word "array", especially because in this case I think it's an Attribute object. I'm wondering if we can have a boilerplate of sorts for 'attributes' since it's such a common variable. It might be worth looking at some of the other Twig conversion issues (or in the Twig sandbox) to see if there's a good description in one of the other patches :)

joelpittet’s picture

Assigned: Unassigned » joelpittet
FileSize
1.52 KB
4.13 KB

Doc cleanup from #22 and #24

@Cottser, I am not sure if it's necessary, I removed it in this patch.

star-szr’s picture

One teensy tiny tweak, then RTBC from me :)

+++ b/core/modules/system/templates/feed-icon.html.twigundefined
@@ -0,0 +1,19 @@
+ * - attributes.class: HTML Classes to be applied to the feed link.

HTML classes (lower c).

chrisjlee’s picture

i'll help out with this small patch

star-szr’s picture

+++ b/core/modules/system/templates/feed-icon.html.twigundefined
@@ -0,0 +1,19 @@
+ * - attributes: Remaining HTML attributes for the feed link.
+ * - attributes.title: A descriptive title of the feed link.
+ * - attributes.class: HTML classes to be applied to the feed link.

Missed this before, I think we should indent attributes.title and attributes.class here.

 * - attributes: …
 *   - attributes.title: …
 *   - attributes.class: …

Also, can we please replace the theme function instead of adding the new preprocess function above drupal_common_theme()? I think that would be better (same as #1939088-17: Convert theme_meter() to Twig).

chrisjlee’s picture

@Cottser Thank you for the feedback!

Docs tweaks and attempted to put back the template preprocess function where it was!

star-szr’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, tested manually and markup matches exactly. Great work guys!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +needs profiling
joelpittet’s picture

Assigned: joelpittet » Unassigned
Issue tags: -needs profiling
joelpittet’s picture

Issue tags: +needs profiling

damn, hate when that happens, re-tagging.

jenlampton’s picture

Status: Needs work » Needs review
jenlampton’s picture

#29: twig-feed-icon-1939096-29.patch queued for re-testing.

star-szr’s picture

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

Needs some touch-ups, things have changed :)

+++ b/core/includes/theme.inc
@@ -2411,19 +2411,29 @@ function theme_more_help_link($variables) {
+  $variables['attributes'] = new Attribute(array());

We don't need this line anymore, see #1982024: Lazy-load Attribute objects later in the rendering process only if needed.

+++ b/core/modules/system/templates/feed-icon.html.twig
@@ -0,0 +1,19 @@
+ * - attributes: Remaining HTML attributes for the feed link.
+ *   - attributes.title: A descriptive title of the feed link.
+ *   - attributes.class: HTML classes to be applied to the feed link.

The indented lines should just say 'title' and 'class'. Indent means 'add a dot'/drill down.

+++ b/core/modules/system/templates/feed-icon.html.twig
@@ -0,0 +1,19 @@
+ * @see template_preprocess()

This line should be removed - see #2013094: [policy adopted, patch needed] Stop saying '@see template_preprocess()' in every twig file.

drupalmonkey’s picture

Assigned: Unassigned » drupalmonkey
drupalmonkey’s picture

Assigned: drupalmonkey » Unassigned
Status: Needs work » Needs review
FileSize
1.31 KB
3.49 KB

Took care of the items in #36.

star-szr’s picture

Issue tags: +Needs manual testing

Another quick round of manual testing would be great before we go ahead with the profiling, re-tagging.

star-szr’s picture

Assigned: Unassigned » star-szr

I will take care of manually testing and profiling this one.

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice, -Needs manual testing, -needs profiling
FileSize
65.15 KB

This looks good to me.

Markup comparison

(debug output in place to show that the markup is in fact coming from the new Twig template…)
1939096-41-markup.png

Profiling

Scenario: default homepage.

=== 8.x..8.x compared (5219624c55207..521962849973a):

ct  : 34,314|34,314|0|0.0%
wt  : 191,228|191,175|-53|-0.0%
cpu : 165,799|165,925|126|0.1%
mu  : 13,882,272|13,882,272|0|0.0%
pmu : 13,926,944|13,926,944|0|0.0%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5219624c55207&...

=== 8.x..theme_feed_icon-1939096-38 compared (5219624c55207..521962a927d49):

ct  : 34,314|34,389|75|0.2%
wt  : 191,228|191,693|465|0.2%
cpu : 165,799|166,748|949|0.6%
mu  : 13,882,272|13,910,952|28,680|0.2%
pmu : 13,926,944|13,956,376|29,432|0.2%

http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=5219624c55207&...

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Oops, sorry, thought I'd committed this awhile back.

0.2% is about double what these normally are, but RSS icons aren't used overly often, so this seems relatively ok. If it's not, alexpott, feel free to revert.

Committed and pushed to 8.x. Thanks!

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

Anonymous’s picture

Issue summary: View changes

Update remaining