Problem/Motivation

Menu links map very well to the entity system, from CRUD operations to access control and rendering. As a huge bonus, we get fieldability support (not in this initial patch though) and the ability to deprecate *a lot* of contrib modules from the menu_* namespace.

Proposed resolution

Convert menu links to the entity system. In order to keep the initial conversion patch at a reasonable size, make the entity class implement ArrayAccess so we can keep most of the current code intact.

Remaining tasks

Review the latest patch.

User interface changes

None.

API changes

  • hook_menu_link_alter() is replaced by hook_menu_link_presave()
  • hook_translated_menu_link_alter() is replaced by hook_menu_link_load()

Probably others that don't come to mind right now.

Original report by Dave Reid

Modules need to be able to load menu links without worry about access (XML sitemap, token/pathauto). My initial workaround for this is in #333590-13: Node menu token fails (node grants not saved yet, menu access checking, oh my!) but I think menu links map very well to the entity system seeing as they could take advantage of entity caching and loaders (menu item = entity, menu that the item belongs to = bundle). We can do other stuff like abstracting the access checking of a menu link from its loading, just like we do with other entities.

Followup issues:
#1842850: Untangle menu_link access control from _menu_link_translate() and friends
#1842852: Move tree related code from menu.inc to the new Menu link module
#1842854: Move system menu block to core
#1842858: [PP-1] Convert menu links to the new Entity Field API
#1835902: Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)
#1805054: Cache localized, access filtered, URL resolved, and rendered menu trees

Related issues:
#1814916: Convert menus into entities
#1497380: Convert shortcut sets to ConfigEntity

CommentFileSizeAuthor
#179 menu-rebuild-performance-916388-179.patch2.25 KBBerdir
#165 916388-menu_link-165.patch189.62 KBamateescu
#165 interdiff.txt1.99 KBamateescu
#163 916388-menu_link-163.patch189.11 KBamateescu
#163 interdiff.txt16.25 KBamateescu
#160 916388-interdiff-160.txt533 bytesandypost
#160 916388-menu_link-160.patch186.93 KBandypost
#158 916388-interdiff-158.txt18.72 KBandypost
#158 916388-menu_link-158.patch186.97 KBandypost
#155 916388-interdiff-155.txt1.37 KBandypost
#155 916388-menu_link-155.patch187.46 KBandypost
#154 916388-interdiff-154.txt5.12 KBandypost
#154 916388-menu_link-154.patch186.6 KBandypost
#151 drupal-916388-151.patch185.96 KBdawehner
#149 drupal-916388-148.patch180.58 KBdawehner
#148 drupal-916388-148.patch180.58 KBdawehner
#141 drupal-916388-141.patch172.37 KBdawehner
#138 interdiff.txt10.86 KBpodarok
#138 916388-139.patch172.84 KBpodarok
#135 without133.png304.05 KBpodarok
#135 with133.png302.5 KBpodarok
#133 916388-menu-link-entity-134.patch172.88 KBpodarok
#132 916388-menu-link-entity-132.patch172.82 KBpodarok
#130 916388-menu-links-entities-130.patch119.4 KBpodarok
#128 916388-menu-links-entities-128.patch172.27 KBpodarok
#122 menu-links-entities-122.patch177.43 KBamateescu
#118 menu-links-entities-118.patch177.43 KBamateescu
#116 menu-links-entities-116.patch177.66 KBamateescu
#110 menu-links-entities-110.patch175.07 KBamateescu
#103 drupal-menu_links_entities-916388-103.patch167.12 KBdisasm
#99 menu-links-entities-99.patch174.9 KBamateescu
#88 menu-links-entities-88.patch174.43 KBamateescu
#88 interdiff-85-88-do-not-test.patch1.76 KBamateescu
#85 menu-links-entities-85.patch174.16 KBamateescu
#85 interdiff-74-85-do-not-test.patch4.44 KBamateescu
#82 router_rebuild_load.png237.61 KBBerdir
#74 menu-links-entities-74.patch173.83 KBamateescu
#74 interdiff-66-74-do-not-test.patch1.39 KBamateescu
#69 menu-links-entities-69.patch174.41 KBamateescu
#69 interdiff-66-69-do-not-test.patch1.46 KBamateescu
#66 menu-links-entities-66.patch172.95 KBamateescu
#62 menu-links-entities-62.patch278.71 KBamateescu
#62 interdiff-52-62-do-not-test.patch15.06 KBamateescu
#50 menu-links-entities.patch277.81 KBamateescu
#50 menu-links-entities_functional-changes-do-not-test.patch187.82 KBamateescu
#50 menu-links-entities_test_and_module_changes-do-not-test.patch89.99 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Here's some initial entity code I played with for D7:

/**
 * Controller class for menu links.
 *
 * This extends the DrupalDefaultEntityController class, adding required
 * special handling for menu link objects.
 */
class MenuLinkController extends DrupalDefaultEntityController {

  protected function buildQuery($ids, $conditions = array(), $revision_id = FALSE) {
    $query = parent::buildQuery($ids, $conditions, $revision_id);
    // Specify additional fields from the menu tables.
    $query->leftJoin('menu_router', 'm', 'base.router_path = m.path');
    $query->fields('m', array_diff(drupal_schema_fields_sql('menu_router'), array('weight')));
    return $query;
  }

  protected function attachLoad(&$menu_links, $revision_id = FALSE) {
    foreach ($menu_links as $mlid => &$item) {
      $item->options = unserialize($item->options);

      if ($item->external) {
        $item->access = TRUE;
        $map = array();
        $item->href = $item->link_path;
        $item->title = $item->link_title;
        $item->localized_options = $item->options;
      }
      else {
        $map = explode('/', $item->link_path);
        if (!empty($item->to_arg_functions)) {
          _menu_link_map_translate($map, $item->to_arg_functions);
        }
        $item->href = implode('/', $map);

        // Note - skip callbacks without real values for their arguments.
        if (strpos($item->href, '%') !== FALSE) {
          $item->access = FALSE;
        }

        // menu_tree_check_access() may set this ahead of time for links to nodes.
        if (!isset($item['access'])) {
          if (!empty($item['load_functions']) && !_menu_load_objects($item, $map)) {
            // An error occurred loading an object.
            $item['access'] = FALSE;
            return FALSE;
          }
          _menu_check_access($item, $map);
        }
        // For performance, don't localize a link the user can't access.
        if ($item['access']) {
          _menu_item_localize($item, $map, TRUE);
        }
      }

      // Allow other customizations - e.g. adding a page-specific query string to the
      // options array. For performance reasons we only invoke this hook if the link
      // has the 'alter' flag set in the options array.
      if (!empty($item->options['alter'])) {
        $item = (array) $item;
        drupal_alter('translated_menu_link', $item, $map);
        $item = (object) $item;
      }

    }
    parent::attachLoad($menu_links, $revision_id);
  }
}

/**
 * Implements hook_entity_info().
 */
function menu_entity_info() {
  $info['menu_link'] = array(
    'label' => t('Menu link'),
    'controller class' => 'MenuLinkController',
    'base table' => 'menu_links',
    'uri callback' => 'menu_link_uri',
    'label callback' => 'menu_link_label',
    'fieldable' => FALSE,
    'entity keys' => array(
      'id' => 'mlid',
      'bundle' => 'menu_name',
      'label' => 'title',
    ),
    'bundles' => array(),
    'view modes' => array(
    ),
    'load hook' => NULL,
  );

  foreach (menu_get_menus() as $type => $name) {
    $info['menu_link']['bundles'][$type] = array(
      'label' => t('@menu-name menu', array('@menu-name' => $name)),
      'admin' => array(
        'path' => 'admin/structure/menu/manage/%menu/edit',
        'bundle argument' => 4,
        'real path' => 'admin/structure/menu/manage/' . $type . '/edit',
        'access arguments' => array('administer menus'),
      ),
    );
  }

  return $info;
}

/**
 * Entity uri callback.
 */
function menu_link_uri($menu_link) {
  return array(
    'path' => $menu_link->link_path,
  );
}

/**
 * Entity label callback.
 */
function menu_link_label($menu_link) {
  retrurn $menu_link['title'];
}
Dave Reid’s picture

Issue tags: +Entity system, +token, +xmlsitemap

Tagging

Dave Reid’s picture

Plan:

1. Make a 'menu_link' and 'menu' entity types, the former provided by system module as it should always be available, the latter provided by menu.module.
2. Remove access control from load operations and convert it to menu_link_access()
3. Provide the full cruds associated with entities, including adding a menu_link_load_multiple() and menu_link_delete_multiple().
4. Provide tokens for menu links and menus in the respective system and menu modules.

Dave Reid’s picture

Title: Convert menu links into entities so we can separate loading from access » Convert menu links and menus into entities so we can separate loading from access
mattyoung’s picture

Would this make it possible to have "non-secret" restrict access menu? Eg: I want to show the menu link to all users, but require them to have acces right to actually gain access.

We should have two permission: one the current one. Another to control visibility of the menu.

Dave Reid’s picture

joachim’s picture

Sounds good, and would simplify things like adding icons to menus.

> (menu item = entity, menu that the item belongs to = bundle)

How would moving a menu item from one menu to another work with that?

dropcube’s picture

Subscribing.

Would like to see how this could work with http://drupal.org/project/entity_translation to make menu links entities translatable (i18n_menu module of http://drupal.org/project/i18n package).

casey’s picture

+1

gaspaio’s picture

+1

catch’s picture

Subscribing, this keeps coming up in conversation, I think it makes sense but it will also be tricky.

jherencia’s picture

Subscribing...

Gábor Hojtsy’s picture

Issue tags: +D8MI

Tagging for Drupal 8 multilingual initiative. Re: http://groups.drupal.org/node/155634

plach’s picture

mrsinguyen’s picture

Subscribing

zkday’s picture

Subscribing

dtarc’s picture

subscribing

sun’s picture

While I agree the idea makes sense, this will (vastly) decrease performance. Tough challenge.

catch’s picture

Yeah menu links are already a serious performance issue - we already have a per-page cache per menu, and separate lookup keys to avoid duplicate cache entries. Cache misses are three queries plus two cache_set() per menu as well.

