In menu_attributes.module, two constants are defined.

define('MENU_ATTRIBUTES_LINK', 'attributes');
define('MENU_ATTRIBUTES_ITEM', 'item_attributes');

But in the code, there are some places where the constants are used, and other places where the string literals are used instead.
Why?

E.g.

function _menu_attributes_form_alter(array &$form, array $item = array(), array &$complete_form) {
  [..]
  unset($form['options']['#value']['attributes']);
  unset($form['options']['#value']['item_attributes']);

  $form['options'][MENU_ATTRIBUTES_LINK] = array(
  [..]
  $form['options'][MENU_ATTRIBUTES_ITEM] = array(

Comments

donquixote created an issue.

joelpittet’s picture

Probably not a good reason... likely those constants should be removed as the pollute global namespace.

Do you have a proposal as you have this set as a 'task'?

donquixote’s picture

I would say the global namespace is not a problem if the constants are properly prefixed - which they are. So not any worse than a function.

I often use constants like this because they allow me to "find usages" and autocomplete with an IDE. And they help me to avoid typos. E.g. with variable_get() or cache_get(). I think it is a matter of taste whether to use them or not.

But if you are using them, they should be used across the board.
E.g. the following would defeat the purpose:

define('MYMODULE_VARNAME', 'mymodule_varname');

function mymodule_foo() {
  $x = variable_get(MYMODULE_VARNAME);
  variable_set('mymodule_varname', $x);
}

Here we have one place where the constant is used, and another place where the string literal is used.
I'd say either use the constant all the way, or the string literal all the way.

Of course this only applies to strings that actually mean the same thing, not strings that are just coincidentally identical.
E.g. if you had a cache key for cache_get() and a variable machine name for variable_get(), and both were the same string, then they should still be separate constants, imo.

In the case of menu_attributes, I am not sure. I think I see two ways the string 'item_attributes' is used:
1. In $item['options']['item_attributes'] saved with each menu link/item.
2. In $info['scope'].

But I tend to say that these two cases are the same, because here:

    foreach ($info['scope'] as $group) {
      // Merge in the proper default value.
      if (isset($item['options'][$group][$attribute])) {
donquixote’s picture

So yes, removing the constants or their usage is one possible way to "fix" this issue.
Replacing all string literals is the other option.

joelpittet’s picture

One thing to be careful of is that 'attributes' is used for both the key in the data table, and the render key and link option key which are not the same thing. So likely there is not 1/2 that are not converted to the constants are like that for a reason (excluding the the one you referenced in the issue summary that looks like an oversight).

Want to provide a patch either way? I'm not too picky.

donquixote’s picture

I am already away from my pc now.
But from what I remember: Renaming the key in one place would require a rename in all other places as well. Even if they are two things, they are still interdependent. Or not?

donquixote’s picture

The attributes are configured in the menu link edit form, stored in a serialized "options" column, and then read in hook_menu_link_alter() and hook_preprocess_menu_link(), right?

joelpittet’s picture

re #7 yes that is correct

#6 not necessarily because the 'attributes' key is used for both link attributes and a special key in drupal render array/link options. So if you were to rename it to 'link_attributes' for example it would break things.

donquixote’s picture

#6 not necessarily because the 'attributes' key is used for both link attributes and a special key in drupal render array/link options. So if you were to rename it to 'link_attributes' for example it would break things.

So menu_attributes does not really "invent" the 'attributes' key, but rather just uses the name already existing in core? Which makes the variable somewhat misleading.

I need to find some free time if I really want to do a patch for this. It would require reviewing and understanding all the usages of string literal and constant.

joelpittet’s picture

It's legacy baggage, it didn't do menu item attributes before only menu link attributes from what I recall...