block_page_alter() queries for both hidden and all regions on every page, but only needs to know about hidden regions on the block admin page.

CommentFileSizeAuthor
#9 block.patch1.53 KBcatch
block.patch1.21 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

figaro’s picture

figaro CreditAttribution: figaro commented

Does the following code actually do anything in your if-statement?:
$visible_regions = system_region_list($theme, REGIONS_VISIBLE)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Nope, but the alternative would be a nested if I think, not keen on either.

figaro’s picture

figaro CreditAttribution: figaro commented

That's my point. If you swap it with isset($visible_regions[$region]) it might just be a tad faster for when the other two conditions fail.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

That's not possible though, since we need to set $visible_regions in order to do an isset() against it.

figaro’s picture

figaro CreditAttribution: figaro commented

I understand your approach now, thanks

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Fixed

looks good. this got introduced during some toolbar madness.

catch’s picture

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

I'm going to set this back to RTBC, since I don't think it got committed.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Favorite-of-Dries

I think it is slightly cleaner to split that if-test. Can you do a quick re-roll?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
block.patch1.53 KB

Here it is with the nested if.

moshe weitzman’s picture

moshe weitzman CreditAttribution: moshe weitzman commented
Status: Needs review » Reviewed & tested by the community
Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -Favorite-of-Dries

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Performance, +Favorite-of-Dries

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.