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.
Task
Use Twig template instead of theme functions for defining themable HTML markup for the ckeditor module.
Steps to test manually
Navigate to admin/config/content/formats/manage/full_html
Remaining
Theme function name/template path | converted |
---|---|
theme_ckeditor_settings_toolbar | converted |
Comment | File | Size | Author |
---|---|---|---|
#72 | interdiff.txt | 698 bytes | star-szr |
#72 | 1963474-72.patch | 14.99 KB | star-szr |
Comments
Comment #1
star-szrTagging.
Comment #2
widukind CreditAttribution: widukind commentedComment #3
widukind CreditAttribution: widukind commentedComment #4
star-szrWow! Fantastic work @widukind.
I tested this manually and I only found one glitch so far - the markup is not output when twig_debug is enabled. I think we may need to solve that outside of this issue though, looking into it now.
Below are just documentation and code standards tweaks and general commentary :)
Per #1913208: [policy] Standardize template preprocess function documentation and since we are removing the theme_ckeditor_settings_toolbar() function, this should be something more like:
"Prepares variables for CKEditor settings toolbar templates."
Extra space before equals sign can be removed.
Render array conversion looks good, I think you just forgot to remove this commented out line, I do this too when converting to render arrays :)
Great catch! :)
The description here should be less abstract than the theme_test.module ones, what about something like this?
"Default theme implementation for the CKEditor settings toolbar."
@ingroup themeable per Twig coding standards.
Let's indent the contents of the {% spaceless %} tag per http://drupal.org/node/1823416#whitespace.
Comment #5
widukind CreditAttribution: widukind commented@Cottser, thanks for the review and feedback.
Made the mentioned tweaks, please see attachments.
In regards to the button picker not showing in twig-debug mode - i think it has something to do with the injected HTML comments.
See Firebug reports this exception
Comment #6
widukind CreditAttribution: widukind commentedComment #7
widukind CreditAttribution: widukind commentedLooking into this a bit deeper, it appears to be the line-breaks surrounding the injected HTML comments that are causing the problem.
Throwing a
$.trim()
around the generated markup seems to do the trick.Comment #8
widukind CreditAttribution: widukind commentedAlright, this time FTW I hope.
Tweaked the JS code some more so that
(a) leading line breaks won't cause jQuery to break when parsing the given markup string
(b) the right container element gets captured for further processing
Tested in FF/Chrome/IE9, works for me.
Please review. Thanks!
Comment #9
star-szrTagging as JavaScript for the JS tweaks - I'm not sure if we can or should be solving those at a higher level. May assign to @nod_ for review if he doesn't see this first.
@widukind - it almost looks like you wrapped the markup in the Twig template at 80 characters. There's no need to do that, please unwrap any lines like this:
Comment #10
star-szrStatus for the Twig template tweaks. Thanks for all your work on this @widukind!
Comment #11
widukind CreditAttribution: widukind commentedUnwrapped markup as requested.
Comment #12
nod_problem was trying to create something detached from the DOM. When we add the HTML directly to the dom then select what we want, it works fine. Twig debug or not.
Comment #13
star-szrI diffed the generated source, the only difference is the added ID and it supports the JavaScript fix for the twig debug output. RTBC, thanks @widukind and @nod_!
Comment #13.0
star-szrAdd conversion summary table
Comment #14
c4rl CreditAttribution: c4rl commentedPer #1757550-44: [Meta] Convert core theme functions to Twig templates, retitling to indicate this issue applies to theme_ functions, which are lower in priority than PHPTemplate conversion issues.
Comment #15
alexpottComment #16
widukind CreditAttribution: widukind commentedComment #17
epersonae2 CreditAttribution: epersonae2 commented#12: core-ckeditor-conf-twig-1963474-12.patch queued for re-testing.
Comment #18
Pierre Paul Lefebvre CreditAttribution: Pierre Paul Lefebvre commentedRerunning profiling, forgot something.
Comment #19
jerdavisComment #20
ernie-g CreditAttribution: ernie-g commentedtake to 8.x
Comment #21
OpenChimp CreditAttribution: OpenChimp commentedprofiling this now
Comment #22
OpenChimp CreditAttribution: OpenChimp commented=== 8.x..8.x compared (519fe63632755..519ff02bc3dc7):
ct : 40,580|40,580|0|0.0%
wt : 377,694|379,099|1,405|0.4%
cpu : 335,787|334,898|-889|-0.3%
mu : 50,206,616|50,206,616|0|0.0%
pmu : 50,271,728|50,271,728|0|0.0%
Profiler output
=== 8.x..1963474 compared (519fe63632755..519ff0874d0e5):
ct : 40,580|40,580|0|0.0%
wt : 377,694|378,089|395|0.1%
cpu : 335,787|334,637|-1,150|-0.3%
mu : 50,206,616|50,206,896|280|0.0%
pmu : 50,271,728|50,273,112|1,384|0.0%
Profiler output
---
ct = function calls, wt = wall time, cpu = cpu time used, mu = memory usage, pmu = peak memory usage
The registry has been rebuilt. [success]
'all' cache was cleared in self
Comment #23
jerdavisStill needs profiling as the above results have no difference in calls, looks like it wasn't pointing to the right page.
Comment #24
jerdavisThis one's been tough to profile. The pages at admin/config/content/formats/%filter_format are actually pretty slow, and for some reason invoke a surprising number of database calls. That has made establishing a good baseline impossible. I started down the path of trying to figure out why the pages were slow, but realistically there are probably a lot bigger fish to fry out there than the filter admin page.
I'd almost suggest we skip profiling this with twig, as the page itself (without twig) is pretty slow. The switch to twig is going to be such a small difference it'd be a better use of time to debug the other performance issues here than to worry about twig.
Comment #25
ernie-g CreditAttribution: ernie-g commentedMANY APOLOGIES
It turns out, the profiling instructions I received at the DrupalCon Code Sprint was incomplete.
I am going to be re-running all my profiling :-/
Comment #26
OpenChimp CreditAttribution: OpenChimp commentedYes. Sorry, we were not aware of the extra steps needed to set up the website so that the profiling tests would be looking at the correct page that will test the patch. I will reproduce this and run the profile again.
Comment #27
steinmb CreditAttribution: steinmb commentedI can confirm the results from #24. The profiling results was is just all over the place. I suggest we move this into core as is.
Comment #28
star-szrNo longer applies, needs a reroll.
Comment #29
hussainwebRerolled patch from #12.
Comment #30
hussainwebComment #32
star-szr#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.
Comment #33
star-szrThanks @hussainweb, reroll looks good to me! Just waiting for testbot.
Comment #35
star-szr#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.
Comment #37
star-szr#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.
Comment #39
hussainwebI just pulled latest from 8.x branch and ran all User tests, and they passed. It looks like some other problem and hence queuing patch from #29 for retest.
Comment #40
hussainweb#29: core-ckeditor-conf-twig-1963474-29.patch queued for re-testing.
Comment #40.0
hussainwebUpdate remaining
Comment #41
star-szrThis needs another reroll.
Comment #42
hussainwebRe-rolled. The conflict was from changes in #1879120: Use Drupal-specific image and link plugins — use core dialogs rather than CKEditor dialogs, containing alterable Drupal forms and #1363112: Simplify names of "element-x" helper classes.
Comment #43
dead_armThe patch in #42 still applies. The only output markup change with the patch applied, also verified in #13, is the id,
id="ckeditor-button-configuration-container"
, added to the fieldset.For reference, to see this go to admin/config/content/formats/manage/basic_html and inspect 'Toolbar configuration'
Comment #44
dead_armWith dev tools:
Comment #45
joelpittetThank you very much @dead_arm I think this is good to go for profiling next:)
Comment #46
star-szrThis definitely needs some reworking after #1872206: Improve CKEditor toolbar configuration accessibility because the markup has changed.
Comment #46.0
star-szrUpdate conversion table
Comment #47
joelpittet42: core-ckeditor-conf-twig-1963474-42.patch queued for re-testing.
Comment #49
infojunkieComment #50
infojunkieRe-rolled for latest version.
Comment #52
infojunkieComment #53
joelpittet@infojunkie thanks for coming to the sprint and this re-roll looks great. I just reviewed the patch quickly and spotted a couple nit picky docblock items.
This is no longer needed.
this should be template_preprocess...
Also wonder if it passes without spaceless as whitespace shouldn't matter and removing it doesn't really make things easier to read. Testbot may complain though, just a warning.
Comment #54
infojunkieComment #55
infojunkieComment #56
joelpittetComment #57
joelpittetI was on to profiling this but it's back to needs re-roll. @infojunkie want to snag this?
Comment #58
infojunkieComment #59
infojunkieRe-rolled.
Comment #60
star-szrThanks @infojunkie!
Comment #61
joelpittetOk Marking this as RTBC. My test results look a bit funny but if you look at it, the 3ms and 11ms is due to PDOExecute which is unrelated. So if you take that off both times, the real WT difference is 2,716 - 3,273= -557microseconds on the first. 8.x vs 8.x and 14,840-11,887 = 2,953 microsecs on the 8.x vs ckeditor branch.
I'll have to look into fixing my.cnf or switching back to mysql or something...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c1d5ca86be3&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=52c1d5ca86be3&...
Comment #62
star-szrAdding steps to test manually.
Comment #63
star-szrIt looks like we're missing a closing
</li>
towards the end of the template. Doesn't need re-profiling but does need to be fixed up before we can RTBC again.Comment #64
joelpittetbecause it's inside another
which isn't allowed.
Comment #65
joelpittetDid markup comparison, there was some whitespace to fix with the addition of trans block. So this fixes the whitespace for that.
Comment #67
infojunkieComment #68
star-szr65: 1963474-65-twig-ckeditor.patch queued for re-testing.
Comment #69
star-szrSome other stuff snuck in, here is #64 combined with the interdiff from #65.
Going to do another manual test as well.
Comment #71
star-szrMarkup and functionality is a go and I reviewed the patch, looks good to me.
(RTBC when green)
Comment #72
star-szrOne line was mis-indented, fixed here.
Comment #74
star-szr72: 1963474-72.patch queued for re-testing.
Comment #75
webchickLooks good! The {% spaceless %} tag name struck me as hilarious for some reason. :P
Committed and pushed to 8.x. Thanks!