ctools_block_content_type_render() doesn't need to load block info [#1988078]

ctools_block_content_type_render() calls _ctools_get_block_info() which calls _ctools_block_load_blocks() which may cause the system to do some unnecessary SQL queries, and will consume a useless amount of memory.

By calling directly module_invoke('block_view'); we actually skip this SQL request without loosing anything: for instance, panels calling this already bypasses the block cache policy, so why the heck not just avoiding to uselessly load the full block table and polluting memory?

This also avoids wrong PHP warnings when _block_rehash() has not been called while a block was added pragmatically in a panel.

Comment	File	Size	Author
#5	1988078-5-useless_block_info-1.2-do_not_test.patch	1.81 KB	pounard
#2	1988078-1-useless_block_info-1.2-do_not_test.patch	1.23 KB	pounard
#1	1988078-1-useless_block_info.patch	1.33 KB	pounard

Comments

Comment #1

pounard

French

commented 6 May 2013 at 13:32

Status:

Active

» Needs review

Status	File	Size
new	1988078-1-useless_block_info.patch	1.33 KB

Comment #2

pounard

French

commented 6 May 2013 at 14:13

Status	File	Size
new	1988078-1-useless_block_info-1.2-do_not_test.patch	1.23 KB

Attaching a patch for the 1.2 version (suitable for some production sites).

Comment #3

merlinofchaos commented 6 May 2013 at 21:58

I seem to recall that the calls you're removing are necessary for translation. The system that is going on to make block titles translatable and alterable is *really* fragile, and I'm *very* wary of changes here, because they have a habit of cascading every time I update something here. We've already had problems with code involved in blocks yo-yoing back and forth because of this.

Comment #4

pounard

French

commented 7 May 2013 at 06:14

Status:

Needs review

» Needs work

I have to admit I rushed writing and putting this patch here, it gives me a public link in order to put it into a drush make file. Nevertheless I think it can be improved for everyone.

I have some concerns too, one minor one is that your original algorithm uses the block.title table column if not overriden in the panel itself. But the block.title field is for core blocks configuration: it sounds very edgy to me that the user would configure the title in the admin/structure/block then put the block into a panel where he could directly change the title from there!

Alteration is probably more fragile, indeed, it might need some love, but I'm quite sure that no man on earth would use the $info array or the block.title object property in order to do the alteration stuff, both mostly contain textual information (except the info cache key) on which it's not possible to base an algorithm on.

I will do some more tests and give some feedback when I'll time for this, thanks for the answer.

Switching to needs work.

Comment #5

pounard

French

commented 7 May 2013 at 14:23

Status	File	Size
new	1988078-5-useless_block_info-1.2-do_not_test.patch	1.81 KB

My patch does some things wrong, and I didn't detected it either in the drupal.org infrastructure tests because it seems to be untested, either on my local VM because the sysadmin choose to make PHP errors silent. Here is a CTools 1.2 version fixed patch.

Comment #6

pounard

French

commented 7 May 2013 at 15:01

@merlinofchaos FYI I did some introspection, are you sure that adding the 'translatable' tag on the {block} table query will indeed triger block title translations? If so, then my patch is not a good thing to apply on multilingual sites, but if not, then the SQL query is useless. What's your opinion about this?

Comment #7

merlinofchaos commented 7 May 2013 at 16:34

I never looked too deeply into what i18nblocks does, myself.

What I remember, and this may be inaccurate because I didn't spend much time with it, but what I remember is that i18n uses hook_block_view_alter and it utilizes the $info and modifies data in it. And that when I didn't have that $info available, then i18n would fail to translate block titles. You might want to check that out a little. Also, a little git annotate on the blocks.inc file should lead you to several issues where this was worked on. In at least one issue, DamZ and someone else disagreed vocally over what should be happening (versus what was really happening) and I've done my best to accommodate everyone's code working as much as possible.

I recall agonizing a lot over that $info even needing to be there, but it seems that it really did need to be there for translation. But I can't say authoritatively why or how it gets used.

Comment #8

pounard

French

commented 16 May 2013 at 12:29

Ok, thanks a lot for this answer, I will try to see into this later if I find some time for it. Meanwhile, I will continue to use this patch on sites where it works.