However if we were able to simplify that somehow to at least cache some information site-wide instead of per page (haven't figured out how yet), it might outweigh the change here.

omercioglu’s picture

Subscribing

marcvangend’s picture

On a side note: If menu links become entities, would that mean that we can make them fieldable and create a more flexible alternative for modules like Menu attributes?

Gábor Hojtsy’s picture

Title: Convert menu links and menus into entities so we can separate loading from access » Convert menu links and menus into entities

Any takers?

Xano’s picture

In response to #3: we have menu router items (mapping URLs to callbacks, access control, etc) and menu links (the actual clickable links. They overlap, but not fully. Which one do you suggest we convert to an entity type?

sun’s picture

Menu router items are not entities.

Aside from that, I'd suggest splitting either menu links or menus into an own issue. The latter can be implemented much more easily than the former.

casey’s picture

I have some code in my sandbox splitting menu.inc into three parts moving anything related to menus into the menu module: http://drupal.org/sandbox/casey/1322008

Far from working though.

Gábor Hojtsy’s picture

@casey: Great! What about starting with converting menus themselves to entities? That sounds like would be the easier one, instead of trying to tackle both parallel.

Dave Reid’s picture

It would probably be best to start with converting menu links. I'm not so sure we should convert menus themselves into entities as they're actually bundles with machine names.

Gábor Hojtsy’s picture

Feedback from my blog post on menu translation (from Quentin DELANCE):

It makes sense for menu items to be entities IMO

From my point of view (Drupal user, admin point of view, I am not a developer), it would make sense as we usualy need to enhance default menu entries with things like images, extra properties to make clickable or not, appear in site map or not and so on.

Actually we tend to use taxonomies when we need to build a complex menu system, as they are fieldables...

Gábor Hojtsy’s picture

Issue tags: +language-content

Tagging for the content handling leg of D8MI.

gdd’s picture

I just want to throw my hat in the ring of people that agree that menu links should probably become entities, but that we should leave everything else as is. We should probably get crell in on this issue, since he is planning all sorts of changes to the menu system as a part of the WSCCI initiative.

Crell’s picture

Issue tags: +WSCCI

Tagging. I'll be posting a writeup of our plans for the routing layer coming out of DrupalCon very shortly, which I'll then link here with more commentary. Short version: Router items are config, not entities, and WSCCI is moving them out of hook_menu. Links are probably entities, although I don't know if they're fieldable. The other things hook_menu does just suck. :-)

andypost’s picture

Menu links could be fieldable to make items into rich-snippets of content like menu_icons module does.

Gábor Hojtsy’s picture

If menu items are neither converted to entities neither to config, they will not be translatable in Drupal 8.

rudiedirkx’s picture

It would probably be best to start with converting menu links.

I agree. More important (or at least more useful) to have fieldable and translatable menu items. Obviously they have to be translatable in D8. More easily than in D7 even.

I wouldn't know where to start though... Clone D8 and rewrite menu.module?

Gábor Hojtsy’s picture

@rudiedirkx: theoretically just making the menu item data entities could be possible in smaller steps, with converting description, etc. to fields as next steps.

pounard’s picture

Field API is today quite slow, even in read operations, aren't you afraid that menu could suffer from this? I really don't know just asking here, a lot of data of the menu API is actually cached, but maybe it could become very heavy when, for example, building menu blocks?

skwashd’s picture

One of my colleagues has worked on this as a contrib module for D7. His sandbox is at https://drupal.org/sandbox/jamesharv/1749006 Unfortunately he forgot to push his code to it. I have a github repo for the code at https://github.com/skwashd/entity_menu_links so it can be reviewed.

pwolanin’s picture

From discussion at Drupalcon:

Simply convert the existing menu links table into the base table for an entity - keep the hierarchy implementation contained within the entity - need to figure out how to handle the book code.

Step 2 would be to split out the hierarchy handling into a separate subsystem - through that interface you should be able to load a set of entity IDs for a particular hierarchy. Note that their are some gotcha's here around the "expanded" functionality in the current menu links.

donquixote and ronald_istos said they would work on this, but they need to to connect up.

donquixote’s picture

donquixote and ronald_istos said they would work on this, but they need to to connect up.

That's the difficult part :(
I would love to hear his input and status.

So, here is some more thoughts, after talking with Crell, Dave Reid, and Peter Wolanin:

  • Menu links = entities: Generally a good idea, and we all mostly agree. I personally don't feel it to be as urgent, I see a higher priority on decoupling and refactoring the code, than making it entities.
    I think the strongest argument is in #33, making them translatable (although I was not aware that w/o entity or conf we can't translate anything). This requires the title to be a field or a translatable property (as I understand, it is still quite vague what entity "property" will mean vs "field")
  • Menu = bundle? I personally disagree, and others mostly agreed with my concerns after some talking.
    Why?
    • Some sites have one menu per language, to allow having different menu structure (beyond just title translation) per language. These would result in a ton of bundles, totally overhead.
    • Some menus might want more than one bundle for the items. E.g. the top level could be the default bundle, while level 1 would have the bundle with the fancy icons.
    • Most sites will be happy with just one bundle for all menu items.
    • As mentioned by others, we'd have to rethink how items can be moved around between menus.

    So, the plan would be, to have one default bundle that can be used for all items until someone wants a different item. We could even leave it to contrib to define other bundle types.

  • Fields on menu items? The tendency was to leave it to contrib to make menu item entities fieldable.
  • Menu = entity? We did not really discuss this in depth, but didn't seem to be a strong argument why we would absolutely need this. Personally I would rather see them as configuration items, so they can more easily be targeted with CSS, and with php code.

Unclear: Where to use field API

  • Link data = field or property?
    Talking with Crell, it seems there are three types of data on an entity in D8, but this is not carved in stone yet:
    1. Fields defined in configuration. These can be user-defined or at least overridden by a user, they can be translated, they use field API.
    2. "Properties", hardcoded. These can not be overridden by the user, but they might still use field types defined with field API (this is actually unclear), and they can be translated.
    3. Hardcoded data that can not be translated and does not use field API.

    This is what I was able to pull from conversations. Apparently this stuff is still being worked on.
    For the title we'd probably use (1) or (2).
    The link usually does not need to be translated, but it could still be interesting to have it use field API.
    The parent item reference should probably not be translated, and performce is a big deal here.

    So far the general idea is to start with everything hardcoded, and decide later if we move any of that to field API.
    Any information on this is welcome.

Remotely related stuff:

  • Menu rebuild. We (mostly me and Crell) did also talk about how the automatic building of the e.g. navigation or admin menu should work in the future. The idea here is that menu_rebuild should be cleaned up, that a diff-based rebuild would be sexy, and that the rebuild should fire a hook that menu links can listen to, instead of explicitly calling the navigation links rebuild.
  • Code refactoring. A lot of menu links code needs to be refactored and rewritten, and moved out of for tree loading, saving and everything. I have some contrib code that we can partially base this on, but probably a lot will have to be rewritten again. Just we should do this in steps.
  • menu links = optional One major goal is that we want to be able to switch off menu links at any time, or to have more than one menu links implementation coexist peacefully. So actually we could have one menu_links_entity module, and one menu_links_plain module, that coexist.
nevergone’s picture

In my opinion, the menu and menu items is entities. That possible both connent data, and menu items properties the menu name. The optional data is field, becasuse good extendable and scalable.

Crell’s picture

One correction to #39: There is no "hard coded non-field data" on Entities in the current entity roadmap. None. Any and all such data is unsupported and should not be used, ever.

I believe as fago is currently using the terms:
Property: A compound data element on an Entity (basically what we call "Fields" in Drupal 7).
Field: A special case of a Property that is user-added rather than defined by the module that declares the Entity type.

Regarding incremental rebuild, there's a related issue: #1446808: Let plugin managers announce changed definitions

ronald_istos’s picture

Here is a summary + possible actions moving forward to provide some focus.

1. Everyone agrees menu links should be entities. Jury is out on Menus => Let us do menu links to start with and that will inform subsequent thought. We can, nevertheless, say that the original idea of menus as bundles of menu links is off the table.

2. There is some code to look at - probably not directly usable but a start. #25 and #37 talk about that.

3. #19 mentions performance concerns - that we really need to figure out.

Next step for me will be to look at existing code and outline a strategy with some sort of patch about how to simply turn menu links into entities. To do that I will need to:
1. Figure out how to built Entities in Drupal 8 - i.e. what has changed from 7 (Crell's comment on Properties and Fields above is *very* confusing).
2. Do a very straightforward conversion of menu links into Entities and progressively enhance that.

gdd’s picture

Issue tags: +Configuration system

Note that this has impact on CMI, because if menu links don't become entities then they need to come into CMI, otherwise they won't be deployable.

Crell’s picture

Ronald: Go for it. The terminology that fago is using is a bit different than what we discussed back in Paris, and I don't know why he switched. :-) I agree, it's confusing. Check with him if you're still confused. He has a large-ish patch in a sandbox branch that is trying to implement the new model we designed. I'd jump straight to that API if you can.

donquixote’s picture

The consequence of #41 would be that all storage-related stuff has to be defined as a "field type" (or "property type", *) first, and only then wired up with the entity type (but be equally available for any other entity type).

The question would be, if the "field type" only defines the basic storage, or also all logic associated with it. E.g. there would be a field type "tree parent reference, (un)ordered", with all the tree loading logic, additional storage for materialized path etc, which would then be available for any entity type.
When I suggested that on DrupalCon Munich, the response was that we rather want to hardcode this logic into the entity type as a first step (I think it was Peter who said that).

(*) If we distinguish "field type" and "property type", then the difference would be that for a "property type" we can skip the field instance configuration UI.

Crell’s picture

Priority: Normal » Critical

This is important for both WSCCI and i18n initiatives, and is a blocker for eliminating hook_menu(). Bumping to critical per discussion with Shannon.

amateescu is working on this, but hasn't posted anything yet. Andrei, can you at least post an interim patch so we know things are happening? :-)

tim.plunkett’s picture

Issue tags: +Configurables

Tagging.

andypost’s picture

Title: Convert menu links and menus into entities » [Meta] Convert menu links and menus into entities

Yeah it really interesting how this goes - I'd recommend to make this issue as META and made a follow-ups

- Convert menu to Config entity like imageStyle now
- Convert menu item to Entity like File and User now

Working on #1552396: Convert vocabularies into configuration & #1588422: Convert contact categories to configuration system I suggest to make separate issues for
- Conversion to entity, tests
- Moving forms to controllers, tests
- Implementing Entity List Controllers, tests

This would help to make patches more reviewable and probably could speed-up process

andypost’s picture

Also module shortcut needs a some changes to work with new menu-links #1304486-17: Completely remove the ability to limit the number of shortcuts per set

EDIT Related #1801304: Add Entity reference field

amateescu’s picture

Title: [Meta] Convert menu links and menus into entities » Convert menu links into entities
Assigned: Dave Reid » amateescu
Status: Active » Needs review
Issue tags: -Configuration system, -Configurables +Needs architectural review
FileSize
89.99 KB
187.82 KB
277.81 KB

First of all, sorry for stashing this work in the wscci sandbox for so long :/

I tried to keep this patch as small as possible but it's still a huge change to review because menu links are *everywhere*.

This is the list of things that I left out from this patch and would like to keep for follow-ups:

  • move the tree code from menu.inc to the new menu_links.module, thus removing the dependency between them
  • bring over menu_link related stuff from the System module (e.g. system_admin_menu_block*, pretty much all the tests, etc.), thus making menu_link an unrequired module
  • rename the table to menu_link and refactor it to use the new multilingual schema and entity field api, thus killing localized_options!
  • clean-up the Shortcut module

And the list of things still to do in the current patch:

  • fix the remaining tests
  • convert book.module to the new entity
  • finish the docs
  • performance testing

Now, on to the patch(es):

  • the first one is the big thing, open it only if you're feeling adventurous :)
  • the second is actually the one that can be reviewed
  • the third contains only changes related to tests, shortcut, toolbar, aggregator, book, user and node

Let's keep this issue focused only on menu links and open a new one for converting menus to config.

Fingers crossed for finding reviewers :D

Status: Needs review » Needs work

The last submitted patch, menu-links-entities.patch, failed testing.

chx’s picture

First: amazing work. Really, I am astonished how far you got. Mostly small problems:

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -0,0 +1,221 @@
+    $path = $menu_link->link_path;

Can we add a comment on why don't we use url() here? (because it'd add base_path)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,606 @@
+//      db_update($this->entityInfo['base table'])

