Properly deprecate MENU_MAX_MENU_NAME_LENGTH_UI

The patch leaves the one remaining instance in core.

grep -R MENU_MAX_MENU_NAME_LENGTH_UI ./core                                                               365ms  Fri 23 Nov 09:36:12 2018
./core/modules/menu_ui/menu_ui.module:const MENU_MAX_MENU_NAME_LENGTH_UI = ConfigEntityStorage::MAX_ID_LENGTH;

This one is a bit interesting because the new value is 166 and the old value is 27 so if you actually followed the advice of the existing deprecation you possibly would create menus with too long IDs for the UI.

Whoops

Hmmm.... okay so this is a false deprecation :(

The menu entity machine name is not only limited by configuration ID it is also limited by \Drupal\Core\Menu\MenuTreeStorage::schemaDefinition(). It has to be able to fit into

        'menu_name' => [
          'description' => "The menu name. All links with the same menu name (such as 'tools') are part of the same menu.",
          'type' => 'varchar_ascii',
          'length' => 32,
          'not null' => TRUE,
          'default' => '',
        ],

One solution is to add an update function and increase the size of this field. The other is to change the deprecation to use a specific constant.

Here's the Drupal 7 code.

/**
 * Maximum length of menu name as entered by the user. Database length is 32
 * and we add a menu- prefix.
 */
define('MENU_MAX_MENU_NAME_LENGTH_UI', 27);

So we no longer add the menu- prefix on the front of custom menus created via the UI. But as noted in #6 we still have the 32 character limit due to the menu_tree table. The other place this appears in the db is in the menu_link_content_data table. There is defined as:

    $fields['menu_name'] = BaseFieldDefinition::create('string')
      ->setLabel(t('Menu name'))
      ->setDescription(t('The menu name. All links with the same menu name (such as "tools") are part of the same menu.'))
      ->setDefaultValue('tools')
      ->setSetting('is_ascii', TRUE);

So it gets the default string length which is defined in MenuItem as 255. A length it can never be because menu names are part of the config entity ID and therefore subject to ConfigEntityStorage::MAX_ID_LENGTH - ie 166.

Discussed with @catch and @dawehner or slack. @catch wondered if there are other reasons for the 32 character limit - for example performance. @dawehner seemed in favour of increasing the menu_tree column size.

I think the safest course of action is to add a new \Drupal\system\MenuInterface::MENU_MAX_ID_LENGTH constant set to 32 and deprecate MENU_MAX_MENU_NAME_LENGTH_UI in favour of it. We can open a followup issue to discuss changing it to 166 and using ConfigEntityStorage::MAX_ID_LENGTH and that issue could also handle the base field.

Here's an approach where \Drupal\Core\Config\Entity\ConfigEntityStorage::save() will protect people from breaking their sites when menu names are too long.

Adding a test which shows we need late static binding FTW.

Argh.

Well if you follow the deprecation in HEAD you'd actually cause bugs in your code so I think we should commit this to 8.8.x and up and therefore changing the deprecation now is fine.

@Berdir yep our options are to either undeprecate in Drupal 8 and do this in Drupal 9.1 or to backport this to 8.8. I'd be in favour of moving forward instead of backward then forward.

Because we don't put entity storage classes there - see UserStorage, TermStorage, NodeStorage... they are all in the main src folder. The entity folder is like a plugin folder where the entity class exists.