Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Issue #1987390 by Cottser, vinmassaro, carsonevans, ezeedub: Aggregator.module - Convert theme_ functions to Twig.
Task
Convert theme_ functions to Twig templates.
Remaining
- Patch review
- Profiling
Theme function name | Conversion status |
---|---|
theme_aggregator_block_item | converted |
Converted to #type table (and committed) in #1938894: Convert aggregator theme tables to table #type | |
Will be converted in #1963544: Convert aggregator/rss to views | |
Will be converted in #1963544: Convert aggregator/rss to views | |
theme_aggregator_summary_item | converted |
Related Issues
#1896060: aggregator.module - Convert PHPTemplate templates to Twig
#1757550: [Meta] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
To test:
1) enable aggregator.module
2) visit the aggregator settings page and add a new feed here admin/config/services/aggregator/add/feed
3) import aggregator items here via the drop module /admin/config/services/aggregator
4) test the aggregator item output here aggregator/sources/1
Comment | File | Size | Author |
---|---|---|---|
#131 | aggregator-twig-1987390-131.patch | 587 bytes | Tim Bozeman |
#131 | interdiff.txt | 585 bytes | Tim Bozeman |
#129 | aggregator-twig-1987390-129.patch | 587 bytes | Tim Bozeman |
#129 | interdiff.txt | 585 bytes | Tim Bozeman |
#125 | aggregator-twig-1987390-125.patch | 587 bytes | Tim Bozeman |
Comments
Comment #1
star-szrInitial patch, I noticed a couple of the preprocess docs should be tweaked to match #1913208: [policy] Standardize template preprocess function documentation.
Comment #1.0
star-szrAdd conversion chart and update remaining
Comment #1.1
star-szrUpdated issue summary.
Comment #2
star-szrHere are the preprocess doc tweaks.
Comment #2.0
star-szrAdd aggregator theme_ function conversions to related
Comment #3
c4rl CreditAttribution: c4rl commentedWe've reversed the purpose here. Per #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions rather than templates. See #1896060: aggregator.module - Convert PHPTemplate templates to Twig for template conversion instead.
Comment #3.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #4
star-szrHere are just the theme_ function conversions.
Comment #4.0
star-szrUpdated issue summary.
Comment #5
epersonae2 CreditAttribution: epersonae2 commented#4: 1987390-4.patch queued for re-testing.
Comment #6
vinmassaro CreditAttribution: vinmassaro commentedGoing to reroll and attempt to profile.
Comment #7
vinmassaro CreditAttribution: vinmassaro commentedComment #8
carsonevans CreditAttribution: carsonevans commentedI am profiling this
Comment #9
vinmassaro CreditAttribution: vinmassaro commentedComment #10
carsonevans CreditAttribution: carsonevans commentedbbranches 519fe4a4d98a1 profile_aggregator
Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..8.x compared (519fe4a4d98a1..519fe4f3d483f):
ct : 22,777|22,777|0|0.0%
wt : 333,158|332,921|-237|-0.1%
cpu : 324,698|324,076|-622|-0.2%
mu : 35,277,512|35,277,512|0|0.0%
pmu : 35,389,704|35,389,704|0|0.0%
Profiler output
Warning: Must specify directory location for XHProf runs. Trying /tmp as default. You can either pass the directory location as an argument to the constructor for XHProfRuns_Default() or set xhprof.output_dir ini param.
=== 8.x..profile_aggregator compared (519fe4a4d98a1..519fe521c4e93):
ct : 22,777|22,777|0|0.0%
wt : 333,158|331,702|-1,456|-0.4%
cpu : 324,698|322,947|-1,751|-0.5%
mu : 35,277,512|35,277,888|376|0.0%
pmu : 35,389,704|35,390,504|800|0.0%
Profiler output
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
Comment #11
carsonevans CreditAttribution: carsonevans commentedComment #12
cafuego CreditAttribution: cafuego commented... am testing manually at the DC Twig sprint.
... am not testing manually, as my D8 install appears to now be broken.
Comment #14
ezeedub CreditAttribution: ezeedub commented#7: 1987390-6.patch queued for re-testing.
Comment #14.0
ezeedub CreditAttribution: ezeedub commentedUpdated issue summary.
Comment #15
ezeedub CreditAttribution: ezeedub commentedadded a feed block with 10 items on the home page.
hits aggregator-block-item.html.twig
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a184397f8eb&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a184397f8eb&...
Comment #16
ezeedub CreditAttribution: ezeedub commentedpointed home page to aggregator/rss/1
hits aggregator-page-rss.html.twig
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a188154f4e0&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a188154f4e0&...
Comment #17
ezeedub CreditAttribution: ezeedub commentedpoint home page to aggregator/sources
hits aggregator-summary-item.html.twig
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a18dd97f04c&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a18dd97f04c&...
Comment #18
ezeedub CreditAttribution: ezeedub commentedset home page to aggregator/opml
hits aggregator-page-opml.html.twig
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a191a350534&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=51a191a350534&...
Comment #19
ezeedub CreditAttribution: ezeedub commentedaggregator-page-rss.html.twig and aggregator-page-opml.html.twig need a second look
Comment #20
tlattimore CreditAttribution: tlattimore commentedHere's some cleanup that needs to be done on the patch in #7:
Neither `item` nor `feed` are used in this function. The comment should contain a description of`url` and `title`
These @todo's should be filled out.
This doesn't have a comment in the doc block. I think it should.
Comment #21
drupalninja99 CreditAttribution: drupalninja99 commented@Thomas, here is my attempt to apply those requested changes in #20 in patch #7. I have re-rolled the #7 patch and included my diff as well. I don't submit patches very often so forgive me if I did anything wrong...
Comment #22
drupalninja99 CreditAttribution: drupalninja99 commentedHere we go again with needs review status...
Comment #23
aspilicious CreditAttribution: aspilicious commentedThats a serious performance regression :s
Comment #24
drupalninja99 CreditAttribution: drupalninja99 commentedI am using the aggregator_test module to compare output against HEAD, 2 things:
1. I opened an issue here http://drupal.org/node/2004622 bc there is Drupal block content showing up on the test feed page
2. Should I create another issue to add more test pages to the aggregator_test module? We only test aggregator_test_rss091.xml but that module has 2 more test xml files and we have no OPML .xml tests that I am aware of. Would it be useful for the purposes of comparing output before and after to add more tests to that module?
Comment #25
drupalninja99 CreditAttribution: drupalninja99 commentedRSS before and after tests
RSS URL used: http://rss.cnn.com/rss/cnn_topstories.rss
Test 1 - compare output of block and page:
Steps:
1. Added aggregator block to left sidebar
2. Tested /aggregator/sources/1 & /aggregator pages
Result: PASS, no differences
Test 2 - compare RSS output:
Steps:
1. Tested /aggregator/rss
Result: FAIL, unable to render RSS
With patch "1987390-7.patch" I received this error:
Notice: Use of undefined constant LANGUAGE_TYPE_CONTENT - assumed 'LANGUAGE_TYPE_CONTENT' in template_preprocess_aggregator_page_rss() (line 481 of core/modules/aggregator/aggregator.pages.inc).
Comment #26
drupalninja99 CreditAttribution: drupalninja99 commented2 Problems at at /aggregator/rss, function aggregator_page_rss():
1. The LANGUAGE_TYPE_CONTENT is not defined and we don't need to check it anyway. I am attaching a patch where we just leave the langcode as is if no $category['langcode'] is found. Let me know if this works
2. The RSS output renders in the content area of the page instead of on it's own (with no Drupal page markup wrapper). I just need to know how to make it print without the wrapper and I could submit a patch to fix this.
Comment #27
linclark CreditAttribution: linclark commentedI'm not sure whether we need to use Twig in aggregator. See #1839468: [Followup] Replace aggregator rss parsing with Zend Feed
Comment #28
linclark CreditAttribution: linclark commentedAlso relevant, #2003108: Switch Views RSS to use Serializer/Zend
Comment #29
drupalninja99 CreditAttribution: drupalninja99 commentedAttaching langcode diff
Comment #30
drupalninja99 CreditAttribution: drupalninja99 commented@Lin I think we would need twig for the non-XML output theme functions. I doubt those would change bc Zend wouldn't replace that. As far as /aggregator/rss (not sure ab aggregator/opml) - those twig/theme functions might go away in favor of Zend (I suppose).
Comment #31
drupalninja99 CreditAttribution: drupalninja99 commentedOK I fixed some things and re-rolled back into 1 patch. Here are my fixes:
1. Work-around for the langcode issue is to make it NULL so we at least don't get an error, but there is probably a more elegant way to find the current site language. When langcode is null you get
<language></language>
in the RSS2. I added print theme('..'); exit; to both the aggregator page rss and opml page functions. This outputs the XML without the Drupal wrapper. I am not sure if this is the proper way to do this.
3. I fixed the twig files which were unnecessarily escaping double quotes with \" in some cases, like the rss page twig file.
The /aggregator/rss is now rendering much better. The only difference I have seen so far is the lack of a language code.
Comment #32
TrevorBradley CreditAttribution: TrevorBradley commentedI'll give profiling the new patch a shot. I'll start working on profiles for all four twig templates.
Comment #33
TrevorBradley CreditAttribution: TrevorBradley commentedHopefully I get this right, but it looks like this needs work. I imported a RSS feed from CNN's Top Stories. With the patch applied, I get a white screen (0 byte file) for the aggregator/rss URL, with no errors in the apache error log. Returning to the 8.x branch I see a nice page displayed.
(Don't take my word for it though! I did do a git pull and git rebase to make sure there weren't any errors, but it could be tester error!)
Comment #34
drupalninja99 CreditAttribution: drupalninja99 commentedI suppose you cleared caches? Dd you make sure to import some items?
Comment #35
drupalninja99 CreditAttribution: drupalninja99 commentedI suppose you cleared caches? Dd you make sure to import some items?
Comment #36
drupalninja99 CreditAttribution: drupalninja99 commentedI suppose you cleared caches? Dd you make sure to import some items?
Comment #37
TrevorBradley CreditAttribution: TrevorBradley commentedI did import items but did not clear caches. Unfortunately this was pointed out to me just as I was leaving Acquia today. Changing status back. Sorry everybody!
EDIT: Verified this is a caching issue. Difficult to fix in the car's back seat back to Vancouver, BC. I'll pass this on to someone else.
Comment #38
drupalninja99 CreditAttribution: drupalninja99 commentedAdded a fix for the language code. Now the RSS is identical with 1 caveat: the HEAD rss page does not have an exit(); after it prints the rss output, so it continues rentering html on the page. I have to patch that page to get a true comparison.
It is a bit debatable perhaps if we need twig files for RSS/opml output at all. There is an API function format_rss_channel() in HEAD that we could continue to use and not convert theme_aggregator_page_rss() to twig at all. If the goal is to kill every theme function no matter what than we can proceed, I just was thinking that I doubt RSS output is overridden all that often.
Comment #39
drupalninja99 CreditAttribution: drupalninja99 commentedActually the LANGUAGE_TYPE_CONTENT constant was still giving me errors, I think that has been eliminated in 8. I went with the simpler
language_default()->langcode
which works for me with no errors.Comment #40
tim.plunkettComment #42
drupalninja99 CreditAttribution: drupalninja99 commented@Tim, gotcha thanks. I wasn't familiar with the new syntax we needed.
Comment #43
drupalninja99 CreditAttribution: drupalninja99 commentedI have added Tim's language fix to my last patch, this is ready for review.
Comment #45
gnuget#43: 1987390-11.patch queued for re-testing.
Comment #47
drupalninja99 CreditAttribution: drupalninja99 commentedHmm, ya I have seen that bug before, seems like a views problem but I could investigate.
Comment #48
joelpittetHi @drupalninja99 thank you very much for giving a go at this conversion!
So a couple general suggestions:
Below are some specific items:
So the @param isn't to tell you what is coming out but what is coming in. So the original item and feed variables should stay.
Not sure why this is printing and exiting, this looks like a mistake.
There is some ending whitespace in the patch, if your editor has it turn on it's remove ending whitespace preference.
This should probably be returned as a renderable array. eg.
array('#theme' => 'aggregator_page_opml', '#feeds' => $feeds);
Cheers, keep up the good work:)
Comment #49
joelpittet#31 Has the twig templates in them, so you had them they may have just not got added to the commit.
@drupalninja99 here's my workflow if it helps: http://pittet.ca/drupal/patch the same as https://drupal.org/node/707484
Comment #50
drupalninja99 CreditAttribution: drupalninja99 commentedHi Joel, thank you for your help. Yes I did my last diff wrong, I am attaching the latest with all of the twig files and your recommended changes including cleaning up some extraneous whitespace.
The thing I haven't figured out is how to get the print to work without the exit. I agree normally you return the renderable array, not print/exit. The issue is that we are not display content within the Drupal page layout. We are outputting only XML to the screen and nothing else. In Drupal 6/7 if you start printing in a page handle it will not display the output in the Drupal layout.
If you just return the renderable array you get XML output in the page layout. That is definitely what we don't want. If you instead print the output you get halfway there, the page starts with just the XML output but then it ends with Drupal page markup which i invalid. So I don't know the proper Drupal8 way to do that. I am trying to find another module that has to do something similar so that I can see how they do it.
I am attaching my latest patch.
Comment #51
drupalninja99 CreditAttribution: drupalninja99 commentedThis is the closest comp I can see as to how to resolve the exit problem,
If the XMLRPC module in xmlrpc.server.inc it does an echo and then a drupal_exit(); so I will add the same to my patch.
Comment #52
drupalninja99 CreditAttribution: drupalninja99 commentedI have added drupal_exit and re-tested from a clean install. This is looking pretty good, I am attaching patch 13 + the interdiff.
Comment #53
drupalninja99 CreditAttribution: drupalninja99 commentedLooking at the drupal_exit() code that I borrowed from xmlrpcs, this was lifted straight from an xmlrpc Drupal7 include so I think it's proper to use it in our use case.
Comment #54
drupalninja99 CreditAttribution: drupalninja99 commentedI also ran the Drupal\aggregator\Tests\AggregatorRenderingTest simple test locally and received no errors this time.
Comment #56
star-szr#52: 1987390-13.patch queued for re-testing.
Comment #58
star-szr#52: 1987390-13.patch queued for re-testing.
Comment #59
joelpittetThanks for adding back the files and providing an interdiff!
There is whitespace errors in the files.
And I really don't think we need the exit and print, we should be able to leave it as just return statement. Or maybe I'm missing something?
Also, please use print instead of echo. I like echo too, but Drupal uses print exclusively.
Comment #60
drupalninja99 CreditAttribution: drupalninja99 commentedI turned on remove trailing whitespace in Netbeans and changed out the echo statements with print. I will explain in my next comment why we need drupal_exit();
Comment #61
drupalninja99 CreditAttribution: drupalninja99 commentedRe: "Or maybe I'm missing something?" - yes.
You can see the 3 options and their corresponding output:
1. (current) print + exit means you get just the XML, see rss-with-exit.txt
2. print + no exit gets you extra Drupal markup after the XML prints, see rss-without-exit.txt. This means your RSS is invalid.
3. No print (just return) + no exit means you get the complete Drupal page with XML in the content area, which obviously is not valid, see rss-without-print-or-exit.txt.
I am pretty sure you have to have the drupal_exit() for the same reason XML RPC uses it and has used it since Drupal 7.
Comment #62
joelpittetok @drupalninja99 attached is an interdiff between comment #7 and your comment #60.
This issue is to be as close to a straight conversion as possible. All other fixes to broken bits should have their own issue. So please confirm that the the way without the twig template is producing the expected results and that the addition print + exit is necessary for the conversion?
I'll review the other bits of this interdiff in the next comment.
Comment #63
joelpittetGreat, this is definitely needed now:)
This looks like a regression, we are trying to avoid using theme() call's and instead returning renderable arrays.
The original ternary operator was fine and clear, just need to change the constant to use the Language::TYPE_CONTENT static. If you a really think it needs a comment, don't use the docblock style inside the function. Inline comment style please.
The aim here is to have this return a renderable array.
Thank you! no more @todos:)
Great that is a good catch.
Same here, nice one!
Comment #64
joelpittetComment #65
drupalninja99 CreditAttribution: drupalninja99 commentedRe: "This looks like a regression, we are trying to avoid using theme() call's and instead returning renderable arrays."
I fully understand what you are saying. I wish Drupal had a way to specify if output should not render the Drupal layout. 8.x HEAD is currently broken because it does not use an exit. What you get is a lot of layout html at the end of the file. So if we want to use return and not print + exit then it will continue to be broken.
Please look at the RSS output I just grabbed from /aggregator/rss on 8.x HEAD. This is not right. I don't have any other way to fix this than to use the convention that already has a precedent in the XML RPC module which dates back at least to Drupal 7 as well as D8. So I have no other ideas for how to handle this use case.
Comment #66
drupalninja99 CreditAttribution: drupalninja99 commentedRe "The original ternary operator was fine and clear" I have reverted this back to the original conditional with the new language change.
I don't have any other fixes though for the print/drupal_exit() logic.
Comment #67
drupalninja99 CreditAttribution: drupalninja99 commentedComment #68
joelpittetOk thanks again @drupalninja99 so it seems this is an issue that has existed for a while and nobody has complained.
Unfortunately I don't have a good solution either for this issue. What you have done makes sense to me though I haven't seen that approach yet in my travels through twig conversion land. Also, d8 was or is supposed to have help for this in the WSSCI initiative, to make sure it doesn't pretend everything is an HTML page(heard this in a talk a while back).
The only way we have dealt with these so far is that we create a follow-up issue related to the specific problem. Leave these issues to as straight conversion as we can. Also, the fix may get in to 8.x faster if it's clear as well what the problem is and what the print/exit's are trying to solve.
If you would like I could help you create a followup issue and we can split the patch between them?
Comment #69
drupalninja99 CreditAttribution: drupalninja99 commentedHi Joel, re: "If you would like I could help you create a followup issue and we can split the patch between them?" - can you go ahead and create an issue regarding the problem? The issue is that we have no way to specify that this is a stand-alone page with no Drupal layout. In my view, I wish we had a way to specify this in the menu hook. So instead of:
We would have a new 'type' constant called "MENU_CUSTOM" or "MENU_NO_LAYOUT" or something like that.
Another option would be to have a parameter in the render array that's returned. So here you would add something additional parameter telling it not to render the full layout.
Comment #70
drupalninja99 CreditAttribution: drupalninja99 commentedI have added another patch where I remove the print/exit() code per Joel's request in lieu of a new ticket to address the output problem. Note that this effectively breaks the RSS/OPML pages as now they will render XML within the content area of the page.
Comment #71
joelpittet@drupalninja99 thanks: Here's a the new issue for you
#2010956: aggregator_page_rss and aggregator_page_opml are producing HTML after their root elements. You are more than welcome to update the issue summary with better details on the problem. And I would suggest posting the print/exit bits to kick off the dialog:) Maybe someone can help with an even better solution.
Comment #72
drupalninja99 CreditAttribution: drupalninja99 commentedSo where do we stand with this patch?
Comment #73
joelpittetPatch looks good, we just need to do some profiling, and to help out write out the scenarios from #15 to #18 to profile would be great so others can do them or confirm them.
Comment #74
star-szrWould like to see some manual testing on this, I see some results from @drupalninja99 in #25 and #26 but many changes since then, I'd like someone else to please run through this :)
As @joelpittet said - writing up manual testing steps would be extremely helpful for manual testing as well as profiling. Tagging as needs STR as well.
Comment #74.0
star-szradd attribution
Comment #75
joelpittetUpdated the summary to remove:
Will be converted in #1963544: Convert aggregator/rss to views
Removed the theme call around datetime
Removed the
aggregator-page-rss.html.twig
andaggregator-page-opml.html.twig
templates and reverted their theme functions.Removed the left over
theme_aggregator_summary_item
Did a markup comparison on the block and it's rendering perfect. Not sure where to find the other templates at the moment.
Comment #76
azinoman CreditAttribution: azinoman commentedfailed testing needs to be rerolled
Comment #77
jenlamptontest bot passed it, let's give it another go.
Comment #78
azinoman CreditAttribution: azinoman commented#75: 1987390-75-twig-aggregator-theme.patch queued for re-testing.
Comment #79
azinoman CreditAttribution: azinoman commentedno Jen I'm pretty sure this one is gonna need a reroll
Comment #80
azinoman CreditAttribution: azinoman commentedComment #82
azinoman CreditAttribution: azinoman commentedI rerolled this.
Comment #84
star-szrThanks @azinoman! The rerolled patch is missing the changes to template_preprocess_aggregator_summary_item().
Before reroll:
4 files changed, 50 insertions, 28 deletions.
After reroll:
4 files changed, 42 insertions, 21 deletions.
(Stats coming from Dreditor)
Comment #85
gnugetComment #86
gnugetRerolled version attached
Comment #86.0
gnugetpage_rss and page_rss being taken care of else where.
Comment #86.1
siccababes CreditAttribution: siccababes commentedadded how to test
Comment #86.2
siccababes CreditAttribution: siccababes commentedadded link
Comment #87
siccababes CreditAttribution: siccababes commentedI followed the instructions to test this patch. Everything ran smoothly. This patch seems fine.
Comment #88
star-szrThanks for testing @siccababes, this still needs manual testing and profiling before we can RTBC it though.
Updating tags because we have a fresh patch and it looks like we have testing steps in the issue summary.
Comment #89
drupalninja99 CreditAttribution: drupalninja99 commentedRSS before and after tests
RSS URL used: http://rss.cnn.com/rss/cnn_topstories.rss
Test 1 - compare output of block and page:
Steps:
1. Added aggregator block to left sidebar
2. Tested /aggregator/sources/1 & /aggregator pages
3. Added permission for anonymous users to view news feeds
Result: PASS, no differences
Test 2 - compare RSS output:
Steps:
1. Tested /aggregator/rss
Result: PASS, no differences
I used daisy diff to compare output.
One note is that the RSS page is not currently rendering correctly in 8.x. It renders the XML first, then the entire HTML page (starting with DOCTYPE). This is a separate issue.
Comment #90
thedavidmeister CreditAttribution: thedavidmeister commentedmanual testing is a novice task
Comment #91
drupalninja99 CreditAttribution: drupalninja99 commentedI did some manual testing in #89, is there something else we need to do on this to satisfy manual testing?
Comment #92
thedavidmeister CreditAttribution: thedavidmeister commentedno, that's fine. Apologies, I didn't see that and the tag was still on the issue.
Comment #93
thedavidmeister CreditAttribution: thedavidmeister commentedIt would be good if you could open an issue for this and link to it from here.
Comment #94
drupalninja99 CreditAttribution: drupalninja99 commentedI think I filed an issue awhile back, I will take a look again. Also I am going to retest just to confirm that it's still broken in head, it's possible it's been fixed.
Comment #95
drupalninja99 CreditAttribution: drupalninja99 commented@thedavidmeister, okay great news RSS is working now, I think it got fixed in head maybe July 10th (or perhaps earlier?)
https://drupal.org/node/1963544
I updated my local copy and re-tested the RSS. Looks good.
Comment #96
star-szrTagging for reroll.
Comment #97
RainbowArrayWorking on the reroll.
Comment #98
RainbowArrayHere's the reroll. No conflicts needed to be resolved on rebase. Yo go, git!
Comment #99
RainbowArrayRemoving the "needs reroll" tag.
Comment #100
RainbowArrayAlso, unassigning myself.
Some day I'll remember to do all these things at once.
Comment #101
joelpittet@mdrummond Thank you very much for the re-roll.
There is a couple clean-up items but this should be ready to go for profiling.
I am pretty sure this isn't needed in the conversion as well.
These can be removed.
Comment #102
jenlamptontag
Comment #103
RainbowArrayMade the updates requested.
Comment #105
joelpittet@mdrummond You may have forgot to rebase? 1.55MB is a bit big for a patch.
Comment #106
RainbowArray0_o
Comment #107
RainbowArrayHere is the reroll of the patch in #86 plus the changes requested in #101. This patch is only 5K, so hopefully should be correct.
Comment #108
RainbowArrayComment #109
joelpittet@mdrummond thanks for the re-roll! That looks like it should work. Though you may have forgot to post the changes from the interdiff you had in #103 that fixed up #100
Going to test them locally though to see if they pass.
Comment #110
joelpittetOh @mdrummond you may know this already but you can apply interdiffs against your git repo too.
curl https://drupal.org/files/interdiff_9270.txt | git apply
Edit: This is applied against your latest working directory should apply cleanly.
Comment #111
joelpittetThe removal of the
use Drupal\Core\Language\Language;
didn't fail my tests locally so that's cool. Give it a go and see what testbot says.Comment #112
RainbowArrayOkay, I've gone through this again, and it should be correct. Yes, the changes in the interdiff aren't showing up in the patch, but the end result is correct. The changes requested in #101 took effect when I applied the patch against HEAD. So... I think we're good to go. We'll see if testbot agrees.
Comment #113
joelpittetSweet on to profiling!
Comment #114
joelpittetprofiling
Comment #115
joelpittetNot totally convinced by my profiling results because of the -1 ct, so probably need another round of testing:
Scenario:
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523c58bd29e4d&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523c58bd29e4d&...
Comment #116
jibranshould be String::checkPlain(Url::stripDangerousProtocols();
Should be String::checkPlain().
Comment #117
thedavidmeister CreditAttribution: thedavidmeister commented@jibran, I'm putting this back to CNR because check_plain() and check_url() are not deprecated functions. @webchick raised some concerns about the DX of using namespaces at the .module level over at #2093143: [meta] Remove calls to @deprecated and "backwards compatibility" procedural functions from core, so we should pick up discussion there :)
Comment #118
joelpittet@jibran thanks, I think for this conversion issue we will leave that out, I'm sure there is an issue to convert those somewhere and if that get's in before this one, we'll re-roll.
If we regressed that change I'd change it right away.
Comment #119
joelpittetOk re-done after some fixes to my laptop's apache/disabled xdebug:
Same scenario as #115
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f527beec76&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=523f527beec76&...
Comment #120
joelpittetProfiled in #119
Manually tested in #89
Comment #121
alexpottCommitted 4a189ea and pushed to 8.x. Thanks!
Comment #122
star-szrI'm not really a fan of re-opening issues but this is a one word change:
The docblock in aggregator-summary-item.html.twig refers to a 'link' variable, but it should be 'url'.
Comment #123
star-szrAnd I would be remiss to miss the tag.
Comment #124
Tim Bozeman CreditAttribution: Tim Bozeman commentedYoink!
Comment #125
Tim Bozeman CreditAttribution: Tim Bozeman commentedI changed line 7 of the docblock in aggregator-summary-item.html.twig from 'link' to 'url'.
Best mentor ever!
Cottser++
Comment #126
joelpittetBack to RTBC for the docblock fix. thanks @Tim Bozeman and @Cottser
Comment #127
star-szrSorry, but the variable should be 'url' (lowercase). Thanks for picking this up @Tim Bozeman :)
Comment #128
Tim Bozeman CreditAttribution: Tim Bozeman commentedWhoops!
Comment #129
Tim Bozeman CreditAttribution: Tim Bozeman commentedDevils in the details! I made the 'url' variable lowercase.
Comment #130
joelpittetNice catch but you're URL for the description was right, just not the variable name. You can keep URL, no need to change it to Url. Sorry to be so picky, and not being picky enough before.
Comment #131
Tim Bozeman CreditAttribution: Tim Bozeman commentedThank you @Cottser & @Joelpittet!
Third times the charm. This patch changes the docblock variable to 'url' and the description to 'URL'.
Comment #132
joelpittetThanks @Tim Bozeman now it's back to RTBC:)
Comment #133
webchickCommitted and pushed to 8.x. Thanks! :)
PS: "Yoink!" made me laugh. :) Thanks for that.
Comment #134
Tim Bozeman CreditAttribution: Tim Bozeman commented:D
Comment #135.0
(not verified) CreditAttribution: commentedadded links