Hrm, why is this commented? Can we remove it or is something missing?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,606 @@
+        $candidates[] = $entity->{"p$depth"};

{"p$depth"} is very hard to read IMO, can we $property = "p$depth"; $entity->$property instead?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,606 @@
+        if ($new_query->count()->execute() == 1) {
+          $new_query->count = FALSE;
+          $result = $new_query->execute();

Even if this happens to work with the current EFQ (I am btw shocked if it does) you never should re-execute() the same query twice, clone before count()->execute() thanks.

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/TreeOutputTest.phpundefined
@@ -51,6 +35,25 @@ function setUp() {
+        'link' => new MenuLink(array('menu_name' => 'main-menu', 'mlid' => 1, 'hidden' => 0, 'has_children' => 1, 'title' => 'Item 1', 'in_active_trail' => 1, 'access' => 1, 'href' => 'a', 'localized_options' => array('attributes' => array('title' =>''))), 'menu_link'),

*blink* multiple lines? Loops? The whole tree buildup here is very hard to read.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLink.phpundefined
@@ -0,0 +1,355 @@
+  public function access(\Drupal\user\User $account = NULL) {

This needs a use Drupal\User\User. We only do the \foo syntax for non-namespaced classes.

+++ b/core/modules/system/system.admin.incundefined
@@ -18,33 +18,39 @@ function system_admin_config_page() {
+        $block['content'] .= theme('admin_block_content', array('content' => system_admin_menu_block($menu_link->router_item)));

I know this comes from the old code but can we slap a TODO here to make this a render array? I dislike inline theme() calls :/ I know render arrays are on their way out but still, structured output and all that. If this is not feasible with a block content then nevermind.

+++ b/core/includes/common.incundefined
@@ -6191,6 +6189,31 @@ function drupal_sort_title($a, $b) {
+  $labelb = $b->label();

label_a and label_b perhaps?

+++ b/core/includes/menu.incundefined
@@ -686,9 +678,10 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
+  // @todo Take care of 'localized_options' later.

Is this still a @todo? I saw localized options below albeit it was marked a "hack".

+++ b/core/includes/menu.incundefined
@@ -1101,8 +1097,8 @@ function menu_tree_all_data($menu_name, $link = NULL, $max_depth = NULL) {
+          if (!empty($link->{"p$i"})) {

Can we put p$i this in a variable? Please? My eyes are bleeding.

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -481,7 +481,7 @@ function aggregator_save_category($edit) {
+  if (isset($op) && module_exists('menu_link')) {
     menu_link_maintain('aggregator', $op, $link_path, $edit['title']);

This can be put into a module_invoke('menu_link', 'maintain'.... it will do nothing if menu_link does not exist.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/CategorizeFeedItemTest.phpundefined
@@ -32,7 +32,7 @@ function testCategorizeFeedItem() {
+    $menu_link = entity_load_multiple_by_properties('menu_link', array('link_path' => $link_path));

Small semantics and it doesn't change the test but it's not menu_link it's menu_links because you are loading multiple ones. Important only if someone uses this as an example.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLink.phpundefined
@@ -0,0 +1,355 @@
+  public $p1;

I would love to see a TODO / followup to study whether it'd be possible to move these p1,p2... columns into a single array property.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLink.phpundefined
@@ -0,0 +1,355 @@
+  public $access;

So many public properties. I understand that' because of the straight port, of course. Followup perhaps to make them protected, move more logic in here from menu.inc etc?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,606 @@
+      if ($entity->isNew() || (array_intersect_key((array) $entity, (array) $entity->original) != (array) $entity->original)) {

You want get_object_vars() here. I am surprised array casting even works. It's absolutely not guaranteed it will work on future PHP versions.

andypost’s picture

Shortcut part probably would have conflicts with #1497380-7: Convert shortcut sets to ConfigEntity

chx’s picture

One more thing which I partially touched already: I see #50 mentioning that later we will let the D8 APIs deal with localized options which is fine but please do not leave in commented queries. We have git to get old code out from if necessary.

ardas’s picture

This is a great and correct idea - we need a powerful mechanism to extend menu item with different fields. No need to make menu items NOT fieldable... I don't understand why they made fieldable = FALSE !!!

Drupal core should handle all data extending, so that the same storage mechanism is utilised ... and there will be no need to develop different modules to extend menu items.

Fields will also solve a problem of menu item icons... just add an image field and override menu item theme function - it is DONE!

+ 1000000 for fieldable=TRUE

podarok’s picture

looks like we need a lot of speed testing here
#55 agree with that, but if we make menu links as fildable possib;y it will be a huge performance impact

pounard’s picture

Not necessarily as long as entity can be cached. Right now they are not until #1596472: Replace hard coded static cache of entities with cache backends happens, but we have to consider this a solution if menu item fields cause problems.

podarok’s picture

#57 cached entities can be performance impact too
cause if we have 20+ K menu links (bundles as in this patch) unserialize and serialize from/to cache may be a trouble too like in #1040790: _field_info_collate_fields() memory usage

catch’s picture

See #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees for caching. Short version, we need to move from caching menu tree arrays, to caching rendered links. If we can do that then the bulk of requests will have (much) better performance than they do now.

podarok’s picture

#59 thanks... following

andypost’s picture

Filed #1814916: Convert menus into entities
updated summary with related links

amateescu’s picture

Status: Needs work » Needs review
FileSize
15.06 KB
278.71 KB

Adressed all the comments from #52 and fixed a few more tests.

About the remaining failures:
- book.module is out of this world, I don't know what we're going to do with it
- I have a feeling that TrailTest is going to give me a very hard time, probably I will end up converting access, href and all the new public properties to protected. Actually, this is what I did initially but converted them to public in order to keep the patch size down :(

Status: Needs review » Needs work

The last submitted patch, menu-links-entities-62.patch, failed testing.

markhalliwell’s picture

Just saw this issue, but also referencing #1287610: Create a Menu Item Type API.

I really like the idea of converting menu links into entities. Although, I also feel there should be a terminology shift from "menu links" into "menu items". Rendered menu items could technically be anything: link (menu, which would use the menu router), plain text or separator (special_menu_items), or a view (menu_views).

fago’s picture

+    menu_cache_clear($entity->menu_name);
+    if (isset($entity->original) && $entity->menu_name != $entity->original->menu_name) {
+      menu_cache_clear($entity->original->menu_name);
+    }
+
+    // Now clear the cache.
+    _menu_clear_page_cache();

I'm wondering whether it makes sense to have that in resetCache() ? That way we'd have a single place that clears all caches related to the entities.

amateescu’s picture

FileSize
172.95 KB

I'm wondering whether it makes sense to have that in resetCache() ? That way we'd have a single place that clears all caches related to the entities.

I'm not sure, but caching of menu links will need to be revisited anyway so I'd prefer to leave that investigation for a follow-up.

It's time for an update here :) I was right about TrailTest, it did gave a very hard time (a few days worth). Investigating that led me from one rabbit hole to another because every usage of menu links is so tied up (leaked-in router item properties, breadcrumbs, active links, etc..) that I was actually starting to turn the current (old) router system to OO.

So I decided to take another approach for now, use ArrayAccess and deal with cleanups properly in followup issues. This reduced the patch size by 100K and hopefully made it a lot easier to review.

amateescu’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, menu-links-entities-66.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.46 KB
174.41 KB

Let's try to move the registration of entity.query to bootstrap.inc, as suggested by @tim.plunkett.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -token, -xmlsitemap, -WSCCI, -D8MI, -language-content

The last submitted patch, menu-links-entities-69.patch, failed testing.

nevergone’s picture

Status: Needs work » Needs review

#69: menu-links-entities-69.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +token, +xmlsitemap, +WSCCI, +D8MI, +language-content

The last submitted patch, menu-links-entities-69.patch, failed testing.

Gábor Hojtsy’s picture

This issue is still a huge enabler for the multilingual initiative to make menu items translatable. Keep up the great work! :)

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
173.83 KB

Fixed the remaining test failures, I just needed to add the entity query factories to the container used by update.php :)

Berdir’s picture

Yay, green!

Some random feedback....

+++ b/core/includes/menu.incundefined
@@ -5,8 +5,11 @@
 
+use Drupal\Component\Utility\NestedArray;
 use Drupal\Core\Cache\CacheBackendInterface;
 use Drupal\Core\Template\Attribute;
+use Drupal\menu_link\Plugin\Core\Entity\MenuLink;
+use Drupal\menu_link\MenuLinkStorageController;
 
 /**

We discussed this before :) Just thinking out loud...

- using modules in an includes file is really not what we want
- but the plan is that menu.inc will be completely replaced by menu_link and the new router system and menu links will eventually become an optional module then, right?
- Not sure if that will happen for 8.x, though.
- Now that entity types are plugins, it's actually possible to define entity types as a component, I think.
- It might be a possibility to keep the functional stuff in a menu_link.inc for the moment and move it to a module once we're that far.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -0,0 +1,227 @@
+/**
+ * Form controller for the node edit forms.
+ */
+class MenuLinkFormController extends EntityFormController {

Wondering if we can shorten these classes a bit. For example, the taxonomy_term classes are just called Term*, as they're within the taxonomy namespace.

Link* on the other hand is rather misleading, so MenuLink probably does make sense.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,286 @@
+ */
+class MenuLink extends Entity implements \ArrayAccess, ContentEntityInterface {

Is a menu link really content? I don't think any more than a user and a user is currently not :)

+++ b/core/modules/menu_link/menu_link.infoundefined
@@ -0,0 +1,8 @@
+description = Provides menu links, trees and bunnies!
+package = Core
+version = VERSION
+core = 8.x
+; @todo Menu links functionality has been moved from system.module and menu.inc
+; to this module, so make it required until everything is moved over.
+required = TRUE

What would happen if you were to disable menu_link?

Sounds to me like people could really mess up their site by identically disabling this. Just like we currently have a hardcoded fallback if you disable block.module to make the main block still available.

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -0,0 +1,189 @@
+/**
+ * Loads a menu link entity.
+ *
+ * This function should never be called from within node_load() or any other
+ * function used as a menu object load function since an infinite recursion may
+ * occur.
+ *
+ * @param int $mlid
+ *   The menu link ID.
+ * @param bool $reset
+ *   (optional) Whether to reset the menu_link_load_multiple() cache.
+ *
+ * @return \Drupal\menu_link\Plugin\Core\Entity\MenuLink|false
+ *   A menu link entity.
+ */
+function menu_link_load($mlid = NULL, $reset = FALSE) {
+  return entity_load('menu_link', $mlid, $reset);

The plan is to remove most of the per-entity-type specific wrapper functions and directly use entity_*(). Maybe we can leave them out from the beginning here?

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -0,0 +1,189 @@
+/**
+ * Saves a menu link.
+ *
+ * After calling this function, rebuild the menu cache using
+ * menu_cache_clear_all().
+ *
+ * @param \Drupal\menu_link\Plugin\Core\Entity\MenuLink $menu_link
+ *   The menu link entity to be saved.
+ */
+function menu_link_save(MenuLink $menu_link) {
+  $menu_link->save();

This one for example is certainly useless :)

+++ b/core/modules/system/system.installundefined
@@ -2209,6 +2023,51 @@ function system_update_8034() {
+  // Add the langcode column.
+  $column = array(
+   'description' => 'The {language}.langcode of this entity.',
+   'type' => 'varchar',
+   'length' => 12,
+   'not null' => TRUE,
+   'default' => '',
+  );
+  db_add_field('menu_links', 'langcode', $column);

Right now in 7.x, this is added by i18n_menu (I think?). Because core then doesn't know about it, the preferred link/breadcrumb generation doesn't the language into account, so i18n_menu() needs to implement hook_init() and inject the static cache, se http://api.drupalize.me/api/drupal/function/i18n_menu_init/7. I *think* there is a hook now, that allows easier access to this, but you still have to duplicate the whole query.

Did you already take care of that in this issue, should we do it here or in a follow-up issue (would be totally fine with me but should probably be major as we should really really do that).

+++ b/core/modules/system/system.installundefined
@@ -2209,6 +2023,51 @@ function system_update_8034() {
+  // Add the 'serialize' property to the options column.
+  $column = array(
+   'description' => 'A serialized array of options to be passed to the url() or l() function, such as a query string or HTML attributes.',
+   'type' => 'blob',
+   'not null' => FALSE,
+   'translatable' => TRUE,
+   'serialize' => TRUE,
+  );
+  db_change_field('menu_links', 'options', 'options', $column);

Is this actually necessary?

Right now, I think the serialize flag does not actually have any effect on the database side, it's only used by drupal_write_record() to automatically serialize().

moshe weitzman’s picture

Just want to reemphasize that we have a 'needs benchmarks' tag on this issue and we should remember to follow through on it. If this patch is only a small regression, maybe we say it is OK if we also get a patch to disable menu link building entirely (donquixote has worked on this).

moshe weitzman’s picture

Issue summary: View changes

Updated issue summary.

andypost’s picture

to keep the functional stuff in a menu_link.inc for the moment and move it to a module once we're that far.

Converting menus itself in #1814916: Convert menus into entities
I came to similar dilemma - the menu.module should manage custom menus and probably their items.
Menu items also used all over a core (mostly their forms to add/edit as sub-forms).
Shortcut module creates a menu-items with own "menu_name-bundle" so just manages same custom menus just prefixing it's machine name and provides a block.

So it seems that Menu and MenuItem should be defined in system.module as Core entities and menu module should be just a UI module - to manage what?

Crell’s picture

Please don't define anything in system module if you can possibly avoid it. :-/ That thing is an abomination of circular dependencies that needs to die. That it wasn't executed this dev cycle is a major regret.

tim.plunkett’s picture

Annotated classes (plugins) need not be provided by a module. They can just live in \Drupal\Core\Plugin\Core\Entity

amateescu’s picture

Re #76: Wouldn't have done it otherwise :) Here's some performance testing done on my local machine.

Anonymous user

  • Frontpage with just the standard links
Before:

Total Incl. Wall Time (microsec):	126,941 microsecs
Total Incl. CPU (microsecs):            124,801 microsecs
Total Incl. MemUse (bytes):	        2,984,520 bytes
Total Incl. PeakMemUse (bytes):         3,003,768 bytes
Number of function xhprof_Calls:	14,042

After:

Total Incl. Wall Time (microsec):	131,325 microsecs
Total Incl. CPU (microsecs):            109,201 microsecs
Total Incl. MemUse (bytes):	        3,039,552 bytes
Total Incl. PeakMemUse (bytes):         3,058,832 bytes
Number of function xhprof_Calls:	14,578
  • Frontpage with 50 menu links generated with devel and all of them displayed
Before:

Total Incl. Wall Time (microsec):	157,640 microsecs
Total Incl. CPU (microsecs):            156,002 microsecs
Total Incl. MemUse (bytes):	        3,539,336 bytes
Total Incl. PeakMemUse (bytes):         3,561,688 bytes
Number of function xhprof_Calls:	18,967

After:

Total Incl. Wall Time (microsec):	169,696 microsecs
Total Incl. CPU (microsecs):            156,001 microsecs
Total Incl. MemUse (bytes):	        3,541,616 bytes
Total Incl. PeakMemUse (bytes):         3,563,936 bytes
Number of function xhprof_Calls:	20,723

Logged in as uid 1

  • Frontpage with just the standard links
Before:

Total Incl. Wall Time (microsec):	166,929 microsecs
Total Incl. CPU (microsecs):            171,602 microsecs
Total Incl. MemUse (bytes):	        3,291,136 bytes
Total Incl. PeakMemUse (bytes):         3,325,168 bytes
Number of function xhprof_Calls:	19,082

After:

Total Incl. Wall Time (microsec):	190,381 microsecs
Total Incl. CPU (microsecs):            202,802 microsecs
Total Incl. MemUse (bytes):	        3,732,256 bytes
Total Incl. PeakMemUse (bytes):         3,760,256 bytes
Number of function xhprof_Calls:	22,147
  • Frontpage with 50 menu links generated with devel and all of them displayed
Before:

Total Incl. Wall Time (microsec):	195,626 microsecs
Total Incl. CPU (microsecs):            187,202 microsecs
Total Incl. MemUse (bytes):	        3,829,920 bytes
Total Incl. PeakMemUse (bytes):         3,862,072 bytes
Number of function xhprof_Calls:	23,444

After:

Total Incl. Wall Time (microsec):	226,778 microsecs
Total Incl. CPU (microsecs):            234,002 microsecs
Total Incl. MemUse (bytes):	        4,235,128 bytes
Total Incl. PeakMemUse (bytes):         4,261,416 bytes
Number of function xhprof_Calls:	27,880

All tests were done with the standard profile and a warm cache.

Berdir’s picture

Looked into this a bit with xhprof.

First: I installed this and HEAD in parallel and the installation with this patch seems to have taken a considerable longer amount. Comparing the test run time of this and patches on the same bot confirmed this. This patch took 1h10m, compared to ~47min of other patches. That's a *huge* difference.

On pages with a warm cache, the differences vary but there aren't many large ones. Some things that I noticed:

- 14 additional calls to the classloader, which is actually the biggest difference. My guess is that this is actually because we're using EFQ on the frontpage? Some sort of caching might help here.
- Looks like a considerable amount of the additional time and especially those function calls are spent in the offset*() methods for the BC layer. That should just go away, so this should be come irrelevant
- ~3 more queries which are executed currently.

Tried to get some comparable results on a first call to a cold cache (truncated cache, cache_bootstrap, cache_config and cache_menu and deleted the dumped container). The total runtimes were relatively close (580 to 600ms), so I might be missing something, not sure what needs to be emptied as well, maybe a menu rebuild? Some things that I noticed:

- ~7 more queries, 9 more Select::__toString() (subquery?)
- Some more Tokenizer parser calls due to the new entity type
- A bunch of smaller things, mostly related to the two above (more queries and more annotation parsing)

Ok, so let's check menu_router_rebuild(). And now it gets interesting. I'm seeing a *huge* difference there. The 800ms (HEAD) vs 7s kind of difference. So something is obviously going wrong here.

- 900 vs 5000 queries.
- 50% of the page request is spent within Drupal\Core\Entity\DatabaseStorageController::load(). See attached screenshot.
- Those loads are coming from loadByProperties(), updateParentalStatus, entity_load_unchanged() and findParent() and a few entity_load_multiple().
- Additionally, there are 580 EFQ queries
- Also interesting: 300 calls to menu_cache_clear(), each of them resulting in a invalidateTags()/Merge Query

You can reproduce this easily by adding a call to menu_router_rebuild(). I'm not sure what exactly will become of this once the existing menu system is replaced completely, I assume things might get better as router rebuild won't mean menu item rebuild, but I have no idea what exactly the plan is with menu items. But we need to improve this quite a bit I think, to improve testrun time, if nothing else.

Berdir’s picture

FileSize
237.61 KB

Forgot the screenshot.

Also, corrected something above. It's a token parser, not a reflection parser, it's just the class that's called StaticReflectionParser.

donquixote’s picture

About menu_router_rebuild() / _menu_navigation_links_rebuild():
I am working on a solution to speed that up.
#1830274: Support a parent_path setting in hook_menu(). / hook_menu_link_alter() should not touch the plid.

This is all based on the traditional menu links system, and I would like to get that in first, to make it backportable to D7.
However, the idea should work as well for a new system.

Idea of the algorithm:
- No expensive calls to menu_link_save() and friends, and use dedicated methods instead.
- One controlled flow, where everything is touched only once, starting at root level, and moving down from there.

This requires the API change mentioned in the linked issue title, but I think that would be quite acceptable for D8 at least.
(and besides mentioning it here, I am ok if you guys leave me tinkering on that one until I have something presentable)

donquixote’s picture

#76 (Moshe)

maybe we say it is OK if we also get a patch to disable menu link building entirely (donquixote has worked on this).

That would be this one, I guess
#1170278: Introduce hook_menu_router_update()

amateescu’s picture

Here's a patch that restores some sanity to _menu_navigation_links_rebuild(). It's still ~3x slower than HEAD but ~7x better than #74 :)

Note to self: I still have to respond to the review from #75.

David_Rothstein’s picture

Priority: Critical » Major

This is an important issue, but I don't believe it's a critical task (see http://drupal.org/node/45111 and http://drupal.org/node/1181250). Specifically, there are ways to translate menus without this, and removing hook_menu() in favor of other methods is not something that must be done to release Drupal 8...

If you disagree and feel this must still be marked critical, please provide a very specific justification and keep in mind that every critical issue significantly prevents other features that people are working hard on from getting into Drupal core, due to the issue queue threshold policy (until/unless #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw is addressed, at any rate). Note that I am not singling out this issue for particular attention but rather (slowly) trying to go through the issue queue and do this in many places so that other features still have a chance to get into Drupal core soon without the thresholds getting in the way. I am trying my best to be impartial, and of course, just because an issue isn't marked "critical" certainly doesn't mean it's unimportant to work on. You can follow my progress via the tag I've just added to this issue :)

Status: Needs review » Needs work

The last submitted patch, menu-links-entities-85.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
174.43 KB

This should fix the exceptions.

catch’s picture

I'm fairly sure that hook_menu() won't even work once the router-ish bits have been moved to the new router system - not without significant refactoring, of which this patch is the first step towards properly separating them out. We might want a critical meta issueto track the various pieces of this and have this as a sub-issue though.

Crell’s picture

Related, for the other bits of hook_menu that we haven't found a new home for yet: #1835902: Allow modules to register links for menus, breadcrumbs and tabs (if not with hook_menu)

amateescu’s picture

And finally a response to #75.

About creating a new module vs menu_link.inc, this was discussed on IRC and I remember the conclusion was that a new module (required for now but optional in the end) is cleaner because we also need to provide the form and storage controllers.
Anyway, as I mentioned in #50 and catch said in #89, this is just the first step, there are many things left to do to properly separate menu links from old router items.

Is a menu link really content? I don't think any more than a user and a user is currently not :)

The end goal is to make them fieldable, so yes, content entity for now. If we decide otherwise until code freeze, it's a trivial change :)

What would happen if you were to disable menu_link?

Can you disable a required module? :-O I guess it would certainly break your site, though not more than disabling system.module...
Or if you meant disabling it *after* it becomes optional, well, that's a different story. You can do that either because you have a custom module that provides navigation-like functionality, or you just don't need it because you are using Drupal only for REST purposes.

useless menu_link_load(), menu_link_save(), etc..

I agree with that and I have no love for those two wrappers, but I do need menu_link_delete() because it has some functionality that can't be moved to the delete() method because of the extra parameters.

{langcode} column... Did you already take care of that in this issue, should we do it here or in a follow-up issue (would be totally fine with me but should probably be major as we should really really do that).

I didn't do anything related to that and it is in the list of followups from #50. Implement the new field api and cleanup localized_title and localized_options.

Right now, I think the serialize flag does not actually have any effect on the database side, it's only used by drupal_write_record() to automatically serialize().

That's the only reason I added the serialize property, we are using drupal_write_record() in the entity save controller.

Now, on to the performance review.

_menu_navigation_links_rebuild() has been improved a lot in #85 but it's only temporary anyway because we want to get rid of it soon.

- Looks like a considerable amount of the additional time and especially those function calls are spent in the offset*() methods for the BC layer. That should just go away, so this should be come irrelevant

From ~4K new function calls on a warm cache, these make up for almost 70-75% of them and, unfortunately, won't go away, they will transform into calls to __get(), __set() and friends, the magic methods from the new entity field api. The plan is to switch to caching rendered entities and this is how we can get rid of them :)

catch’s picture

I haven't looked all the way through this in depth yet, but I've been following the issue and have looked through the patch a couple of times.

Yes to a separate required module for now. Required modules aren't great, but they're better than stuffing things into system module and looks like there's a chance this will eventually be optional.

Also fine with this being the 'straight conversion' patch and handling the deeper issues of what to do about hook_menu() and link rebuilding etc. in follow-ups.

I asked amateescu to create follow-up issues and link to them from the issue summary, as well as to the existing follow-ups that already exist. Won't mark this RTBC yet because I've not fully reviewed it (and the module_invoke() in aggregator looked a bit odd, why not module_exists() for that case?) but seems like it's close.

donquixote’s picture

Just a quick note: It seems that hook_menu_link_alter() is dead with this patch? Replaced with entity hooks?
(I don't mind it being removed)

amateescu’s picture

Yep, hook_menu_link_alter() is replaced by hook_menu_link_presave() and hook_translated_menu_link_alter() is replaced by hook_menu_link_load(). This will be written in the change notice.

amateescu’s picture

Issue summary: View changes

Added link to shortcuts conversion

amateescu’s picture

The issue summary has been rewritten :)

Crell’s picture

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +token, +xmlsitemap, +WSCCI, +D8MI, +language-content

The last submitted patch, menu-links-entities-88.patch, failed testing.

Crell’s picture

I thought as much. :-( amateescu, can you reroll, and specify what sort of review/backup/support you need here? Both WSCCI and D8MI are looking forward to this. :-)

amateescu’s picture

Status: Needs work » Needs review
FileSize
174.9 KB

Let's see if I managed to fix all the conflicts :/

The only thing needed is just a regular patch review, as I removed all the large architectural changes (access and translation handling) from this patch since #66.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -token, -xmlsitemap, -WSCCI, -D8MI, -language-content

The last submitted patch, menu-links-entities-99.patch, failed testing.

andyceo’s picture

Status: Needs work » Needs review

#99: menu-links-entities-99.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +token, +xmlsitemap, +WSCCI, +D8MI, +language-content

The last submitted patch, menu-links-entities-99.patch, failed testing.

disasm’s picture

attached is a reroll.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -token, -xmlsitemap, -WSCCI, -D8MI, -language-content

The last submitted patch, drupal-menu_links_entities-916388-103.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +token, +xmlsitemap, +WSCCI, +D8MI, +language-content

The last submitted patch, drupal-menu_links_entities-916388-103.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

The current testbot failure is due to the run-tests.sh script behaving differently than running tests from the UI. I have successfully ran the first 30 tests in the UI on my local, but get this as the first failure in running the script:

Fatal error: Class name must be a valid object or a string in /www/wscci/core/includes/entity.inc on line 280

Call Stack:
    0.0010     369448   1. {main}() /www/wscci/core/scripts/run-tests.sh:0
    0.0052     785160   2. simpletest_script_run_one_test() /www/wscci/core/scripts/run-tests.sh:22
    0.1828   16664416   3. Drupal\simpletest\TestBase->run() /www/wscci/core/scripts/run-tests.sh:381
    0.1954   16812800   4. Drupal\simpletest\WebTestBase->setUp() /www/wscci/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php:696
    0.2091   17417600   5. install_drupal() /www/wscci/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php:728
    0.2811   18874616   6. install_run_tasks() /www/wscci/core/includes/install.core.inc:85
    1.3213   22105224   7. install_run_task() /www/wscci/core/includes/install.core.inc:430
    1.3213   22105224   8. install_bootstrap_full() /www/wscci/core/includes/install.core.inc:570
    1.3213   22105312   9. drupal_bootstrap() /www/wscci/core/includes/install.core.inc:1502
    1.3213   22105272  10. _drupal_bootstrap_full() /www/wscci/core/includes/bootstrap.inc:2134
    1.3213   22105384  11. menu_set_custom_theme() /www/wscci/core/includes/common.inc:4826
    1.3213   22105464  12. menu_get_custom_theme() /www/wscci/core/includes/menu.inc:1725
    1.3219   22105040  13. menu_get_item() /www/wscci/core/includes/menu.inc:1710
    1.3223   22105752  14. menu_router_rebuild() /www/wscci/core/includes/menu.inc:463
    1.3362   22659768  15. _menu_navigation_links_rebuild() /www/wscci/core/includes/menu.inc:2616
    1.3362   22660520  16. entity_get_controller() /www/wscci/core/includes/menu.inc:2684

FATAL Drupal\action\Tests\BulkFormTest: test runner returned a non-zero error code (255).
disasm’s picture

Status: Needs review » Needs work
amateescu’s picture

Status: Needs work » Needs review
FileSize
175.07 KB

Let's try this reroll.

Status: Needs review » Needs work

The last submitted patch, menu-links-entities-110.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
+++ b/core/modules/system/system.installundefined
@@ -2328,6 +2142,7 @@ function system_update_8039() {
+<<<<<<< HEAD

Had a merge conflict? ;)

How is the performance of this last reroll?
Any idea how to improve this in the future?
What is needed for this patch to get in?
Any other maintainer is going to be added to this patch? Can we remove the todo?

andypost’s picture

I think we need to get rid of menu.inc at all. Working on #1814916: Convert menus into entities I'd like to decouple this classes into lib/Drupal/Core/Menu/Entity/ folder to allow menu and menu_item modules to stay optional. Ideally we can move some abstracted functionality into Component folder

Berdir’s picture

That's not the goal of this issue.

This is just about menu *links* functionality, which builds on top of menu router functionality, which makes up the other half of menu.inc. The remaining stuff in menu.inc we'll hopefully get rid of when everything has been ported to the new router system. Whenever that will happen :)

tim.plunkett’s picture

#113 would be a follow-up, likely postponed on #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity or something like it. Let's just finish this issue first.

amateescu’s picture

FileSize
177.66 KB

Fixed that merge conflict and here's another re-roll to keep up with HEAD.

@#112

How is the performance of this last reroll?

I don't trust my local dev environment for profiling so consider this a call for help on this area :)

Any idea how to improve this in the future?

Depends on the output of the above. What I know for sure that can be improved is removing the use of ArrayAccess.

What is needed for this patch to get in?

Some more reviews I guess.

Any other maintainer is going to be added to this patch? Can we remove the todo?

I don't think we can remove it, I surely can't be the sole maintainer of this critical piece of functionality.

Edit: Interdiffs can be found here: http://drupalcode.org/sandbox/Crell/1260830.git/shortlog/refs/heads/9163...

chx’s picture

I can help maintaining this.

amateescu’s picture

FileSize
177.43 KB

Merged HEAD and added @chx as a maintainer, thanks for that :)

Status: Needs review » Needs work

The last submitted patch, menu-links-entities-118.patch, failed testing.

andypost’s picture

something went wrong with merge again

aspilicious’s picture

menu's are entities now (objects)

"+function menu_link_add(array $menu) {"

is going nuts

amateescu’s picture

Status: Needs work » Needs review
FileSize
177.43 KB

Ok then, let's try again.

andypost’s picture

Played a lot with this patch and I think that only benchmarks delays RTBC, actually I found no visual effect with performance.
... a bit related #1783964: Allow entity types to provide menu items

Anonymous’s picture

Issue tags: +needs profiling

tag update.

podarok’s picture

Status: Needs review » Needs work
root@pubuntu:/var/www/d8/drupal# git apply --check menu-links-entities-122.patch
error: patch failed: core/modules/menu/menu.module:409
error: core/modules/menu/menu.module: patch does not apply
error: core/modules/system/lib/Drupal/system/Tests/Menu/RouterTest.php: No such file or directory

looks like needs reroll

podarok’s picture

Status: Needs work » Needs review
Issue tags: -Needs architectural review, -Entity system, -token, -xmlsitemap, -WSCCI, -D8MI, -needs profiling, -language-content

#122: menu-links-entities-122.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs architectural review, +Entity system, +token, +xmlsitemap, +WSCCI, +D8MI, +needs profiling, +language-content

The last submitted patch, menu-links-entities-122.patch, failed testing.

podarok’s picture

Status: Needs work » Needs review
FileSize
172.27 KB

reroll

Status: Needs review » Needs work

The last submitted patch, 916388-menu-links-entities-128.patch, failed testing.

podarok’s picture

Status: Needs work » Needs review
FileSize
119.4 KB

yet one

Status: Needs review » Needs work

The last submitted patch, 916388-menu-links-entities-130.patch, failed testing.

podarok’s picture

Status: Needs work » Needs review
FileSize
172.82 KB

woops

yet one

podarok’s picture

to MenuLink array conversion test fix

podarok’s picture

some profiling info for the admin/structure/menu/manage/admin page

without patch #133

Overall Summary
Total Incl. Wall Time (microsec): 548,359 microsecs
Total Incl. CPU (microsecs): 540,035 microsecs
Total Incl. MemUse (bytes): 8,365,612 bytes
Total Incl. PeakMemUse (bytes): 8,927,460 bytes
Number of Function Calls: 65,567

with patch #133

Overall Summary
Total Incl. Wall Time (microsec): 700,349 microsecs
Total Incl. CPU (microsecs): 680,043 microsecs
Total Incl. MemUse (bytes): 8,911,904 bytes
Total Incl. PeakMemUse (bytes): 9,528,824 bytes
Number of Function Calls: 76,796

6,5% regression

podarok’s picture

FileSize
302.5 KB
304.05 KB

and here are top calls output

podarok’s picture

Issue tags: -needs profiling

removing tags

Status: Needs review » Needs work

The last submitted patch, 916388-menu-link-entity-134.patch, failed testing.

podarok’s picture

FileSize
172.84 KB
10.86 KB

reroll

podarok’s picture

Status: Needs work » Needs review

bot?

Status: Needs review » Needs work

The last submitted patch, 916388-139.patch, failed testing.

dawehner’s picture

FileSize
172.37 KB

This just fixes some of the issues, if noone took it over, I will try to fix them later during the day.

dawehner’s picture

So the current problem is the following:

  • Shortcuts sets are now config entities, so the current menu arrays are stored in the config file.
  • Before that THIS patch worked in the database because of the serialization of the $shortcut->links
  • But now we end up with storing menu link entities into config, which simply does not work.

One possible solution solution could be to just store the menu item IDs in the config file and load them on the fly.
Do you agree with that?

In general though it feels wrong/bad to have content entities as part of config entities. The other way round sounds better.

tim.plunkett’s picture

If we store content entities in a config entity, we'd have to use the UUID. Not sure if that's a good thing or not, but not sure what the other options are.

andypost’s picture

Shortcut sets does not save links within itself but probably could save a set of UUIDs of links

The suggestion was to make links exportable with shortcuts #1497380-37: Convert shortcut sets to ConfigEntity
Also I filed follow-up #1895938: Add label() method to ShortcutSet where we can clean-up/refactor a logic behind a shortcuts

podarok’s picture

Status: Needs work » Needs review

#141 needs review

Status: Needs review » Needs work

The last submitted patch, drupal-916388-141.patch, failed testing.

aturetta’s picture

But first it needs a re-roll...

Fatal error: Cannot redeclare system_update_8046() (previously declared in ./core/modules/system/system.install:2015)
 in ./core/modules/system/system.install on line 2064
dawehner’s picture

FileSize
180.58 KB

This is far from a real rereole as shortcut is still not working fine, but at least the shortcuts in the standard.install get's created.

I will work on that tomorrow.
One strategy to reduce the amount of changes could be to a) just save the uuids but load all menu items during runtime.

dawehner’s picture

Status: Needs work » Needs review
FileSize
180.58 KB

Puh, finally got it working.

Status: Needs review » Needs work

The last submitted patch, drupal-916388-148.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
185.96 KB

Here is the right version.

Status: Needs review » Needs work
Issue tags: -Needs architectural review, -Entity system, -token, -xmlsitemap, -WSCCI, -D8MI, -language-content

The last submitted patch, drupal-916388-151.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
Issue tags: +Needs architectural review, +Entity system, +token, +xmlsitemap, +WSCCI, +D8MI, +language-content

#151: drupal-916388-151.patch queued for re-testing.

andypost’s picture

Fixed deprecated entity_get_controller() and menu_link_add() to use Menu entity

andypost’s picture

Status: Needs review » Needs work
FileSize
187.46 KB
1.37 KB

Upgrade for shortcut was wrong, so filed #1899682: Add upgrade path tests for shortcut module

Tests should pass now

andypost’s picture

Status: Needs work » Needs review

Green again!!!

dawehner’s picture

What a huge amount of work done here, hopefully I didn't went too much into details with that review (stopped after some time, so that's certainly not complete).

Thanks andypost to make it green again!

+++ b/core/includes/menu.incundefined
@@ -1260,17 +1266,18 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
+              $query = entity_query('menu_link')

I guess we should use the drupal_container() to factor us a query now.

+++ b/core/includes/menu.incundefined
@@ -1260,17 +1266,18 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
+                foreach ($result as $mlid) {
+                  $parents[$mlid] = $mlid;
+                }

As the entity query already does $id => $id, can't we just set $parents += $result?

+++ b/core/includes/menu.incundefined
@@ -2734,156 +2678,115 @@ function menu_get_router() {
+    // The Menu link module is not available at install time, so we need to
+    // hardcode the default storage controller.
+    $menu_link_controller = new MenuLinkStorageController('menu_link');

I'm wondering whether we could still make this configurable by putting that has a service into the container, like menu.link_storage.fallback ?
Another question: can we actually rely on that class, as it's part of the menu_link module?

+++ b/core/includes/menu.incundefined
@@ -2734,156 +2678,115 @@ function menu_get_router() {
-  foreach ($menu as $path => $item) {
-    if ($item['_visible']) {
-      $menu_links[$path] = $item;
-      $sort[$path] = $item['_number_parts'];
+  $router_items = array();
+  foreach ($menu as $path => $router_item) {
+    if ($router_item['_visible']) {
+      $router_items[$path] = $router_item;
+      $sort[$path] = $router_item['_number_parts'];

Should we try to keep these kind of changes out of the patch, so it's smaller? I don't think this is worthwhile to remove again.

+++ b/core/includes/menu.incundefined
@@ -2734,156 +2678,115 @@ function menu_get_router() {
+      // For performance reasons, do a straight query now and convert to a menu
+      // link entity later.

What about putting a @todo here and open a follow-up?

+++ b/core/includes/menu.incundefined
@@ -2734,156 +2678,115 @@ function menu_get_router() {
+        $menu_link = MenuLink::buildFromRouterItem($router_item);

Maybe we could put that into a method on the link controller, so it can be replaced if needed?

+++ b/core/modules/aggregator/aggregator.moduleundefined
@@ -391,7 +391,7 @@ function aggregator_save_category($edit) {
+  if (isset($op) && module_exists('menu_link')) {

Whether you put that into aggregator menu_link, it always feels wrong.

+++ b/core/modules/book/book.moduleundefined
@@ -548,7 +549,9 @@ function _book_update_outline(Node $node) {
+    menu_link_save($node->book);

What about $node->book->save() instead?

+++ b/core/modules/book/book.moduleundefined
@@ -574,9 +577,10 @@ function _book_update_outline(Node $node) {
+  catch (Exception $e) {
+    // Failed to save the menu link.
+    return FALSE;

Shouldn't we at least try to log something?

+++ b/core/modules/book/book.moduleundefined
@@ -916,7 +920,7 @@ function book_node_prepare(Node $node) {
+  return MENU_MAX_DEPTH - 1 - (($book_link['mlid'] && $book_link['has_children']) ? entity_get_controller('menu_link')->findChildrenRelativeDepth($book_link) : 0);

We could also get the controller from the plugin.manager.entity, but I'm not sure whether that is the "standard" now.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -412,11 +414,7 @@ function modifyMenuLink(&$item) {
-    $this->drupalGet('admin/structure/menu/manage/' . $item['menu_name']);
-    $this->assertText($title, 'Menu link was edited');

It seems to make sense to verify that the new title was saved.

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -520,8 +518,8 @@ function enableMenuLink($item) {
+    $item = menu_link_load($mlid);

I guess we should use entity_load directly as much as possible?

+++ b/core/modules/menu/lib/Drupal/menu/Tests/MenuTest.phpundefined
@@ -537,8 +535,16 @@ function assertMenuLink($mlid, array $expected_item) {
+    $query = entity_query('menu_link')

Let's use $this->container->get('entity.query')->get('menu_link')

+++ b/core/modules/menu/menu.admin.incundefined
@@ -574,53 +387,80 @@ function menu_edit_menu_submit($form, &$form_state) {
+    'mlid' => 0,
+    'plid' => 0,

I'm wondering whether we should put such default values into the storage controller?

+++ b/core/modules/menu/menu.moduleundefined
@@ -327,14 +336,12 @@ function menu_parent_options($menus, $item, $type = '') {
+    $menu_link = entity_create('menu_link', array('mlid' => 0));

Another place where it could make sense to default mlid to 0?

+++ b/core/modules/menu/menu.moduleundefined
@@ -503,42 +494,48 @@ function menu_node_prepare(Node $node) {
+        $query = entity_query('menu_link')
+          ->condition('link_path', 'node/' . $node->nid)
+          ->condition('menu_name', $menu_name)
+          ->condition('module', 'menu')
+          ->sort('mlid', 'ASC')
+          ->range(0, 1);
...
+        $query = entity_query('menu_link')
+          ->condition('link_path', 'node/' . $node->nid)
+          ->condition('menu_name', array_values($type_menus), 'IN')
+          ->condition('module', 'menu')
+          ->sort('mlid', 'ASC')
+          ->range(0, 1);

Couldn't we try to use just a single entity query, as IN is actually not required and the rest of the query is the same?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -0,0 +1,229 @@
+   * Overrides Drupal\Core\Entity\EntityFormController::form().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::actions().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::validate().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::submit().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::save().
...
+   * Overrides Drupal\Core\Entity\EntityFormController::delete().

Missing "\" at the front.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -0,0 +1,229 @@
+    if ($delta < 50) {
+      // Old hardcoded value.
+      $delta = 50;
+    }

What about using $delta = max($delta, 50)?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -0,0 +1,229 @@
+    $normal_path = drupal_container()->get('path.alias_manager')->getSystemPath($menu_link->link_path);

I think 'path.alias_manager.cached' is the proper service to use here.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -0,0 +1,229 @@
+    $menu_link->hidden = (int) !$menu_link->enabled;

Don't we want to cast it to a boolean?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -0,0 +1,229 @@
+      $form_state['rebuild'] = TRUE;

Wouldn't cause this to loose some data?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+  /**
+   * @var bool
+   */
+  protected $preventReparenting = FALSE;

Needs documentation.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+      self::$routerItemFields = array_diff(drupal_schema_fields_sql('menu_router'), array('weight'));

In order to be able to override better use static::

Do we use array_diff for such cases beside of http://drupal.org/node/1525176 ?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+    $query->fields('m', self::$routerItemFields);

static

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+   * Overrides Drupal\entity\DatabaseStorageController::attachLoad().
...
+   * Overrides Drupal\entity\DatabaseStorageController::save().

Same as before :(

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+  protected function preSave(EntityInterface $entity) {

Maybe it would help to have a general description what we do in that function?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+  public function loadUpdatedCustomized(array $router_paths) {
+    $query = parent::buildQuery(NULL);
...
+  public function loadModuleAdminTasks() {
+    $query = $this->buildQuery(NULL);
+    $query

Shouldn't we use an entity query instead?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+      $parent_has_children = ((bool) $query->execute()) ? 1 : 0;

Can't we cast it to int directly?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+    // @todo Since all we need is a specific field from the base table, does it
+    // make sense to convert to EFQ?

I would say so.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+  public function countMenuLinks($menu_name) {

In general I like to provide API methods but using entity_query at the end.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,287 @@
+   * @var integer
...
+   *
+   * @var integer

According to http://drupal.org/node/1354 it should be int instead of integer

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,287 @@
+  /**
+   * @var integer
+   */
+  public $has_children = 0;
+
+  /**
+   * @var integer
+   */
+  public $expanded = 0;
+
+  /**
+   * @var integer
+   */
+  public $weight = 0;
+
+  /**
+   * @var integer
+   */
+  public $depth;
+
+  /**
+   * @var integer
+   */

Some of those could get some documentation :)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,287 @@
+   * Implements ArrayAccess::offsetExists().
...
+   * Implements ArrayAccess::offsetGet().
...
+   * Implements ArrayAccess::offsetSet().
...
+   * Implements ArrayAccess::offsetUnset().

What about adding a @deprecate here, so people don't start to use it?

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -0,0 +1,211 @@
+function menu_link_save(MenuLink $menu_link) {

What about adding a @depcreated here as well?

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -0,0 +1,211 @@
+ * @param array<MenuLink> $links
...
+ * @return array<MenuLink>

I would love to have such a documentation standard in core, see #1772420: [policy only] Documentation standard for arrays of a certain type (param/return types)

+++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutStorageController.phpundefined
@@ -27,8 +42,22 @@ public function save(EntityInterface $entity) {
+      foreach ($entity->links as &$link) {

No need to reference it, as it's already an object.

andypost’s picture

@dawehner thanx a lot for deep review

Patch addressed most of all except documentation of variables and changes to controllers
Also #1899682: Add upgrade path tests for shortcut module is commited so no this hunk

Status: Needs review » Needs work

The last submitted patch, 916388-menu_link-158.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
186.93 KB
533 bytes

Reverted book changes. Seems arrayAccess still needed

dawehner’s picture

Did some more review coming from the bottom, so maybe read it like that as well.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+  protected $preventReparenting = FALSE;

Missing docs.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+  public function preventReparenting($value = FALSE) {

MenuLink is one of these examples where replacing the storage controller could be a real usecase, so we should implement an interface in order to tell people about new exposed methods. Could be another follow up.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -0,0 +1,562 @@
+   * @return array<MenuLink>

Another odd comment :)

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,287 @@
+  public $link_title = '';
...
+  public $options = array();
...
+  public $module = 'menu';
...
+  public $hidden = 0;
...
+  public $external;
...
+  public $has_children = 0;
...
+  public $expanded = 0;

Should we document all this vars in this issue, or open an follow up?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,287 @@
+    $duplicate->plid = NULL;

Maybe there should be a comment, why the plid is removed when duplicating.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,287 @@
+    $menu = menu_get_router();

At some point we should rename these functions as they don't actually get the router but the menu links back.

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Plugin/Core/Entity/MenuLink.phpundefined
@@ -0,0 +1,287 @@
+   * @return MenuLink

I think we still want to expose the full namespace, but I'm not sure what the current "standard" tells us to do.

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -0,0 +1,211 @@
+ *   A menu link entity.

... or false if it was not possible to be loaded.

+++ b/core/modules/menu_link/menu_link.moduleundefined
@@ -0,0 +1,211 @@
+function menu_link_clone($links, $menu_name = NULL) {

This function is not used anywhere, I think we should drop it. In general the name is quite confusing as it does something on multiple items.

+++ b/core/modules/shortcut/shortcut.moduleundefined
@@ -344,7 +344,8 @@ function shortcut_set_load($id) {
 function shortcut_shortcut_load($entities) {
   foreach ($entities as $id => $entity) {
-    $entity->set('links', menu_load_links('shortcut-' . $id));
+    // @todo Decide whether we want to load the links dynamically like that.
+//    $entity->set('links', menu_load_links('shortcut-' . $id));
   }
 }

This function can be removed entirely as the logic is handled in the storage controller now.

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/MenuRouterTest.phpundefined
@@ -312,27 +312,30 @@ function testMenuName() {
+    $menu_links = entity_load_multiple_by_properties('menu_link', array('router_path' => 'menu_name_test'));
...
+    $menu_links = entity_load_multiple_by_properties('menu_link', array('router_path' => 'menu_name_test'));

Added a follow up to convert them: #1903774: Replace entity_load_multiple_by_properties() uses in menu module/menu tests with an entity_query

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/TreeOutputTest.phpundefined
@@ -51,6 +35,26 @@ function setUp() {
+    // @todo Prettify this tree buildup code, it's very hard to read.

Opened a follow up for that: #1903768: Fix coding standards in MenuTreeTest

+++ b/core/modules/system/lib/Drupal/system/Tests/Menu/TreeOutputTest.phpundefined
@@ -51,6 +35,26 @@ function setUp() {
+        'link' => new MenuLink(array('menu_name' => 'main-menu', 'mlid' => 1, 'hidden' => 0, 'has_children' => 1, 'title' => 'Item 1', 'in_active_trail' => 1, 'access' => 1, 'href' => 'a', 'localized_options' => array('attributes' => array('title' =>''))), 'menu_link'),

Shouldn't we use entity_create here instead, because you might want to replace the menu_link with another implementation?

+++ b/core/modules/system/system.admin.incundefined
@@ -19,13 +19,19 @@ function system_admin_config_page() {
+    if (!empty($result)) {
+      $menu_links = menu_link_load_multiple($result);
+    }
...
+    foreach ($menu_links as $item) {

This maybe throws a notice, as menu_links is not defined.

+++ b/core/modules/system/system.moduleundefined
@@ -2671,7 +2671,10 @@ function system_admin_menu_block($item) {
+    $menu_links = entity_load_multiple_by_properties('menu_link', array('router_path' => $item['path'], 'module' => 'system'));

@@ -2679,17 +2682,8 @@ function system_admin_menu_block($item) {
+  $menu_links = entity_load_multiple_by_properties('menu_link', array('plid' => $item['mlid'], 'menu_name' => $item['menu_name'], 'hidden' => 0));

See #1887058: Remove usages of entity_load_multiple_by_properties() and EntityStorageController::loadByProperties() so probably also worth for another follow up?

+++ b/core/modules/system/system.moduleundefined
@@ -2679,17 +2682,8 @@ function system_admin_menu_block($item) {
@@ -3454,18 +3448,8 @@ function system_get_module_admin_tasks($module, $info) {

@@ -3517,6 +3501,7 @@ function system_get_module_admin_tasks($module, $info) {
+      $item = entity_create('menu_link', $item);

It feels kinda wrong to let this function return menu link entities? In general at least we should maybe add a follow up to fix the documentation of that function.

+++ b/core/modules/user/user.moduleundefined
@@ -1119,13 +1119,13 @@ function user_menu_site_status_alter(&$menu_site_status, $path) {
+function user_menu_link_presave($link) {

Opened a follow up for that #1903750: Convert menu_link hooks to use Type-Hinting

+++ b/core/profiles/minimal/minimal.infoundefined
@@ -2,6 +2,7 @@ name = Minimal
+dependencies[] = menu_link

+++ b/core/profiles/testing/testing.infoundefined
@@ -3,5 +3,6 @@ description = Minimal profile for running tests. Includes absolutely required mo
+dependencies[] = menu_link

We should maybe add at least a @todo or try to avoid it in general, as this could add a cost to most of the tests.

tim.plunkett’s picture

Status: Needs review » Needs work

Definitely needs work for removing the dependency in the testing profile.

amateescu’s picture

Status: Needs work » Needs review
FileSize
16.25 KB
189.11 KB

Implemented all the changes suggested in #161 except the following:

MenuLink is one of these examples where replacing the storage controller could be a real usecase, so we should implement an interface in order to tell people about new exposed methods. Could be another follow up.

Actually, I plan to take everything out of the storage controller and move this responsability to a new Tree field module, so most of the storage controller code from here is temporary.

I think we still want to expose the full namespace, but I'm not sure what the current "standard" tells us to do.

The current standard is that when you are in a namespaced file, you can leave out the full namespace.

Shouldn't we use entity_create here instead, because you might want to replace the menu_link with another implementation?

Even better, I converted that test to DUTB :P

It feels kinda wrong to let this function return menu link entities? In general at least we should maybe add a follow up to fix the documentation of that function.

The plan is to move those system_* blocks to menu_link.module, here's the followup issue: #1842854: Move system menu block to core

We should maybe add at least a @todo or try to avoid it in general, as this could add a cost to most of the tests.

Definitely needs work for removing the dependency in the testing profile.

Menu link is a *required* module for now (what can you expect from something coming from system.module and menu.inc :P) and I just wanted to explicitely show the new dependency for all profiles. Anyway, I removed the explicit dependency by popular request :)

Also, I had to start a new branch for this, starting with the patch from #160 because of some re-rolls that didn't contain an interdiff :(

Status: Needs review » Needs work

The last submitted patch, 916388-menu_link-163.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.99 KB
189.62 KB

I've no idea who broke user_menu_link_presave() and turned it into user_menu_link_alter() :/

mgifford’s picture

This patch applies nicely. I didn't see if it changed things to entities, but menus still seem to function fine with my brief testing.

I was looking at a somewhat related issue #1543750: Allow menu items without path and had hoped that it might have been fixed here. Unfortunately, you still can't seem to be able to add a menu item without a path:

Error message - The path '#bottom' is either invalid or you do not have access to it.

pwolanin’s picture

Looking at this patch, I'm not sure I see how the LEFT JOIN to menu_router is replaced? After this patch is access to a link no longer checked? Or that's fully postponed to a follow-up issue?

amateescu’s picture

The LEFT JOIN is not replaced, just moved to MenuLinkStorageController::buildQuery().

No change is done regarding access checks, they work just like now thanks to the (temporary) ArrayAccess implementation of the menu link entity. The followup issue is to actually move that access logic to the entity controller.

catch’s picture

Status: Needs review » Reviewed & tested by the community

OK I just read through again and couldn't find anything to complain about.

There's a very significant performance regression here, and this is going to need to be added to #1744302: [meta] Resolve known performance regressions in Drupal 8 once it's in.

On the other hand, there is #1805054: Cache localized, access filtered, URL resolved, and rendered menu trees which for cache hits would be a significant performance improvement compared to Drupal 7 menu link caching (since we'd not be loading the menu links at all for most requests).

Going to bump this to RTBC since it blocks a lot of work, but I'll give it at least 72 hours before commit in case there's any last minute concerns.

larowlan’s picture

webchick’s picture

Title: Convert menu links into entities » Change notice: Convert menu links into entities
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

By Australia time, it's now been more than 48 hours. :) I'm going to call that good enough, so we can make progress here at the sprint. Hope that's ok, catch.

Committed and pushed to 8.x (adjusting the update function number). Thanks!

This will definitely need a change notice methinks. :)

dman’s picture

hook_translated_menu_link_alter() has gone away.
hook_menu_link_alter() has gone away.

Obviously this is a major (good) change that invalidates a few pending patches, like This big documentation cleanup :-(

Yes, a change notice about that would have been useful.
NVM, the guys at the table here pointed out that api.drupal.org was now out of date with the truth... :-B

fago’s picture

Is there an issue for EntityNG conversion of menu links yet?

amateescu’s picture

Yep, it's in the OP. Working on a change notice now.

amateescu’s picture

Status: Active » Needs review
andypost’s picture

Title: Change notice: Convert menu links into entities » Convert menu links into entities
Status: Needs review » Needs work

Suppose change notice should describe dropped menu_link related functions and mentioning about access checks must tell hiw to get access-checked menu-link

amateescu’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

I haven't seen change notices to document the removal of functions, so only hook updates are mentioned for now.

Added a note about getting access-checked menu links under the load example code.

Berdir’s picture

Hm.

This still slows down menu rebuilds enough to considerably slow down test runs. Did a lock test with a randomly picked test class ("Drupal\user\Tests\UserAccountLinksTests"), took 11s before this patch and 15s now.

This results in a lot of aborted test runs as they now take too long and a huge backlog.

Will have a look to see if there's something we can do...

Berdir’s picture

Have been profiling this a bit.

Made two small changes that gets the execution time of my example test back to 13 seconds but there's a lot of things that could be improved.

What I did:
- Remove the count query in findParent() and instead count() the results. That is rather pointless if we, in most cases, expected to find exactly one result, which seems to be the case. This did get the amount of queries from 7xx to 4xx.
- Use a single db_update() in updateParentalStatus() instead of load() & save(). While not nice, that is *considerably* faster.

Some other things that I noticed/some thoughts:
- We're doing hundreds of tag cache clears, for every single saved menu link. Every single saved menu link also calls through the reset function and does a full resetCache(). We should probably inline the (static) cache clears that we actually need and avoid multiple static clears during a single rebuild.
- Seeing 3 calls to dfac() for every test method, twice in the installer and once after all modules were enabled. Wondering if we can improve that. For testing purposes, I commented out the one in resetAll() and things worked just fine. And it saved another 2 seconds. We might want to open an issue to try and get rid of that call there. And maybe look at those in the installer and try to do more specific cache clears there as well instead of using the sledgehammer-method :) This could more than revert the performance regression during tests if we can get that right.
- 1200 calls to menu_get_ancestors() and therefore 1200 uncached calls to the state keyvalue store. I need to get back to that caching issue.
- findParent() is still quite slow.
- Wondering if we can improve updateParentalStatus(), maybe collect plid's and do a single check after we saved all the menu links? It is recalculating many plid's multiple times.
- 2k calls to Drupal\Core\Entity\DatabaseStorageController::create, 90% of that is spent in the (new) create hook, the only implementation seems to be field but a lot of the time actually seems to be spent in rebuilding/calculating the existing hooks, as far as I can see.

sun’s picture

Status: Needs review » Reviewed & tested by the community

Those relatively simple measures look sensible to me.

That said, I still did not look at the originally committed patch, but I'll file follow-up issues if necessary and link to them from here.

plach’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
@@ -345,9 +345,10 @@ protected function updateParentalStatus(EntityInterface $entity, $exclude = FALS
+      db_update('menu_links')
+        ->fields(array('has_children' => $parent_has_children))
+        ->condition('mlid', $entity->plid)
+        ->execute();

Any idea of how to make this work with storage-agnostic entities? Are we requiring menu links to be always stored on a SQL database?

chx’s picture

I will work on #1893772: Move entity-type specific storage logic into entity classes and then other storrage engines can override the specific storage controller.

plach’s picture

Can we put the lines above in a method of the MenuLinksStorageController so that we can provide different implementations depending on the storage without needing to rewrite the whole method?

plach’s picture

@chx:

(crosspost) Yes, I didn't notice the lines were already part of the storage controller.

Berdir’s picture

I guess something like that will need to happen in #1893772: Move entity-type specific storage logic into entity classes, doesn't make much sense to create a method now and then have to move it over to a new class IMHO.

For the record, my patch in #179 was more a work in progress and still took 1h30 on that testbot, that one seems to vary between that and up to 1h45m at the moment, so I'm not sure how much my patch actually improves the situtation. it's likely that the difference was bigger on my local system. But it should improve the situation at least a bit and I'me fine with committing that and then open additional separate follow-up tasks.

plach’s picture

Ok, works for me.

catch’s picture

Status: Reviewed & tested by the community » Active

Thanks Berdir. Those changes look great and I've committed/pushed to 8.x, leaving open for the change notice.

amateescu’s picture

Title: Convert menu links into entities » Change notice: Convert menu links into entities
Status: Active » Needs review

Well, the change notice exists already.. is there anything else we'd like to see in it?

http://drupal.org/node/1914008

dcrocks’s picture

Shouldn't the " * @ todo The paragraph above is lying! This hasn't been (re)implemented yet." be removed from menu_link.api.php for hook_menu_link_presave?

amateescu’s picture

Nope, not yet. We have to figure out if we want to condition firing entity load hooks for menu links based on the old options['alter'] mechanism. And this @todo is on hook_menu_link_load(), not presave.

dcrocks’s picture

This is probably the cause of #1912806: Regression: 'User Account' displayed on front page for anonymous users. What should I look at?

podarok’s picture

Status: Needs review » Fixed

change notice looks good

amateescu’s picture

Priority: Critical » Major

Reverting the priority as well.

amateescu’s picture

Title: Change notice: Convert menu links into entities » Convert menu links into entities

.. and the title :/

holtzermann17’s picture

Looking at the API docs for menu_link and menu, I'm not seeing what seems like a straightforward Organic Groups-like "win" -- the ability to straightforwardly delete the membership of a link in a given menu. Did I miss something? In D7 there's menu_link_delete, which, of course, gets rid of the entire link, and menu_link_save which allows for a complicated way to do revisions (the $existing_item). Maybe converting links into entities will make that easier? (Sorry if I've missed something "obvious"!)

tstoeckler’s picture

Re @holtzermann17:
Converting menu links into entities allows for $menu_link->save() and $menu_link->delete(). Menu links are not revisionable in core but in theory that is overridable in contrib, although that will probably be non-trivial. It's only "obvious" if you are up to speed with the new entity API, so no harm done asking :-)

holtzermann17’s picture

@tstoeckler: Thanks.

After looking a bit further I see that the D8 entity has something called MenuLink::$menu_name -- changing that would presumably change what menu the link is assigned to, or one could copy-and-modify to make two similar links in different menus. This isn't exactly the "OG model" I was thinking about but it does seem potentially less awkward than the D7 menu_link_save method.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Gábor Hojtsy’s picture

So long as the menu items are not in bundles per menu, language settings per menu are impossible as-is right now. Language settings for entities are bundle based, so eg. (for nodes) you can make your press release bundle type translatable while your blog posts are not. This makes them behave pretty differently in terms of UIs showing up for translation, etc. If there is only one bundle for menus or the bundles are cross-menu or even different depending on the level the item is in the menu, the language settings are impossible. I opened #1966298: Introduce menu link bundles per menus to discuss this (again).

andypost’s picture

andypost’s picture

Issue summary: View changes

Rewritten the issue summary.

jnicola’s picture

I upgraded D8 alpha 6 to Alpha 7, and this is the error I am seeing now:

Twig_Error_Runtime: An exception has been thrown during the rendering of a template ("Some mandatory parameters are missing ("menu_link") to generate a URL for route "/admin/config/user-interface/shortcut/link/{menu_link}/delete".") in "core/themes/seven/templates/page.html.twig" at line 74. in Twig_Template->displayWithErrorHandling() (line 279 of /var/sites/jessenicola.dev-vm/core/vendor/twig/twig/lib/Twig/Template.php).

It happens when accessing /#overlay=admin/content, and going to directly to admin/content causes the same issue.

Is there an issue for this already, or worthy of opening a new issue?

effulgentsia’s picture

Folks here might be interested in / have input for #2178723: [meta-2] Finalize module-facing API of declaring menu links, especially comments #39 and later.