Greetings,

I want to propose a performance improvement which uses drupal cache and reduces amont of SELECTs:

function block_class($block) {
  $cache = &drupal_static(__FUNCTION__);
  if (count($cache) <= 0) {
    $cache = db_query("SELECT CONCAT(module, delta) AS block_key, css_class FROM {block_class}")->fetchAllKeyed();
  }
  return !empty($cache[$block->module. $block->delta]) ? check_plain($cache[$block->module. $block->delta]) : '';
}

And a small change here to not call array_merge if classes are empty for a particular block:

function block_class_preprocess_block(&$vars) {
  $block = $vars['block'];
  $classes = trim(block_class($block));
  if (!empty($classes)) {
    $vars['classes_array'] = array_merge($vars['classes_array'], explode(' ', $classes));
  }
}

Your thoughts?

Comments

andypost’s picture

andypost
he/him
Primary language Russian
commented

Makes a lot of sense! please provide a patch.
Also better do not query all block_class table in preprocess and encapsulate all quiery staff in single function block_class(load) is good.
And replace static with drupal_static for testability

ardas’s picture

ardas commented

Hello Todd,

I have just seen your latest code and it seems you have already implement this but in a different manner. But the question why are you using static variable instead of using drupal_static() function?

berenddeboer’s picture

berenddeboer commented
Status: Active » Closed (fixed)
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Closed (fixed) » Active

I looks like fixed but requires a follow-up:
1) use drupal_static pattern to prepare for D8 in preprocess
2) a minor code-style issues, suppose it requires a new issue "Allow block_class to pass core review"

EDIT: the common practice to mark issues as Fixed to allow others to review changes. Also there's a some //commented staff in function that needs clean-up

berenddeboer’s picture

berenddeboer commented
Status: Active » Needs work

Thanks andypost for the correction!

dydave’s picture

dydave commented
Status: Needs work » Fixed

Hi ardas and andypost,

Thanks a lot for posting this feature request with code, all your reviews and kind follow-up.

@ardas:
I have reviewed the code suggested in the issue summary and it really seems like very good improvement in performance and code readability/maintenance.

I'm not really sure why the change wasn't implemented earlier when #2 was posted, then ticket abruptly closed at #3: I'm not certain I understand why the initial code wasn't really discussed previously.

In any case, I went ahead and committed the changes to the 7.x-1.x version at ad5af33 and 53462df, since it seems like a better way to handle the static cache for block_class_preprocess_block and block_class, which seems to be called by block_class_preprocess_panels_pane as well as direct access in theme template file (tpl.php, <?php print block_class($block); ?>).

Thanks a lot for your work and help on that.
However, if it isn't too much to ask, next time, we would greatly appreciate a standard patch (as it was requested at #1), it could save us some more time applying the changes for commit.

@andypost:
Thanks for keeping an eye on this issue.

To quickly follow up with the points you listed in #4:
1) Indeed, we're much better off using drupal_static and the changes suggested by @ardas in the issue summary have been committed at ad5af33 and 53462df.
2) Validation of coding standards has been carried at #1937930: Make block_class module pass Coder Review and already committed to all supported branches (for more information, see #1937930-4: Make block_class module pass Coder Review), but I invite you to take a quick look now at the PAReview/Ventral automated review of the 7.x-1.x branch.

Lastly, I took the opportunity in this ticket to do an extra change to the suggested code to support DBTNG, committed at a31b969.

I allowed myself to mark this issue as fixed for now, but feel free to re-open it, or post a new ticket, at any time if you have any further objections, suggestions, comments, complaints, recommendations, questions or issues with these commits or any of the points mentioned in this comment, we would certainly be glad to provide more information or explain in further details (we would surely be happy to hear your feedback).

Any further comments, feedback, questions, issues, testing, reporting, objections, suggestions or concerns on any of these commits or this ticket in general, would be highly appreciated.

Thanks again to everyone for your help, great work, reviews, feedback and comments on this issue.
Cheers!

andypost’s picture

andypost
he/him
Primary language Russian
commented

@DYdave Thanx a lot for taking over the module. 7.x-2.x branch is really promising. The number of queries seems reduced to 0 :)

Status: Fixed » Closed (fixed)

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