Whack. Who signed off those patches?

CommentFileSizeAuthor
#5 drupal8.static.5.patch6.99 KBsun
drupal8.static.0.patch3.15 KBsun

Comments

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Priority: Major » Normal
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Needs work

Why the changes to drupal_static() itself? That needs an explanation.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review 
+++ b/includes/bootstrap.inc
@@ -2879,8 +2879,10 @@ function &drupal_static($name, $default_value = NULL, $reset = FALSE) {
   // Reset all: ($name == NULL). This needs to be done one at a time so that
   // references returned by earlier invocations of drupal_static() also get
   // reset.
-  foreach ($default as $name => $value) {
-    $data[$name] = $value;
+  if ($reset) {
+    foreach ($default as $name => $value) {
+      $data[$name] = $value;
+    }
   }

That's a trivial change actually -- as the directly preceding comment explains, the function resets all statics to their defaults. However, it does not limit that operation to the $reset = TRUE case.

In order to see everything that's in drupal_static() and find these unnecessary statics in the first place, I called:

$all = drupal_static(NULL, NULL, FALSE);

and I naturally expected that passing FALSE for $reset does not reset anything - sounds logical, right? :) Instead, I expected to have it return everything it has.

1 days to next Drupal core point release.

catch’s picture

catch
he/him
Primary language English
commented

Looks like a good change, but the drupal_static() tests should probably have caught this.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.static.5.patch6.99 KB

Thanks! I've added assertions for that expectation and improved the drupal_static() test in general.

patrickd’s picture

patrickd commented
Status: Needs review » Needs work 
git apply ~/Desktop/drupal8.static.5.patch
error: patch failed: core/includes/form.inc:2470
error: core/includes/form.inc: patch does not apply

I've a general performance question on drupal_static
if (isset($data[$name]) || array_key_exists($name, $data)) {
array_key_exists costs the most performance in drupal_static, but why is the isset($data[$name]) not enough?

// edit
Removing array_key_exists($name, $data) the exclusive wall time dropped from 1.0% to 0.5% avg. (tested on my laptop).

Adding

if ($reset) {
    foreach ($default as $name => $value) {
      $data[$name] = $value;
    }
  }

made no significant difference for me

// edit :P
just talked in irc about this (array_key_exists($name, $data)) isset would return false if the value is null even if the key exists.. that's the reason

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
Issue tags: -Performance

#5: drupal8.static.5.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal8.static.5.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Issue summary: View changes
Status: Needs work » Closed (won't fix)

This issue appears to be obsolete; #1577902: [META] Remove all usages of drupal_static() & drupal_static_reset()