MenuLinkNG part2 (no UI or conversions): MenuLinkTree API and unit tests

Wow, what a heroic effort!

Just observations

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -0,0 +1,175 @@
   +class DefaultMenuLinkTreeManipulators {
   

   I really love how this class collects a bunch of helper methods! Nice name!

2. +++ b/core/lib/Drupal/Core/Menu/MenuActiveTrail.php
   @@ -0,0 +1,97 @@
   +/**
   + * Provides the default implementation of the active menu trail service.
   + */
   +class MenuActiveTrail implements MenuActiveTrailInterface {
   

   I wonder whether we could give a one-liner how the default implementation looks like? Just a quick summary

3. +++ b/core/lib/Drupal/Core/Menu/MenuActiveTrail.php
   @@ -0,0 +1,97 @@
   +   *   A ruote match object for finding the active link.
   

   Typo: ruote, you probably didn't worked enough of routes, yet.

4. +++ b/core/lib/Drupal/Core/Menu/MenuActiveTrailInterface.php
   @@ -0,0 +1,54 @@
   +   *   The menu link for the given route name, parameters and menu, or NULL if
   +   *   the current user cannot access the current page (i.e. we have a 403
   +   *   response).
   

   It should also return NULL if there is no matching menu link, right?

5. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -0,0 +1,228 @@
   +  /**
   +   * Service providing overrides for static links
   +   *
   +   * @var \Drupal\Core\Menu\StaticMenuLinkOverridesInterface
   +   */
   +  protected $overrides;
   

   This one is not used in this class. This is also nice in terms of separation of concerns, because the tree service should not assume anything about the storage. No, you don't need to inject a service just to use "overrides" in a comment ;)

6. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -0,0 +1,228 @@
   +    $parameters = new MenuTreeParameters();
   

   oh nice!

7. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php
   @@ -0,0 +1,125 @@
   + * \Drupal\Core\Menu\MenuLinkTreeInterface objects represent a menu link's data.
   

   Should be MenuLinkTreeElement instead of interface, right?

8. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php
   @@ -0,0 +1,125 @@
   +    $sum = function ($carry, MenuLinkTreeElement $element) {
   +      return $carry + $element->count();
   +    };
   +    return 1 + array_reduce($this->subtree, $sum);
   

   <3

9. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
   @@ -0,0 +1,142 @@
   +/**
   + * Defines an interface for retrieving menu link trees.
   + */
   +/**
   

   Let's merge the two docblocks, this looks odd.

10. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelector.php
    @@ -0,0 +1,165 @@
    +
    +class MenuParentFormSelector implements MenuParentFormSelectorInterface {
    

    Missing docs, just a note

11. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelector.php
    @@ -0,0 +1,165 @@
    +          $title .= ' (' . t('disabled') . ')';
    

    Just in case someone wants to fix that t() call

12. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelectorInterface.php
    @@ -0,0 +1,56 @@
    +
    +interface MenuParentFormSelectorInterface {
    

    ... docs

13. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
    @@ -0,0 +1,263 @@
    + * @group Drupal
    

    just in case: the new phpunit test "standard" recommends to not add @group Drupal anymore

14. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
    @@ -0,0 +1,263 @@
    +   *
    +   * This mocks a tree with the following structure:
    +   * - 1
    +   * - 2
    +   *   - 3
    +   *     - 4
    +   * - 5
    +   *   - 7
    +   * - 6
    +   * - 8
    +   *
    

    I love those kind of comments, really helpful!

15. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeParametersTest.php
    @@ -0,0 +1,162 @@
    +    $parameters->addCondition('id', 1337, '<');
    

    OH I see the start of the Hundred Years' War

part 1 is close. starting on this.

this do not test patch is an updated just diff to part 1 that is in #2301239-34: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage
menu-2256521-MenuLinkTree-review-3-do-not-test.patch is just part 2.

does not include improvements mentioned in #2

A bit:

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -0,0 +1,175 @@
   +class DefaultMenuLinkTreeManipulators {
   

   Do we need an interface? The access especially could be potentially needed to be swapped out?

2. +++ b/core/lib/Drupal/Core/Menu/MenuActiveTrailInterface.php
   @@ -0,0 +1,54 @@
   + * The active trail of a given menu is the trail from the current page to the
   + * root of that menu's tree.
   

   i wonder whether we should rather move this detail into the class. the trail could be also decided on other logic and not go back to the root of the tree.

3. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -0,0 +1,228 @@
   +   * Service providing overrides for static links
   

   missing dot

4. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -0,0 +1,228 @@
   +    // Pre-load all the route objects in the tree for access checks.
   +    if ($data['route_names']) {
   +      $this->routeProvider->getRoutesByNames($data['route_names']);
   +    }
   

   I wonder whether this is the right layer where the preloading is done.

5. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTree.php
   @@ -0,0 +1,228 @@
   +  /**
   +   * Helper function that recursively instantiates the plugins.
   +   */
   +  protected function createInstances($data_tree) {
   

   ... Needs docs updates. It should also describe the difference between MenuLinkManager::createInstance and self::createInstaces()

6. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php
   @@ -0,0 +1,125 @@
   +  /**
   +   * The menu link for this element in a menu link tree.
   +   *
   +   * @var \Drupal\Core\Menu\MenuLinkInterface|array
   +   */
   +  public $link;
   

   it should clarify what the array is. maybe a list of MenuLinkInterface[]?

7. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php
   @@ -0,0 +1,125 @@
   +  /**
   +   * Whether this link is accessible by the current user.
   +   *
   +   * @var bool|NULL
   +   */
   +  public $access;
   

   NULL means that the access has not been determined yet. I guess we should document that.

8. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
   @@ -0,0 +1,142 @@
   +   *   A data structure representing the tree as returned from ::load().
   

   should be static::load

9. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeInterface.php
   @@ -0,0 +1,142 @@
   +   * Find expanded links in a menu given a set of possible parents.
   

   finds

10. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelector.php
    @@ -0,0 +1,165 @@
    +   * @param MenuLinkTreeInterface $menu_link_tree
    ...
    +   * @param EntityManagerInterface $entity_manager
    

    Should be absolute with the namespace.

1. +++ b/core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php
   @@ -0,0 +1,151 @@
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Tests \Drupal\Core\Menu\MenuLinkTree',
   +      'description' => '',
   +      'group' => 'Menu',
   +    );
   +  }
   

   We should convert it to use the new annotation based tests.

2. +++ b/core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php
   @@ -0,0 +1,151 @@
   +    /**
   +     * This creates a tree with the following structure:
   +     * - 1
   +     * - 2
   +     *   - 3
   +     *     - 4
   +     * - 5
   +     *   - 7
   +     * - 6
   +     * - 8
   +     *
   +     * With link 6 being the only external link.
   +     */
   

   Don't we use "//" for all inline comments?

3. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -0,0 +1,263 @@
   + * @group Drupal
   

   remove @group Drupal

4. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -0,0 +1,263 @@
   +   *
   +   * @var array
   

   is the tree an array of some objects?

5. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -0,0 +1,263 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function getInfo() {
   +    return array(
   +      'name' => 'Tests \Drupal\Core\Menu\DefaultMenuLinkTreeManipulators',
   +      'description' => '',
   +      'group' => 'Menu',
   +    );
   +  }
   +
   

   no need anymore

6. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -0,0 +1,263 @@
   +  /**
   +   * Helper function to create a mock tree.
   +   *
   

   doc changes needed

7. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -0,0 +1,263 @@
   +    $this->originalTree[1] = new MenuLinkTreeElement($this->links[1], FALSE, 1, FALSE, array());
   +    $this->originalTree[2] = new MenuLinkTreeElement($this->links[2], TRUE, 1, FALSE, array(
   +      3 => new MenuLinkTreeElement($this->links[3], TRUE, 2, FALSE, array(
   +          4 => new MenuLinkTreeElement($this->links[4], FALSE, 3, FALSE, array()),
   +        )),
   +    ));
   +    $this->originalTree[5] = new MenuLinkTreeElement($this->links[5], TRUE, 1, FALSE, array(
   +      7 => new MenuLinkTreeElement($this->links[7], FALSE, 2, FALSE, array()),
   +    ));
   +    $this->originalTree[6] = new MenuLinkTreeElement($this->links[6], FALSE, 1, FALSE, array());
   +    $this->originalTree[8] = new MenuLinkTreeElement($this->links[8], FALSE, 1, FALSE, array());
   

   It is odd that we don't set the key 0. Would it be possible to substract 1 to all of them?

8. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -0,0 +1,263 @@
   +    $this->mockTree();
   +    $tree = $this->originalTree;
   

   This is odd. Should this function maybe return the tree?

9. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
   @@ -0,0 +1,172 @@
   + * @group Drupal
   

   remove

10. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
    @@ -0,0 +1,172 @@
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'Tests \Drupal\Core\Menu\MenuActiveTrail',
    +      'description' => '',
    +      'group' => 'Menu',
    +    );
    +  }
    +
    

    remove

11. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
    @@ -0,0 +1,172 @@
    +  /**
    +   * Provides test data for all test methods.
    +   */
    +  public function provider() {
    

    Some improvements for the docs would be cool.

12. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
    @@ -0,0 +1,172 @@
    +    $request = (new Request());
    ...
    +    $request = (new Request());
    ...
    +    $request = (new Request());
    

    no need for this additional parenthesis

13. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkMock.php
    @@ -0,0 +1,68 @@
    +
    +class MenuLinkMock extends MenuLinkBase {
    

    needs a one liner

14. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeElementTest.php
    @@ -0,0 +1,63 @@
    + * @group Drupal
    ...
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'Tests \Drupal\Core\Menu\MenuLinkTreeElement',
    +      'description' => '',
    +      'group' => 'Menu',
    +    );
    +  }
    

    remove

15. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeElementTest.php
    @@ -0,0 +1,63 @@
    +    $this->assertSame(1, $child_item->count());
    +    $this->assertSame(2, $parent_item->count());
    

    OH I am a bit confused about that method name. Would childCount() make more sense?

16. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuLinkTreeParametersTest.php
    @@ -0,0 +1,162 @@
    + * @group Drupal
    ...
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'Tests Drupal\Core\Menu\MenuLinkTreeParameters',
    +      'description' => '',
    +      'group' => 'Menu',
    +    );
    +  }
    +
    

    remove

