the str_replace() used to perform the deletion of old variables when a node type is edited is sloppy -- if the variable name itself contains the node type name anywhere in the string, the variable name will be misinterpreted, and the variable will not be deleted. attached fixes this up to be specific.

CommentFileSizeAuthor
#5 node_type_variable_handling_2.patch1.15 KBpasqualle
#3 node_type_variable_handling_1.patch1.12 KBpasqualle
node_type_variable_handling.patch1.06 KBhunmonk

Comments

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

can you provide steps how to test this patch, please

hunmonk’s picture

hunmonk commented

basically, you need to go to the node type edit form for a node type (ex. 'page' node type at admin/content/types/page), change the 'Type' setting to rename the type, save it, and make sure that that persistent variables are properly transferred to the new names (ie, if you change from 'page' to 'blah' for the type name, then variable 'foo_blah' should be created w/ the same value as 'foo_page', then 'foo_page' should be deleted.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
StatusFileSize
new node_type_variable_handling_1.patch1.12 KB

ok, now I understand the problem with the str_replace function

created a new patch, which is much more readable, I think

how can I test the 'Reset to defaults' option? I changed it to delete the old variable, because that would the correct variable to delete. But the two variable names should be the same in this case anyway.

hunmonk’s picture

hunmonk commented

@Pasqualle: couple of things:

  1. The 'Reset to defaults' button only appears on module-created node types (ie, 'non-custom' types). This makes sense, because custom types have no defaults. So if you visit admin/content/types/blog, for example, you'll see that button.
  2. The variable_del() in the 'Reset to defaults' check can use either the old or new variable type name -- they will always be the same since the type name is hard coded by the module creating the type. From a readability standpoint, I'd say we stick with what you have.
  3. Overall, I think your code is more readable than my initial patch.

Code style looks good. I fully tested the lastest patch, and it works properly if the type name changes, doesn't change, or if variables are reset to defaults.

The only thing I might suggest is to write out $variable_new, instead of $var_new -- but it's probably fine as is.

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
StatusFileSize
new node_type_variable_handling_2.patch1.15 KB

replaced
$var_new -> $variable_new
$var_old -> $variable_old

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented
Status: Needs review » Reviewed & tested by the community

notice: Undefined index: type in ..\modules\node\node.module on line 569.

I see a php notice, when testing the "reset to defaults" function, but it was not introduced with this patch. So creating a new issue for it.

the "reset to defaults" and the type change is working correctly, so RTBC

pasqualle’s picture

pasqualle
Location 🇭🇺 Budapest
commented

This is an obvious programmer mistake. Explanation:

before patch

  // Save or reset persistent variable values.
  foreach ($variables as $key => $value) {
    $key .= '_'. $type->type;
    if ($op == t('Reset to defaults')) {
      variable_del($key);
    }
    else {
      if (is_array($value)) {
        $value = array_keys(array_filter($value));
      }
      variable_set($key, $value);

      if ($type->old_type != $type->type) {
        $key = str_replace($type->type, $type->old_type, $key);
        variable_del($key);
      }
    }
  }

after patch

  // Save or reset persistent variable values.
  foreach ($variables as $key => $value) {
    $variable_new = $key .'_'. $type->type;
    $variable_old = $key .'_'. $type->old_type;

    if ($op == t('Reset to defaults')) {
      variable_del($variable_old);
    }
    else {
      if (is_array($value)) {
        $value = array_keys(array_filter($value));
      }
      variable_set($variable_new, $value);

      if ($variable_new != $variable_old) {
        variable_del($variable_old);
      }
    }
  }

try

$key = 'comment'
$type->type = 'comment'
$type->old_type = 'foo'

after str_replace $key == 'foo_foo' but should be 'comment_foo'

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Reviewed & tested by the community » Fixed

Thanks, committed.

yched’s picture

yched commented
Version: 6.x-dev » 5.x-dev
Status: Fixed » Patch (to be ported)

Shouldn't this be backported ?

marcingy’s picture

marcingy commented
Status: Patch (to be ported) » Closed (won't fix)

Marking as won't fix as d5 is end of life.