Expand Menu with methods

I was thinking of menu_link, not menu...

Hm, I do agree that getLangcode() (although that doesn't really match with most other EntityInterface methods, e.g. language()) would be useful, but seems like the wrong issue to add it :)

Also won't apply anymore.

Rerolled. Will conflict with #2084197: Uninstalling menu module removes all custom menus.

Giving this a go, if that's ok!

Got a patch for this, bit airport's WiFi said no more WiFi for the laptop. Will upload it tonight!

Patch attached.

#8: 2030645-expand-menu-with-methods.patch queued for re-testing.

Re-testing to see if it still passes. Any feedback? =)

#8: 2030645-expand-menu-with-methods.patch queued for re-testing.

The main thing I can see is:

+++ b/core/modules/system/lib/Drupal/system/Entity/Menu.php
@@ -59,7 +59,7 @@ class Menu extends ConfigEntityBase implements MenuInterface {
+  protected $description;

This member variable needs to be reverted back to being public for the patch - https://drupal.org/node/2016679#comment-7616179

patch is not applying ...

patche rerolled

Reroll the patch.

Is there a reason we do have this setter method even nothing in core uses it? I would though agree that for consistency introducing them would be great idea!

+++ b/core/modules/system/src/Entity/Menu.php
@@ -60,8 +60,31 @@ class Menu extends ConfigEntityBase implements MenuInterface {
+  /**
+   * {@inheritdoc}
+   */
+  public function setDescription($description) {
+    $this->description = $description;
+    return $this;
+  }
...
+  /**
+   * {@inheritdoc}
+   */
+  public function setLocked($locked) {
+    $this->locked = $locked;
+    return $this;
+  }

+++ b/core/modules/system/src/MenuInterface.php
@@ -15,6 +15,25 @@
+  /**
+   * Sets the description of the menu.
+   *
+   * @param string $description
+   *   The menu description.
+   *
+   * @return \Drupal\system\MenuInterface
+   *   The called menu entity.
+   */
+  public function setDescription($description);

@@ -22,4 +41,15 @@
+  /**
+   * Sets the locked status of a menu.
+   *
+   * @param bool $locked
+   *   TRUE to set the menu as locked, FALSE to set it as unlocked.
+   *
+   * @return \Drupal\system\MenuInterface
+   *   The called menu entity.
+   */
+  public function setLocked($locked);

We have not been adding setters that are completely unused in the other issues.

Can this issue also protect the id and label properties? Thanks.

Maybe we should set those as private methods? Btw latest patch patched cleanly (comment #16)

Removed setters. Make id and label properties protected.

Whoops, missed this label usage in test.

Reroll with conflicts resolved. Let's see if the testbot disagrees with my reroll-fu.

It looks like the patch in #24 includes out-of-scope changes.

Will now re-roll #21 and include interdiff from #23.

OK, so reroll with patch from #21 and interdiff from #23, rebased to 8.0.x. Success with zero conflicts.

+++ b/core/modules/menu_ui/src/Tests/MenuCacheTagsTest.php
@@ -60,7 +60,7 @@ public function testMenuBlock() {
     // Verify that after modifying the menu, there is a cache miss.
     $this->pass('Test modification of menu.', 'Debug');
-    $menu->label = 'Awesome llama';
+    $menu->set('label', 'Awesome llama');
     $menu->save();
     $this->verifyPageCache($path, 'MISS');

YesCT says this test needs to be refactored, to figure out if we need a setLabel().

This is as far as we got looking at the patch, will come back later.

Since the point of the test is to show the cache miss, and since I can't find any instances of using set('label') within the menu_ui module, I'd say it's safe to not add a setter for label.

In case we would like to have we should start to think about introducing one on the base entity level.

Reclassifying as a bug since exposure of properties as public allows code to not use the API eg ->id vs ->id(). This makes Drupal more stable and prevents bugs. Nice work.

Committed b34bcec and pushed to 8.0.x. Thanks!

Did we want a test for getDescription() ?