Another small improvement of drupal_render(): conditionally call element_children().

CommentFileSizeAuthor
#14 drupal_render_rollback.patch1.05 KBcatch
#4 element_children_before.png61.09 KBcatch
#4 element_children_after.png44.73 KBcatch
#3 drupal_render.patch1.01 KBcasey
#2 drupal_render.patch1.03 KBcasey
patch.txt1.03 KBcasey

Comments

casey’s picture

casey commented
Status: Active » Needs review
casey’s picture

casey commented
StatusFileSize
new drupal_render.patch1.03 KB
casey’s picture

casey commented
StatusFileSize
new drupal_render.patch1.01 KB

bluh, line endings.

catch’s picture

catch
he/him
Primary language English
commented
StatusFileSize
new element_children_after.png44.73 KB
new element_children_before.png61.09 KB

This looks good, we already have tests for drupal_render() sorting so it's clearly passing those.

Attached before/after screenshots from kcachegrind. When displaying ten nodes, this cuts calls to element_children() by half.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Forgot to set RTBC.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

moshe weitzman’s picture

moshe weitzman commented
Status: Fixed » Reviewed & tested by the community

No brainer. +1.

Bojhan’s picture

Bojhan commented
Status: Reviewed & tested by the community » Fixed

You guys crossposted

yched’s picture

yched commented
Status: Fixed » Active

Hm, I get node forms without title, body, nor any Field API widgets in current HEAD. Back to normal if I revert that patch :-p.

casey’s picture

casey commented

me too :p how did this pass?

yched’s picture

yched commented

"How did this pass".
How indeed. We do test Field API form integration, but on our 'test_entity' forms, not specifically on node forms. Not sure why this patch would act on node forms or 'test_entity' forms differently.

yched’s picture

yched commented

Er, yet with body and title as fields, it's quite surprising that *0* test failed in our whole suite...

casey’s picture

casey commented

Ow I do see title and body, but forms are sorted incorrectly.

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Needs review
StatusFileSize
new drupal_render_rollback.patch1.05 KB

Hm, I looked at node listing when doing the profiling, but not the forms. Here's a rollback anyway.

dries’s picture

dries commented
Status: Needs review » Needs work

Rollbacked using patch #14.

casey’s picture

casey commented
Status: Needs work » Closed (works as designed)

Call to element_children is necessary as it is sorting the elements (since D7).