The naming of the variables in the new class panels_allowed_layouts should IMHO be reconsidered:
- $inclusive is very vague and could be renamed to something like $allow_new.
- $module_name is okay, but we should make up our mind if we want to use $module_name (as in panels_common_settings), or just $module (as in most other places). I think the latter is clear enough and shorter.
- $layout_settings is too ambiguous. One expects that it has something to do with the layout settings, panels_edit_layout_settings_form() provides, but in fact it has nothing to do with that. It might be better to call it simply $layouts. $allowed_layouts on the other hand would falsely imply that only allowed layouts are in the array.

Comments

sdboyer’s picture

sdboyer commented

I don't think that 'inclusive' is vague - it is, I think, the most technically descriptive term available. But I can also see that considering it that way is probably something you'd only grasp once you grok the whole class, so changing it to $allow_new is probably wise.

I disagree on $module, and I'm sticking with $module_name - the drupal way isn't so much about 'shorter' as it is about 'accurate,' and to me, $module_name evokes a string value much more clearly than does simply $module. Plus, being more explicit removes the possibility of ambiguity later if we need other $module-ish named vars.

Yep yep on $layout_settings - I'll change it to $allowed_layout_settings when I get the chance. Need the settings in there, b/c of your latter point, but it does tend to confuse with the other layout settings namespace.

sdboyer’s picture

sdboyer commented
Status: Active » Fixed

OK, committed these changes:

$inclusive changed to $allow_new

$layout_settings changed to $allowed_layout_settings

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

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