1. index 5ca7305..00830a5 100644
   --- a/.htaccess
   
   --- a/.htaccess
   +++ b/.htaccess
   
   +++ b/.htaccess
   @@ -126,20 +126,17 @@ DirectoryIndex index.php index.html index.htm
   

   nope, this is wrong. probably a merge error.

2. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
   @@ -0,0 +1,222 @@
   +   * @codeCoverageIgnore
   ...
   +   * @codeCoverageIgnore
   

   why this?

3. +++ b/core/lib/Drupal/Core/Menu/MenuTreeParameters.php
   @@ -0,0 +1,222 @@
   +  public function topLevelOnly() {
   

   at least some verb would be cool for this methdo name, what about setTopLevelOnly?

Question about getParentSelectOptions - It is marked deprecated in MenuParentFormSelectorinterface.php so i am not sure why are we using deprecated method in the patch.

   * @return array
   *   Keyed array where the keys are contain a menu name and parent ID and
   *   the values are a menu name or link title indented by depth.
   *
   * @deprecated
   */
  public function getParentSelectOptions($id = '', array $menus = NULL);

   * @return $this
   *
   * @codeCoverageIgnore
   */
  public function setMaxDepth($max_depth) {

I am not sure why are we using @codeCovergaeIgnore to ignore the test coverage on setMaxDepth function.

I am working on part 2 based on dawehner reviews. Peter is working on rebasing the branch against head so I need to wait for him to finish so I can clone the repo and continue to work. I am going to start work again tomorrow on this issue.

A couple of more reviews …

+ * @group Drupal
+class MenuLinkTreeParametersTest extends UnitTestCase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'Tests Drupal\Core\Menu\MenuLinkTreeParameters',
+      'description' => '',
+      'group' => 'Menu',
+    );
+  }

