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.
git commit -m "Issue #1898034 by Cottser, jenlampton, joelpittet, Shawn DeArmond, idflood, Hydra, artofeclipse, rich.yumul, chrisjlee, gnuget, c4rl, thund3rbox: block.module - Convert PHPTemplate templates to Twig."
Task
Convert PHPTemplate templates to Twig templates.
Remaining
- Patch needs review
- Manual testing (steps below)
Template path | Conversion status |
---|---|
core/modules/block/templates/block-admin-display-form.tpl.php | converted |
core/modules/block/templates/block.tpl.php | converted |
Testing steps
Verify that various blocks are being output through block.html.twig.
Related
#1757550: [Meta] Convert core theme functions to Twig templates
#1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
#1938898: Convert block theme tables to table #type
Comment | File | Size | Author |
---|---|---|---|
#135 | interdiff.txt | 870 bytes | Hydra |
#135 | 1898034-135-twig-block.patch | 24.8 KB | Hydra |
#132 | 1898034-132-twig-block.patch | 25.48 KB | joelpittet |
#132 | interdiff.txt | 2.41 KB | joelpittet |
#123 | interdiff.txt | 705 bytes | Hydra |
Comments
Comment #1
c4rl CreditAttribution: c4rl commentedTagging
Comment #2
rcaracaus CreditAttribution: rcaracaus commentedComment #3
chrisjlee CreditAttribution: chrisjlee commentedgoing to attempt this process. my first twig patch.
Comment #4
chrisjlee CreditAttribution: chrisjlee commentedattached is the patch. hope i did this correctly.
Comment #6
chrisjlee CreditAttribution: chrisjlee commented#4: 1898034-block-twig.patch queued for re-testing.
Comment #8
mlncn CreditAttribution: mlncn commentedTaking a look at why the bot is hating on chrisjlee's patch.
Comment #9
mlncn CreditAttribution: mlncn commentedSorry didn't get far on this (rookie mistake: trying to run the entire test suite through the web ui. 9 hours?)
Perhaps it needs in custom_block.module
Comment #10
star-szrThis shouldn't touch custom_block.module since that will be handled in #1898038: custom_block.module - Convert theme_ functions to Twig.
Working on a revised patch.
Comment #11
star-szrI have more plans here (mostly we need to change the preprocess functions), but for now I'd like to see what the failures are if we keep custom_block_add_list in custom_block_theme() and remove the .tpl.php files.
Comment #13
star-szrAnother work in progress patch.
Docs are lagging behind and I'm adding @todos in the patch that need to be fixed before this is committable. Mostly posting to see what the tests look like though, there should be less failures now.
block-admin-display-form handling adapted from #1818678-11: Convert core/modules/block/block-admin-display-form.tpl.php to twig.
I compared the markup before and after, at a glance the even/odd classes don't quite match up but other than that looks pretty decent.
Comment #14
star-szrComment #15
jenlamptonAlso keep an eye on #1649780: Remove first/last/odd/even classes in favor of CSS3 pseudo selectors we may not need even/odd/zebra stuff for much longer.
Comment #16
star-szrAdding patch from @steveoliver in #1868212: Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table) to fix the exceptions.
To fix the other test failures, I've done the following:
Comment #17
star-szrAnd back to CNW, @todos to fix!
Comment #17.0
star-szrAdd draft commit message
Comment #18
star-szrOops. Fixed up a @todo as well.
Comment #19
star-szrComment #20
star-szrSummary of changes:
Would love to know if the following line is correct or not. #block_config doesn't seem to reliably contain 'label', at least not in the block template suggestions test.
Comment #21
star-szrCan we remove this? The issue is #1777532: Extend Twig_Loader_Filesystem instance to get .twig templates paths with hierarchy for theme dependencies in the sandbox which is still open but I don't see how it relates to the block template specifically.
Comment #22
star-szrOne more tweak, move the default "block" class to block.html.twig.
Comment #23
star-szr#22 will fail. Trying to figure out some things regarding the Attribute() class and whitespace operators in #drupal-twig.
Comment #25
star-szrNeither of these lines work as expected. You end up with
<div id="block-test-block" class="blockblock-block-test">
and<div class="contentfoobar-class">
. So something is wrong, Attribute should be inserting a leading space before each class - see Attribute::__toString(). Is Twig or our Twig implementation stripping this out? Or am I doing something else wrong?Comment #26
jenlamptonYou are accidentally stripping out the space with the Twig whitespace controller:
{{-
you need to remove the minus sign in both cases.Comment #27
star-szrWhen I remove the minus signs, I get
<div class="content ">
whencontent_attributes.class
is empty. Kinda ugly, so I was attempting to strip that out.Comment #28
star-szrI'll roll a new patch to remove the whitespace controllers (edit: and update the tests that verify the markup output), and remove the patch from #1868212: Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table), since that is now a core issue with a patch and is affecting #1898416: filter.module - Convert theme_ functions to Twig as well so should be its own issue.
We'll get a lot of exceptions in the tests, so unfortunately #1868212: Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table) will need to get resolved first before tests will pass here.
Comment #28.0
star-szrUpdating commit message
Comment #29
star-szrRevised patch to make the changes described in #28. Expecting the exceptions from #13 to come back.
Comment #31
star-szrThis is blocked on #1868212: Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table), reviews/feedback still welcome though! Reviewing it again, I noticed a couple docs tweaks, I'll roll a patch over the next few days unless someone else wants to.
This needs to change to "display a block".
This needs to be updated as well.
Comment #32
chrisjlee CreditAttribution: chrisjlee commentedJust docs changes as asked in #31
Comment #33
star-szrAwesome, thanks @chrisjlee!
Comment #34
star-szrFirst patch - reroll for #1875260: Make the block title required and allow it to be hidden.
Second patch and interdiff - docs tweaks and the following changes:
to the much more sensible:
I'm not sure about the block variable actually containing the "The full block entity" or not…
Comment #36
joelpittetThis should probably be for those Attribute exceptions...
+ $variables['attributes'] = new Attribute(array());
Just a guess but I saw some something about that someplace.
Here's a reroll with that to see. (Tried testing local but clean tests failed on block)
Comment #37
joelpittetEngage!
Comment #39
star-szrThanks @joelpittet. I just overhauled #1868212: Notice: Indirect modification of overloaded element of Drupal\Core\Template\Attribute has no effect (theme_table). As it turns out the exceptions here should actually get fixed by #1938898: Convert block theme tables to table #type.
Revised patch based on #34, interdiff is from #34. This should fix the 2 fails by relaxing the XPath on the language switcher test a bit.
Also restores the following change reverted in #34, Drupal\block\Tests\BlockTemplateSuggestionsUnitTest still doesn't like
$variables['block']->label
.Comment #39.0
star-szrblocker
Comment #39.1
star-szrUpdating blocking issues
Comment #40
star-szrActually, instead of waiting for either of those issues, this minor tweak should get us green again (adds an empty class array to avoid the Attribute exceptions).
I expect #39 to have one fail along with the exceptions (label again). I'm not sure why this is needed, but this change should address the failure by always ensuring there is a value for label. Would love to know if there's a better way or if the problem is somehow in Drupal\block\Tests\BlockTemplateSuggestionsUnitTest. We may be able to avoid this whole mess by just using block.label in the block template but that seems less friendly to themers.
Comment #41
star-szrThis just replaces all instances of:
with:
Comment #41.0
star-szrRemoving blocking issues entirely, we can get a green patch with a minor tweak.
Comment #41.1
star-szrAdd conversion summary table
Comment #41.2
star-szrLinkify #type table issue and add to related
Comment #42
star-szrTagging for manual testing, basic steps for testing are up in the summary.
Comment #43
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedI'm going to review this.
Comment #44
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedProblem: #1033116: Rename template.php to THEMENAME.theme to eliminate ambiguity around "the template file" vs. "template files"
Basically, template.php doesn't exist anymore.
This patch needs to be re-rolled.
Comment #45
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedOkay, here's the re-rolled patch. While the testbot is doing its thing, I'll test it manually too.
Comment #46
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedFound a problem: It doesn't look like HTML special chars are being correctly output.
Original Drupal 8:
With this patch:
Comment #47
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedOkay, I found a check_plain() in template_preprocess_block(). Re-rolled patch attached.
I also compared the HTML of the block output between this patch and current D8 of the following blocks:
Who's online (where I discovered the issue. now fixed.)
Who's new
Book navigation
User Login
Powered by Drupal
Comment #48
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedSomeone else should look at this, I suppose.
Comment #50
star-szr#47: 1898034-47.patch queued for re-testing.
Comment #51
star-szrThanks @Shawn DeArmond!
Rerolling and updating to account for #1968322: Remove unused $id and $zebra variables from templates.
Comment #52
star-szrThis also removes docs about all the other standard variables (is_front, logged_in, is_admin) from the template, I don't think we need to document the standard variables in every template :)
Comment #53
jenlamptonreviewing
Comment #54
jenlamptonin block.html.twig we can remove the @todo about block ID if we don't print the block ID specially in the template file. If the ID will go away in the long run let's not treat it specially now. Nothing breaks now if we make this change now, and the change that removes the ID later won't need to touch this template then either.
The docblock for the block preprocess function had some incorrect information in it, and was overly verbose as compared to all the other preprocess function docblocks. Templates are now located in a 'templates' directory within the modules file, and there is no theme_block function.
Markup looks great, only a few whitespace changes.
Manual testing results:
before:
after:
Also created a few follow-up issues:
#1972120: Remove CSS ID on blocks
#1972122: Remove the DIV tag around block content
Comment #55.0
(not verified) CreditAttribution: commentedUpdating remaining, adding testing steps
Comment #55.1
jenlamptonrm sand
Comment #56
star-szr#54: twig-block-1898034-54.patch queued for re-testing.
Comment #57
jenlamptonpulled and created new patch. trying again.
Comment #58.0
(not verified) CreditAttribution: commentedcommit
Comment #59
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedSomehow a diffed line from views.module got mixed in here.
Try this one.
Comment #61
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedDoh. There's also some stuff from theme.inc that doesn't apply.
@jenlampton, I think you had some other stuff in there when you rolled your patch.
Maybe try applying #52 again and making the whitespace changes to that.
Comment #62
jenlamptonI've made a mess. will clean up, sorry guys.
Comment #63
jenlamptonOkay, this is a re-roll of #52 accounting for changes in HEAD.
Should pass tests.
Comment #64
jenlamptonAnd here's a new patch with the same interdiff as before.
(hopefully should also pass tests)
Comment #66
jenlamptonnow that's a test I can work with :)
Comment #67
jenlamptonOkay, this test checked to see if
.content
was being added appropriately in preprocess. but we aren't adding it in preprocess anymore, it gets added directly in the template. So we can safely remove this whole test now.Comment #69
star-szrYeah, I originally removed that unneeded 'test-class' assertion in #16. Looks like this is getting closer though :) rock on!
I think to fix the tests we have to adjust for the 'id' attribute being printed towards the end of the wrapper div instead of at the beginning.
We didn't have this before and tests passed, is it needed?
Comment #70
jenlampton@Cottser, Ah no, we don't need that. That was left over from other monkeying around.
Manually printing the block ID at the beging of the template made the tests pass. I think we need to change the tests to account for printing them anywhere, will take another stab at it.
Comment #71
jenlamptonTests passing locally for me now. Fingers crossed!
Comment #72
joelpittetre-tagging. @Shawn DeArmond Giver heck
Comment #73
c4rl CreditAttribution: c4rl commentedGiven that conversion of the block administrative table would be the duty of #1938898: Convert block theme tables to table #type, we should not convert this per similar feedback given on #1898454-17: system.module - Convert PHPTemplate templates to Twig. Correct?
Comment #74
c4rl CreditAttribution: c4rl commentedPer my comments in #73, here's #71 without the administrative form converted.
Comment #75
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedmanually testing...
Comment #76
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedTested the following blocks:
Also:
Note: If this issue no longer includes the admin block list, it should not be a "Testing Step" to test "output through block-admin-display-form.html.twig" You might want to change that on OP.
Can this be RTBC?
Comment #77
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedNot It!
Comment #77.0
Shawn DeArmond CreditAttribution: Shawn DeArmond commentedTweak commit message
Comment #78
c4rl CreditAttribution: c4rl commentedPer #76, I've revised the issue summary to not include the administrative form.
Comment #79
jenlamptonGave this baby another quick re-test, and it's lookin' good!
Nice job guys :)
Comment #79.0
jenlamptonRemoved admin from summary.
Comment #80
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to .tpl.php templates rather than theme_ functions (though there are no theme_ functions in this module).
Comment #80.0
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #80.1
c4rl CreditAttribution: c4rl commentedUpdated issue summary.
Comment #81
patrickfgoddard CreditAttribution: patrickfgoddard commentedRe-rolling.
Comment #82
patrickfgoddard CreditAttribution: patrickfgoddard commentedRe-rolled patch applied. I ran the block tests locally, and passed.
Interdiff not included due to not understanding to create on a reroll.
Comment #83
star-szrThanks @thund3rbox, reroll looks great!
Now that #1982024: Lazy-load Attribute objects later in the rendering process only if needed is in let's revert all these changes please and remove the 'block' and 'content' classes being added to block.html.twig:
Can be just
<div{{ attributes }}>
Can be just
<div{{ content_attributes }}>
Comment #84
idflood CreditAttribution: idflood commentedHere is a reroll with the changes defined in #83 based on the patch in #82. I've quickly tested the output of the blocks and it looks fine.
I will try to compare against a clean drupal 8 to see if there is a difference.With this patch there is just one difference. The "content" div doesn't have the "content" class.
Comment #86
idflood CreditAttribution: idflood commentedThis adds back the "content" class and should make the tests pass.
Comment #87
idflood CreditAttribution: idflood commentedComment #89
star-szrI suspect this is something as simple as the order of the classes within the rendered output. We just need to run that failing test locally and change the expected output in the test to match what is being output now.
Edit: any takers? :) For instructions on running tests locally, see http://drupal.org/node/519364 and http://drupal.org/node/30036.
Comment #90
idflood CreditAttribution: idflood commentedThe following patch should fix the tests. Only two little modifications.
Comment #91
steveoliver CreditAttribution: steveoliver commented#90: drupal_twig_block-1898034-90.patch queued for re-testing.
Comment #92
thedavidmeister CreditAttribution: thedavidmeister commented+ {{ title_prefix }}
{{ title_attributes }}
+ {{ title_suffix }}
+ <div{{ content_attributes }}>
Undocumented variables that are used in the Twig template.
+ * - contents: The contents of this block.
Variable name is "content" not "contents".
Generally we're documenting properties of variables in Twig as:
Comment #93
thedavidmeister CreditAttribution: thedavidmeister commentedDo we really need manual testing for this HTML with the epic
$expected[]
going on inside the tests?Comment #94
star-szrThis will need a reroll now that #1927608: Remove the tight coupling between Block Plugins and Block Entities is in.
Comment #95
star-szrTagging for profiling.
Comment #96
joelpittetOk, I did a manual re-roll because my mergetool is a pain. Though this should fix also the concerns in #92
And also a small 1 space in the line:
which had 3 spaces instead of 4 before
{{ content }}
I added also the new docs for 'plugin_id' and 'configuration' which were new to the template file. And still left out the helper variables that were dropped above.
Comment #97
star-szrThis was RTBC in #79, assigning to @steveoliver for review but feel free to jump in @jenlampton :)
Comment #98
thedavidmeister CreditAttribution: thedavidmeister commented+ <h2{{ title_attributes }}>{{ label }}</h2>
+ <div{{ content_attributes }}>
title_attributes and content_attributes still undocumented. Was that intentional?
Comment #99
steveoliver CreditAttribution: steveoliver commented@joelpittet: wanna add docs for title_attributes and content_attributes? I'd then consider this RTBC after passing perf tests.
Comment #100
joelpittetTry that on for size:)
Comment #101
star-szrGiven that we need to convert all .tpl.php files in one patch, I don't think we can wait for #1938898: Convert block theme tables to table #type. I think we need to convert it here, as we had in #71. Can we pull that conversion back in and test it and see if anything needs doing?
Comment #102
star-szrJust a thought but we might be able to bring that conversion back in by reverse-applying the interdiff in #74.
Comment #103
gnugetDone.
(I did the reverse of #74 as Cottser suggest)
Comment #105
star-szr#103: 1898034-103-twig-block.patch queued for re-testing.
Comment #106
star-szrThanks @gnuget!
This should get things pretty solid as far as the markup output - I compared the markup on admin/structure/block using DaisyDiff and visual diff so it should be identical from a DOM perspective - some attributes changed order in the markup but that's about it.
We could do more of a straight template conversion here instead of handling things in preprocess as well, e.g.
<table>
markup in the template. Thoughts?Comment #107
steinmb CreditAttribution: steinmb commentedTrying to get the hang of profiling. a big thanx to Fabianx for help and xhprof-kit. So, this is #106 running on a clean installed D8 with 5 blocks enabled on the front page.
Comment #108
star-szrThose profiling results seem a bit odd @steinmb. It's comparing this issue as the baseline to 8.x where we generally want to use 8.x as the baseline. But even then, I don't see how 8.x would be adding 7,282 function calls, if anything the Twig version would be more likely to have more function calls. Not that many more though :)
And regarding the block-admin-display-form conversion, I asked this question in IRC today:
And we landed on yes, let's do table markup in the template. More of a straight conversion and the template would be much more useful than {{ table }}{{ children }}.
This would be a good task for a new or existing Twig contributor to take on: do a .tpl.php to .html.twig conversion of block-admin-display-form.tpl.php and likely remove most or all of the preprocess changes in template_preprocess_block_admin_display_form() from the patch in #106. The goal for conversion is to get the markup to match with the .tpl.php version (attributes/classes in a different order or whitespace differences are fine). If this interests you, please assign the issue to yourself. Any questions, ask in #drupal-twig on IRC or post here. Thanks!
Comment #109
gnugetComment #110
gnugetThis patch add the changes of #108
And seems to the markup is identical.
Comment #111
star-szrLooks fairly reasonable to me at a glance, thanks @gnuget!
Comment #112
intergalactic overlords CreditAttribution: intergalactic overlords commentedI have checked html output. Both block admin page and block display html output is ok.
Comment #113
thedavidmeister CreditAttribution: thedavidmeister commentedComment #114
TrevorBradley CreditAttribution: TrevorBradley commentedThis is my first time at profiling, so go easy on me!
Clean D8.x install, switched to Stark theme, created 5 test blocks (called "Test Block 1" to "Test Block 5" with simple content) added them to the Content area, created a Simple Page node and made it the default page and verified that it rendered properly.
For this profiling I made sure not to touch my laptop while the bbenches were running, as we were getting weird numbers with other apps running.
EDIT: this is against the 1898034-110 patch
Comment #114.0
TrevorBradley CreditAttribution: TrevorBradley commentedFormatting
Comment #115
thedavidmeister CreditAttribution: thedavidmeister commented-3.5% for Twig? That's awesome. Can we get confirmation from somebody else though?
The numbers are probably fine but we usually see a slowdown of about 0.3-0.5% with Twig rather than a speedup so I'd personally like to see if that result is repeatable :)
Comment #116
star-szrNow that #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead is in, we can remove all these lines (and the building of form_submit) from the preprocess function and just print {{ form }} in the .html.twig. See #1898454: system.module - Convert PHPTemplate templates to Twig for a very similar example.
Template and template docs will need to be updated as well.
No arrow operators '->' in Twig templates, just dots.
Also we need a blank line before the two @see lines :)
Comment #117
Hydra CreditAttribution: Hydra commentedI'll take this one
Comment #118
Hydra CreditAttribution: Hydra commentedOkay, here it is, removed render_children and did some template docs.
Comment #119
Hydra CreditAttribution: Hydra commentedOkay, here it is, removed render_children and did some template docs.
Comment #120
jenlamptonremoving novice tag since it needs profiling.
Comment #121
cosmicdreams CreditAttribution: cosmicdreams commentedCurrently reviewing this issue, studying #1920886: drupal_render() should check if it's rendering a 'render element' and if so call drupal_render_children() (inline) instead as result.
Comment #122
cosmicdreams CreditAttribution: cosmicdreams commentedAs a result of not using drupal_render_children anymore, we should get rid of this variable.
We are going to be using drupal_render to render a renderable instead. So we need the renderable, not the renderable's children.
Why the extra whitespace? I don't get why that's important.
Why does one line have whitespace and the other does not?
Comment #123
Hydra CreditAttribution: Hydra commentedYay thx for the nice review!
Okay, first of all, the variable is obsolete, thx for pointing it!
About the extra white space, as far es I know, the output of the twig html is slightly different then the output of the tpl.php files related to whitespaces. We don't really care about whitespace in the output markup, so we just adjust the test to work with the new output. Test went out green so we should not care about this ;)
Comment #124
TrevorBradley CreditAttribution: TrevorBradley commentedJust a note that I'm here in the DrupalCon code sprint room willing to reprofile this patch as soon as it passes through the approval queue.
Also, Scott at Acquia (sorry, don't recall drupal name) suggested I also need to test the twig template for the admin page. He stated it was critical that node.html.twig was also loaded and suggested the following setup:
1) Make /admin/structure/block the default home page.
2) Create a single node with content.
3) Create a view that displays full node bodies, and insert this view as a block to be displayed on all pages.
Resulting in an /admin/structure/block home page that renders node/1 as a block on the page.
Still planning on going forward and testing this with the new patch, but let me know if Scott's idea needs review. :)
Comment #125
TrevorBradley CreditAttribution: TrevorBradley commentedAlright, same test as yesterday - one node, 5 test blocks, all put on the same home page. Not doing Admin yet.
Comment #126
cosmicdreams CreditAttribution: cosmicdreams commentedCode Reviewed and it looks great.
Comment #128
Hydra CreditAttribution: Hydra commented#123: 1898034-123.patch queued for re-testing.
Comment #129
TrevorBradley CreditAttribution: TrevorBradley commentedGot a problem.
Attempted to test the block admin twig file and hit a snag.
I set up the following from scratch:
Set up a blank system
Switched theme to stark
Created a single node
Created a view to view all nodes and display as a block.
Added this block to the content area of every page.
Made admin/structure/block the default home page
Set up permission so that anonymous can administer blocks.
bbranch works find, the first half of bbranches works fine 8.x vs 8.x works fine, but it would start to struggle comparing 8.x to my git branch. I was getting the following errors in the apache error log:
Then I noticed that if I attepmted to view the website, I'd get a white screen of death, associated with the same error.
I can clear the error in the patch branch, but only if I delete the sites/default/files/php directory. If I do that, the page renders correctly without error. If I then switch back to the 8.x branch, I get the *same* error, that doesn't go away unless I also clear the sites/default/files/php directory.
Unless bbranches will delete the php directory as it switches between branches, I'm not sure I can do the performance testing here. (I'll check with Scott though)
EDIT: Looks like this was a rebasing issue..... test results coming up...
Comment #130
TrevorBradley CreditAttribution: TrevorBradley commentedBlock Admin Test results...
Comment #131
Hydra CreditAttribution: Hydra commentedThis doesn't look right, somebody else should defenetly check this out.
Comment #132
joelpittetI got similar results at about 7% wall time on the block admin page.
This is with my little space patch and moving row_class to attributes. And this profile is on this uploaded patch. So similar results even when I moved things around a bit.
@FabianX can you have a look?
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519b182c941bf&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519b182c941bf&...
Comment #133
star-szrGreat work on this folks! Fixing/updating tags.
P.S. I think I am "Scott from Acquia" referenced above but I'm not from Acquia, I just sprinted at their offices :)
Comment #134
star-szrAdding back profiling tag so we can profile the block output again for 5 blocks.
Please remove these hunks from the patch, they are causing an unnecessary conflict when rolling the megapatch since this whole docblock is updated in #1898050: book.module - Convert PHPTemplate templates to Twig.
Thanks :)
Comment #135
Hydra CreditAttribution: Hydra commentedOkay let's get rid of it.
Comment #136
geoffreyr CreditAttribution: geoffreyr commentedProfiling.
Comment #137
geoffreyr CreditAttribution: geoffreyr commentedRan this against the front page with a bunch of default Drupal blocks added. This doesn't cover the admin block UI. (Right now, I don't know how to hook up Fabianx's xhprof scripts up to auth and then use the admin pages.)
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c60f26cf68&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519c60f26cf68&...
Comment #138
star-szr0.6% looks good to me :)
I worked a bit too much on this one to RTBC myself - can someone please re-review?
Comment #139
thedavidmeister CreditAttribution: thedavidmeister commented0.9%, yeah?
Comment #140
star-szr@thedavidmeister - Nope, baseline wt was off by 0.3% so if we adjust for that, only 0.6% :)
Comment #141
Hydra CreditAttribution: Hydra commented@geoffreyr block ui should be available if you set permissions for anonymous user for "Administer blocks", and seth admin/structure/blocks as your frontpage
Comment #142
geoffreyr CreditAttribution: geoffreyr commentedRighto, will give this another run.
Comment #143
geoffreyr CreditAttribution: geoffreyr commentedScenario: admin/structure/block on the homepage, various sidebar blocks. Significant difference here.
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d44ffb3845&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=519d44ffb3845&...
Comment #144
thedavidmeister CreditAttribution: thedavidmeister commentedComment #145
Fabianx CreditAttribution: Fabianx commentedBack to RTBC. (I also re-reviewed the whole patch.)
The admin-overview profiling, while important is not a blocker for this patch.
@alexpott said to me in Person that he does not care about 40ms for an admin overview page, which is still very much work-in-progress and changing anyway.
Comment #146
alexpott+1. Ready for #1987510: [meta] Convert all core *.tpl.php templates to Twig as singular patch
Comment #147
alexpottA folllow-up will be created to work on the block ui
Comment #147.0
alexpottUpdated issue summary.
Comment #148
alexpottCommitted bd5769d and pushed to 8.x. Thanks!
Comment #149.0
(not verified) CreditAttribution: commentedUpdated issue summary (commit message).