Expand Shortcut with methods

I'm not sure but i think we need to use setId() in this special case because we use the functionality from a setid() method in the class shortcut

  public function postCreate(EntityStorageControllerInterface $storage_controller) {
    // Generate menu-compatible set name.
    if (!$this->getOriginalID()) {
      // Save a new shortcut set with links copied from the user's default set.
      $default_set = shortcut_default_set();
      // Generate a name to have no collisions with menu.
      // Size of menu_name is 32 so id could be 23 = 32 - strlen('shortcut-').
      $id = substr($this->id(), 0, 23);
     $this->set('id', $id); // setId() methods ?
      if ($default_set->id() != $id) {
        foreach ($default_set->links as $link) {
          $link = $link->createDuplicate();
          $link->enforceIsNew();
          $link->menu_name = $id;
          $link->save();
          $this->links[$link->uuid()] = $link;
        }
      }
    }
  }

i work on it

Hi - I've rerolled the patch for latest 8.x branch and made some changes for review.

I do not believe that changes are required to Entity and ConfigStorageController which were proposed previously.

Biggest problem I see is that of removing the public modifiers on the id, label and links parameters of the ShortcutSet class. The ConfigEntityBase class uses reflection in its getExportProperties method to get all public parameters for export. We could override the getExportProperties but I wonder if this is the right approach?

Second issue is the $links parameter on the ShortcutSet class. It is used in two different ways to store two different types of data which feels really wrong - but maybe this is a standard pattern? Usually it is an associative array with the key as the MenuLink UUID and the value as the MenuLink object. However, in preSave and during Entity creation, it is a string array of UUIDs. I broke out the conversion from UUIDS to obejects into its own function but maybe there is a cleaner way to do this?

I'm at DrupalCon Prague code - in the coder lounge Thurs and Fri if anyone has a moment to go through this with me (ceng_ on irc).

Thanks!

1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/ShortcutSet.php
   @@ -46,36 +46,37 @@
   -   * The machine name for the configuration entity.
   -   *
   -   * @var string
   +   * @TODO this is still public because ConfigEntityBase::getExportProperties()
   +   * uses reflection to pick out the public properties to export.
       */
      public $id;
   

   Yes, I'd just leave those as they are for now. We're removing them for content entities, as they're not really there anyway, but let's not touch them here I think. But if we want to, I think @timplunkett knows how to do it.

2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/ShortcutSet.php
   @@ -46,36 +46,37 @@
   +   * @TODO $links should contain consistent values, usually this is MenuLink
   +   * objects but in preSave below its value is changed to an array of UUID
   +   * strings for export.
   

   Ok, this is weird, not sure what to do about it yet.

   What we usually do is store the raw values (like UUID's) but getter methods then load the entities or whatever.

3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/ShortcutSet.php
   @@ -46,36 +46,37 @@
   +  public function __construct(array $values, $entity_type) {
   +    $values['links'] = empty($values['links']) ? array() : $this->loadMenuLinksFromUuids($values['links']);
   +    parent::__construct($values, $entity_type);
   +  }
   

   Then this shouldn't be needed I think?

Thanks @berdir,

I think you are suggesting that $this->links should contain just the UUIDs (and so be renamed $link_uuids). Then the getMenuLinks function will load all the MenuLink objects when called.

Should I worry about caching the list of MenuLink objects via a private variable on the StorageSet class? That would make two variables which would need to be kept in sync, $this->link_uuids and $this->menu_links.

OK, so this patch contains ..

• The name of the $links variable is now $menu_link_uuids
• This variable is now only ever an array of uuids
• Updated the standard install profile as it was still setting the value of $links directly.
• Dropped the constructor and links conversion functions from the previous patch since having a consistent $menu_link_uuids means these checks are not needed
• Replaced entity_load_by_uuid with loadByProperties from the menu_link storage controller as suggested by @berdir
• getMenuLinks function now loads the menu links by id on demmand - no caching in StorageSet Entity for this
• addMenuLink and removeMenuLink functions both take a menu link UUID as the parameter now

Happy to chat about this with anyone in Prague today - I'm in the coder lounge!

The previous patch in #9 has a coding standards issue. The class parameter $menu_link_uuids should be camel case $menuLinkUuids. Attached patch fixes that.

#10: shortcut_expand-with-methods_2030657_10.patch queued for re-testing.

Changed the menu_link_uuids variable back to links.

It needs to be camelCase because it is a class property but needs to be underscore case because reflection puts it into a CMI export file. Needs to be one word so works in both cases, links makes most sense.

I've changed the modifiers to protected and overridden getExportProperties() like the \Drupal\views\Plugin\Core\Entity\View class does as @tim.plunkett suggests here:

https://drupal.org/node/2016679#comment-7659387

@ceng: I am sorry for you that your patch did not get a review earlier.
You patch was so out of date that I made a new patch.
Hope that my new patch get reviewed a little bit earlier. ;)

I also have a question: The shortcut entity has a computed base field called "path".
Does there need to be a function getPath() or not?

Checked the code an tested the form Add Shortcut

Patch ok

The patch looks good to me as well, RTBC++.

Committed/pushed to 8.x. Just a note I manually handled the file permission issues in the patch.