Only allow loading by plugin definition properties

Can you expand on why this is needed or what it actually does?

Updated, but I think this is nothing we should do. If you as a developer ask for a wrong name, this is just your fault.

Shouldn't be worked on until after #2301319: MenuLinkNG part5: Remove dead code; and party!

ready to go ahead

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -635,11 +636,12 @@ protected function prepareLink(array $link, $intersect = FALSE) {
+        throw new InvalidPluginDefinitionException('An invalid plugin definition was specified: ' . $name);

Use sprintf or formatString, please

I might consider using just \InvalidArgumentException instead of the plugin exception?

Also, the method doc needs to have an @throws added

I think we should also expand the MenuTreeStorageTest to check that the exception is thrown.

Let's put the @throws on the interface, and leave this as @inheritdoc

Also, the exception message isn't quite right: 'An invalid plugin definition' should be more like 'An invalid property name'

Also, while you're at it:
Consider using @expectedException and @expectedExceptionMessage instead of the the try/catch.

@tim.plunkett Is @expectedException usable? It's not phpunit.

Oh, I missed that it was simpletest. Carry on!

Looks good.

+++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
@@ -300,6 +300,24 @@ public function testSubtreeHeight() {
+    try {
+      $this->treeStorage->loadByProperties($properties);
+    }

We need a $this->fail('An invalid property name throws an exception.');

+++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
@@ -300,6 +300,24 @@ public function testSubtreeHeight() {
+      $this->pass(t('An invalid property name throws an exception.'));

We don't use t() assertion messages anymore.

Thanks @cilefen

Are we sure we want to do this? We don't do this in any of the other loadByProperties implementations - perhaps we're being overly defensive - yes we'll get an SQL error during development but that error will explain nicely to the developer that they've done a loadByProperties on a non-existing property - maybe I'm missing something?

diff --git a/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
index d68ccb2..f24d54c 100644
--- a/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
+++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
@@ -305,12 +305,13 @@ public function testSubtreeHeight() {
   public function testLoadByProperties() {
     $properties = array('foo' => 'bar');
     try {
+      $msg = 'An invalid property name throws an exception.';
       $this->treeStorage->loadByProperties($properties);
-      $this->fail('An invalid property name did not throw an exception.');
+      $this->fail($msg);
     }
     catch (\InvalidArgumentException $e) {
       $this->assertEqual('An invalid property name, foo was specified.', $e->getMessage());
-      $this->pass('An invalid property name throws an exception.');
+      $this->pass($msg);
     }
     $this->addMenuLink(1);
     $properties = array(array('menu_name' => 'tools'));

@cilefen the pass and fail messages should not be different. Perhaps what is above would be better.

@alexpott - so, yes we want to do this. Here's why: it should only be supported by the API to load using the definition properties. However, any given implementation may have a variety of additional properties/columns in the storage. We need to prevent people from making assumptions about the implementation.

+++ b/core/lib/Drupal/Core/Menu/MenuTreeStorage.php
@@ -635,11 +635,12 @@ protected function prepareLink(array $link, $intersect = FALSE) {
+      if (!in_array($name, $this->definitionFields())) {
+        throw new \InvalidArgumentException(String::format('An invalid property name, @name was specified.', array('@name' => $name)));
+      }

Can we list the available fields in the exception?

Includes @alexpott's and @dawehner's suggestions. As always, thank you for reviewing.

Cool, thank you.

Committed 25627b0 and pushed to 8.0.x. Thanks!

Committed 25627b0 and pushed to 8.0.x. Thanks!

+++ b/core/modules/system/src/Tests/Menu/MenuTreeStorageTest.php
@@ public function testLoadByProperties() {
+    $properties = array(array('menu_name' => 'tools'));
+    $links = $this->treeStorage->loadByProperties($properties);
+    $this->assertEqual('tools', $links[1]['menu_name']);

Sorry, but the test code is broken; bogus double-array:

-    $properties = array(array('menu_name' => 'tools'));
+    $properties = array('menu_name' => 'tools');

HEAD should fail, but it does not. Here's why:

Insufficient parameter validation. The validation in loadByProperties() performs a loose comparison; a non-empty array is always "in" an array that is not empty.

Rule of thumb: Always use a strict comparison for in_array() unless you have a very good reason to not use it:

-      if (!in_array($name, $this->definitionFields())) {
+      if (!in_array($name, $this->definitionFields(), TRUE)) {

In addition to that though, for some unknown reason, HEAD seems to return a result that happens to match the assertEqual() test (equally loose comparison), which I can't explain, but definitely must not happen.

@sun your attention to detail is appreciated. Here are the fixes you suggested.

@cilefen: It wasn't me. :) This is really just one of many errors that would have been natively caught by PHPUnit: #2304461: KernelTestBaseTNG™

So, I think the other problem here is that all menu links are in tools by default, so it would be a better test to load one using a different menu name.

Added the test link to a different menu as per @pwolanin's suggestion and added an additional assert to be sure we are looking at the correct link.

Minor, but we should use a distinct string ID instead of 1.

The actual fail was the 0 to string compare, not nested array, so let's test that too.

This change to the test verifies that case fails without the strict comparison in the full patch.

@pwolanin Thanks.

Committed a04820c and pushed to 8.0.x. Thanks!