getInfo() and @group Drupal can be removed

 * @group Drupal
+ * @group Menu
+ *
+ * @coversDefaultClass \Drupal\Core\Menu\MenuLinkTreeElement
+ */
+class MenuLinkTreeElementTest extends UnitTestCase {
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getInfo() {
+    return array(
+      'name' => 'Tests \Drupal\Core\Menu\MenuLinkTreeElement',
+      'description' => '',
+      'group' => 'Menu',
+    );
+  }
+

The same …

+class MenuLinkMock extends MenuLinkBase {

Needs a one-liner.

+class MenuActiveTrailTest extends UnitTestCase {

getINfo and @group Drupal

+  public function testGetActiveLink(Request $request, $links, $menu_name, $expected_link) {
+    $this->requestStack->push($request);
+    if ($links !== FALSE) {
+      $this->menuLinkManager->expects($this->exactly(2))
+        ->method('loadLinksbyRoute')
+        ->will($this->returnValue($links));
+    }

+  public function testGetActiveTrailIds(Request $request, $links, $menu_name, $expected_link, $expected_trail, $expected_cache_k
+      $this->menuLinkManager->expects($this->exactly(2))
+        ->method('loadLinksbyRoute')
+        ->will($this->returnValue($links));

We should check with ->with() that the expected route name is passed into the loadLinksByRoute method.
On top of that the test could be simplified a bit by using a RouteMatch object instead of a CurrentRouteMatch, so we would not
have to pass along the request stack and the request.

+class DefaultMenuLinkTreeManipulatorsTest extends UnitTestCase {

getInfo()

+   * The original menu tree build in mockTree()
+   * Array of menu link instances

Missing dot

+          4 => new MenuLinkTreeElement($this->links[4], FALSE, 3, FALSE, array()),
+        )),

One level too much indentation

+/**
+ * Tests the menu link tree.
+ *
+ * @see \Drupal\Core\Menu\MenuLinkTree
+ */
+class MenuLinkTreeTest extends KernelTestBase {

Needs @group + removal of getInfo()

+   * The tested menu link tree.
+   * The menu link plugin mananger

Dots

+interface MenuParentFormSelectorInterface {

+   * Get a form element to choose a menu and parent.

Gets ...

+   * @param string $id
+   *   Optional ID of a link plugin. This will exclude the link and its
+   *   children from the select options.
+   * @param array $menus
+   *   Optional array of menu names as keys and titles as values to limit
+   *   the select options.  If NULL, all menus will be included.

Should use (optional)

+   * @deprecated

I guess we wanted people to better use parentSelectElement instead(). We should write that down here.

  * @param string $id
+   *   Optional ID of a link plugin. This will exclude the link and its
+   *   children from being selected.
+   * @param array $menus
+   *   Optional array of menu names as keys and titles as values to limit
+   *   the values that may be selected. If NULL, all menus will be included.

(optional)

+class MenuParentFormSelector implements MenuParentFormSelectorInterface {

+   * @param array $menu_names
+   *   Optional array of menu names to limit the options, or NULL to load all.

(optional)

+interface MenuLinkTreeInterface {

+ * If desired, one can transform (::transform()) that tree of menu links, for
+ * array (::build()) for rendering as HTML.

I guess we want rather static::transform() given that this is more how our codestyle looks like, though this could be discussed
and added to PSR-5

+  public function getCurrentRouteMenuTreeParameters($menu_name);

This method needs verbs in the one-liner.

+   *               by ControllerResolverInterface::getControllerFromDefinition()

Let's use an absolute path, to make xjm's life easier.

+class MenuLinkTreeElement {

+    $sum = function ($carry, MenuLinkTreeElement $element) {

I wonder whether it is worth to try out 'static $element' instead. Otherwise the adaptability of this class could be more difficult

+class MenuLinkTree implements MenuLinkTreeInterface {

+  public function transform(array $tree, array $manipulators) {
+    foreach ($manipulators as $manipulator) {
+      $callable = $manipulator['callable'];
+      if (!is_callable($callable)) {
+        $callable = $this->controllerResolver->getControllerFromDefinition($callable);
+      }

We don't really have to check is_callable, ... the controller resolver returns just the callable if it is already one.

+class MenuActiveTrail implements MenuActiveTrailInterface {

+    // Note: this is a very simple implementation. If you need more control
+    // over the return value, such as matching a prioritized list of menu names,
+    // you should substitute your own implementation for the 'menu.active_trail'
+    // service in the container.

Maybe it is worth to mention that loadLinksByRoute() already sorts by depth, weight and ID, so it is
not that simple ...

#2: Glad to see you like 6. and 8. so much :)

#4.1: No: a menu link tree manipulator is just a callback that accepts a menu link tree and returns the manipulated menu link tree. This service just contains the default manipulators. So when you'd want to use a different access check manipulator (which is indeed utterly essential), you'd change

     $manipulators = array(
       array('callable' => 'menu.default_tree_manipulators:checkAccess'),
       array('callable' => 'menu.default_tree_manipulators:generateIndexAndSort'),
     );
     $tree = $this->menuLinkTree->transform($tree, $manipulators);

to

     $manipulators = array(
       array('callable' => 'my_module.my_service:myOptimizedAccessChecker'),
       array('callable' => 'menu.default_tree_manipulators:generateIndexAndSort'),
     );
     $tree = $this->menuLinkTree->transform($tree, $manipulators);

See menu_link.cacheable_access_check_tree_manipulator in #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees.

#4.4: I agree that the preloading doesn't belong here.

#5.7: the numeric keys correspond to mlids, which were the IDs used to load menu links in 7/HEAD, and now still exist in the DB. 0 was and still is the virtual "root" node of the menu link tree. But off the top of my head (still getting back into this), I think it would be fine to convert this to string keys, which is what part1 + part2 (this patch) introduce.

#5.15: I think you're confused by the variable names. count() counts everything in the given tree, with the MenuLinkTreeElement object it's invoked on being considered the root. So given a tree of the shape

PARENT
  |-- CHILD

Then it makes sense. childCount() would imply only the children are counted, but this is not the case.

#6.2/#8: Because these are straight setters, there's close to no value in writing test coverage for them. If deemed valuable enough, we can add test coverage for that of course.

#6.3: agreed.

#7: Because pwolanin and I hadn't gotten around yet to removing it; it's a low-priority thing in the grand scheme of things. It's an implicit @todo, if you will.

#5.15: I think you're confused by the variable names. count() counts everything in the given tree, with the MenuLinkTreeElement object it's invoked on being considered the root. So given a tree of the shape

Then it makes sense. childCount() would imply only the children are counted, but this is not the case.

Thanks for clarifying! Makes sense

#6.2/#8: Because these are straight setters, there's close to no value in writing test coverage for them. If deemed valuable enough, we can add test coverage for that of course.

I would be totally fine to not add test coverage here, or at all, but hiding this seems bad.

:D

Here's the current step2 only patch (do-not-test), and step1 + step2 combined for tests

missed this typo before:

MenuLinkInterface::isResetable()

Should be:

MenuLinkInterface::isResettable()

- will have to propagate that to the next steps too.

I wonder whether this is the right layer where the preloading is done.

Added a follow up #2303463: Move route preloading for menu trees out of the MenuLinkTree

It is odd that we don't set the key 0. Would it be possible to substract 1 to all of them?

These are IDs so using 1 makes a little bit more sense, tbh.

This is odd. Should this function maybe return the tree?

There is no point in changes this.

Fixed the resettable bit.

Needs step 2 only patch. Also issue summary is practically missing, makes it hard to review.

Here's a clean step2 patch

+++ b/core/includes/batch.inc
@@ -445,16 +445,15 @@ function _batch_finished() {
-    // Use \Drupal\Core\Form\FormSubmitterInterface::redirectForm() to handle
-    // the redirection logic.
-    $redirect = \Drupal::service('form_submitter')->redirectForm($_batch['form_state']);
+    // Use drupal_redirect_form() to handle the redirection logic.
+    $redirect = drupal_redirect_form($_batch['form_state']);
     if (is_object($redirect)) {
       return $redirect;
     }
 
     // If no redirection happened, redirect to the originating page. In case the
     // form needs to be rebuilt, save the final $form_state for
-    // \Drupal\Core\Form\FormBuilderInterface::buildForm().
+    // drupal_build_form().
     if (!empty($_batch['form_state']['rebuild'])) {
       $_SESSION['batch_form_state'] = $_batch['form_state'];
     }
diff --git a/core/lib/Drupal/Core/Form/FormBuilderInterface.php b/core/lib/Drupal/Core/Form/FormBuilderInterface.php

diff --git a/core/lib/Drupal/Core/Form/FormBuilderInterface.php b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
index 463a7a2..bf00f50 100644

index 463a7a2..bf00f50 100644
--- a/core/lib/Drupal/Core/Form/FormBuilderInterface.php

--- a/core/lib/Drupal/Core/Form/FormBuilderInterface.php
+++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php

+++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
+++ b/core/lib/Drupal/Core/Form/FormBuilderInterface.php
@@ -48,7 +48,7 @@ public function getFormId($form_arg, &$form_state);

@@ -48,7 +48,7 @@ public function getFormId($form_arg, &$form_state);
    * @return array
    *   The form array.
    *
-   * @see \Drupal\Core\Form\FormBuilderInterface::buildForm()
+   * @see drupal_build_form()

+++ b/core/modules/views/includes/ajax.inc
@@ -10,8 +10,7 @@
- * Wrapper around \Drupal\Core\Form\FormBuilderInterface::buildForm() to handle
- * some AJAX stuff automatically.
+ * Wrapper around drupal_build_form to handle some AJAX stuff automatically.
  * This makes some assumptions about the client.
  */
 function views_ajax_form_wrapper($form_id, &$form_state) {
@@ -25,7 +24,7 @@ function views_ajax_form_wrapper($form_id, &$form_state) {

@@ -25,7 +24,7 @@ function views_ajax_form_wrapper($form_id, &$form_state) {
     ),
   );
 
-  $form = \Drupal::formBuilder()->buildForm($form_id, $form_state);
+  $form = drupal_build_form($form_id, $form_state);

Bad merge.

Just a bad patch (rolled against outdated HEAD), the merge is fine.

Here's a patch against the right base.

And with a couple more fixes from kgoel

Removing an @deprecated marker that didn't make sense.

This patch has really good test coverage, really nice!

On top of that there is a clear separation of concerns, compared to the earlier versions of the menu patches which had a lot backed into one gigantic class.

Let's burn the karma here, but I really can't find anything anymore.

Note for committers et al: pwolanin wrote MenuParentFormSelector(Interface) and MenuLinkTreeTest, I wrote pretty much everything else. dawehner only fixed his own nitpicks. Hence it's acceptable for dawehner to RTBC this.

Impressive stuff - really well documented and tested - just a couple of minor things I noticed.

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -0,0 +1,175 @@
   +/**
   + * Provides a couple of menu link tree manipulators.
   + */
   +class DefaultMenuLinkTreeManipulators {
   

   This seems a bit terse - lets explain what it provides.

2. +++ b/core/lib/Drupal/Core/Menu/MenuLinkTreeElement.php
   @@ -0,0 +1,128 @@
   +  public $link;
   ...
   +  public $subtree;
   ...
   +  public $depth;
   ...
   +  public $hasChildren;
   ...
   +  public $inActiveTrail;
   ...
   +  public $access;
   ...
   +  public $options = array();
   

   Allegedly there is a followup to convert (at the request of Dries) this to protected - and they are public cause of performance. However I think we should protect them first and look at performance implications later. Especially since the class does not make any use of some of these properties so we are just setting and using them from outside of the class.

3. +++ b/core/lib/Drupal/Core/Menu/MenuParentFormSelector.php
   @@ -0,0 +1,173 @@
   +class MenuParentFormSelector implements MenuParentFormSelectorInterface {
   +
   +  use StringTranslationTrait;
   

   According to the docs "If the class is capable of injecting services from the container, it should inject the 'string_translation' service and assign it to $this->stringTranslation."

4. +++ b/core/modules/system/src/Tests/Menu/MenuLinkTreeTest.php
   @@ -0,0 +1,140 @@
   +    \Drupal::service('router.builder')->rebuild();
   

   This at least needs a comment as to why we're doing this. Since other tests do not start with this.

5. +++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
   @@ -0,0 +1,251 @@
   +  protected function setUp() {
   +    $this->accessManager = $this->getMockBuilder('\Drupal\Core\Access\AccessManager')
   +      ->disableOriginalConstructor()->getMock();
   +    $this->currentUser = $this->getMock('Drupal\Core\Session\AccountInterface');
   +
   +    $this->defaultMenuTreeManipulators = new DefaultMenuLinkTreeManipulators($this->accessManager, $this->currentUser);
   +  }
   

   Missing a call to parent setUp

6. +++ b/core/tests/Drupal/Tests/Core/Menu/MenuActiveTrailTest.php
   @@ -0,0 +1,170 @@
   +  protected function setUp() {
   +    $this->requestStack = new RequestStack();
   +    $this->currentRouteMatch = new CurrentRouteMatch($this->requestStack);
   +    $this->menuLinkManager = $this->getMock('Drupal\Core\Menu\MenuLinkManagerInterface');
   +
   +    $this->menuActiveTrail = new MenuActiveTrail($this->menuLinkManager, $this->currentRouteMatch);
   +  }
   

   No call to parent::setUp()

#29:

1. Fixed.
2. These properties are public not for performance reasons. They're only public because this object is merely a simple value object. Before, this used to be an associative array. We basically want this to be the PHP equivalent of a C struct, to 1) ensure that all (and only) the expected key/value pairs exist and 2) ensure that each of the key/value pairs is fully documented (rather than the mess that we used to have). Adding getters and setters for each of these is utterly useless, and only creates unnecessary verbosity. I agree with the general rule of "ensure classes have interfaces and methods and protected properties, this helps when overriding things", but this is just a value object that will never be overridden. If that's still not sufficiently convincing, I'll fix this in a follow-up.
3. Fixed.
4. I don't know why this was there; pwolanin wrote this. Removing this still has all tests passing. So: removed :)
5. Fixed.
6. Fixed.

I totally agree with wim's comment and changes.

#32: Except that we need MenuLinkTreeElements to be mutable, because menu link tree manipulators need to be able to manipulate them (to set access check results etc.).

@effulgentsia - so we can skip most of the setters, but we need to be able to at least manipulate the subtree and access values. I was mistaken when I thought we could avoid that - we'd have to e.g. pass the manipulators into the load step, which would be a substantial refactoring.

Thanks for answering my questions - I can see why the decision to use public properties has been made this way - if we want to change this we can do this is a follow up.

Committed 50ac470 and pushed to 8.x. Thanks!

Awesome, thanks alexpott!

We were already working to address it anyway, to not impact our velocity too much. I've now posted that in a follow-up issue: #2304363: Make MenuLinkTreeElement's properties protected, add getters, add limited setters — in case you or Dries decide you want those properties to be protected after all.

Now we can work on #2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI! :)

Adding the critical regression as follow up

Tagging the child issues retroactively.