Follow-up to #916388: Convert menu links into entities.

Problem/Motivation

We now have a menu link entity but there's still quite some work to do in order to implement proper multilingual support.

Proposed resolution

Let's convert the menu link entity to the new Entity Field API as implemented in #1696640: Implement API to unify entity properties and fields. localized_title and localized_options need to die as a result of this conversion.

Remaining tasks

See above.

User interface changes

None.

API changes

A lot! TBD.

Postponed until

#2054011: Implement built-in support for internal URLs
#2207893: Convert menu tree building to a service.
#2227179: Step 2: Move the menu tree storage to a separate service.

CommentFileSizeAuthor
#193 1842858-165.gz41.82 KBalexwriter2003
#4 1842858-menu-link-entityng-4.patch29.02 KBdsnopek
#5 1842858-menu-link-entityng-5.patch39.3 KBdsnopek
#14 entityng-menu-link-1842858-14.patch39.23 KBshanethehat
#17 menu_link_ng.patch34.31 KBamateescu
#19 menu_links_ng-1842858-19.patch49.1 KBamateescu
#22 interdiff.txt17.74 KBamateescu
#22 menu_links_ng-1842858-22.patch58.34 KBamateescu
#24 menu_links_ng-1842858-24-interdiff.txt7.08 KBBerdir
#24 menu_links_ng-1842858-24.patch63.69 KBBerdir
#26 menu_links_ng-1842858-26-interdiff.txt6.44 KBBerdir
#26 menu_links_ng-1842858-26.patch67.5 KBBerdir
#28 menu_links_ng-1842858-28-interdiff.txt637 bytesBerdir
#28 menu_links_ng-1842858-28.patch67.54 KBBerdir
#32 menu_links_ng-1842858-32-interdiff.txt4.39 KBBerdir
#32 menu_links_ng-1842858-32.patch71.52 KBBerdir
#34 fix-redirect-handling-2056093-15-interdiff.txt5.37 KBBerdir
#34 fix-redirect-handling-2056093-15.patch7.39 KBBerdir
#36 menu_links_ng-1842858-34-interdiff.txt5.04 KBBerdir
#36 menu_links_ng-1842858-34.patch74.91 KBBerdir
#40 menu_links_ng-1842858-40-interdiff.txt28.77 KBBerdir
#40 menu_links_ng-1842858-40.patch86.21 KBBerdir
#43 menu-link-ng-1842858.reroll.patch85.78 KBlarowlan
#46 menu-link-ng-1842858.46.patch87.74 KBlarowlan
#46 menu-link-ng-1842858.46.interdiff.txt3.48 KBlarowlan
#48 menu-link-ng-1842858.48.patch87.74 KBlarowlan
#48 menu-link-ng-1842858.48.interdiff.txt612 byteslarowlan
#51 menu-link-ng-1842858.51.patch85.77 KBpfrenssen
#54 interdiff.txt701 bytespfrenssen
#54 menu-link-ng-1842858-54.patch85.76 KBpfrenssen
#57 interdiff.txt17.47 KBherom
#57 menu-link-ng-1842858-57.patch86.38 KBherom
#64 interdiff.txt1.94 KBherom
#64 menu-link-ng-1842858-64.patch86.41 KBherom
#69 interdiff.txt22.09 KBherom
#69 menu-link-ng-1842858-69.patch96.66 KBherom
#71 menu-link-ng-1842858-71-interdiff.txt17.58 KBBerdir
#71 menu-link-ng-1842858-71.patch98.63 KBBerdir
#73 interdiff-1842858-69-73.txt35.94 KBherom
#73 menu-link-ng-1842858-73.patch109.71 KBherom
#75 interdiff-1842858-73-75.patch49.76 KBherom
#75 menu-link-ng-1842858-75.patch128.64 KBherom
#80 interdiff-1842858-75-80.txt7.73 KBherom
#80 menu-link-ng-1842858-75-80.patch127.19 KBherom
#85 interdiff-1842858-80-85-do-not-test.diff1.97 KBdas-peter
#85 menu-link-ng-1842858-85.patch127.23 KBdas-peter
#87 interdiff-1842858-85-87.patch5.16 KBdas-peter
#87 menu-link-ng-1842858-87.patch129.72 KBdas-peter
#89 menu-link-ng-1842858-89.patch127.94 KBherom
#91 menu-link-ng-1842858-91.patch129.83 KBdas-peter
#91 interdiff-1842858-89-91-do-not-test.patch2.97 KBdas-peter
#92 menu-link-ng-1842858-91.patch129.83 KBdas-peter
#92 interdiff-1842858-89-91-do-not-test.patch2.97 KBdas-peter
#95 menu-link-ng-1842858-95.patch134.8 KBdas-peter
#95 interdiff-1842858-91-95-do-not-test.patch5.85 KBdas-peter
#96 interdiff-1842858-95-96-do-not-test.patch30.85 KBdas-peter
#96 menu-link-ng-1842858-96.patch135.42 KBdas-peter
#100 interdiff-1842858-96-100-do-not-test.patch520 bytesdas-peter
#100 menu-link-ng-1842858-100.patch135.38 KBdas-peter
#109 menu-link-ng-1842858-109.patch136.55 KBBerdir
#111 interdiff-1842858-109-110.txt583 bytesherom
#111 menu-link-ng-1842858-110-do-not-test.patch136.53 KBherom
#112 menu-link-ng-1842858-112.patch136.53 KBeffulgentsia
#114 interdiff-1842858-112-114.txt6.39 KBherom
#114 menu-link-ng-1842858-114.patch135.79 KBherom
#117 menu-link-ng-1842858-117.patch135.8 KBherom
#119 menu-link-ng-1842858-119.patch135.79 KBherom
#121 interdiff-1842858-119-121.txt9.35 KBherom
#121 menu-link-ng-1842858-121.patch135.16 KBherom
#123 interdiff-1842858-121-123.txt870 bytesherom
#123 menu-link-ng-1842858-123.patch135.22 KBherom
#127 interdiff-1842858-123-127.txt601 bytesherom
#127 menu-link-ng-1842858-127.patch135.26 KBherom
#127 interdiff-1842858-119-127.txt10.93 KBherom
#129 interdiff-1842858-127-129.txt893 bytesherom
#129 menu-link-ng-1842858-129.patch135.23 KBherom
#130 menu-link-ng-1842858-130.patch136.23 KBherom
#131 interdiff-1842858-130-131.txt3.42 KBherom
#131 menu-link-ng-1842858-131.patch136.27 KBherom
#137 menu-link-1842858-137.patch136.42 KBtim.plunkett
#139 interdiff-1842858-137-139.txt1.73 KBherom
#139 menu-link-1842858-139.patch136.4 KBherom
#140 interdiff-1842858-137-139.txt3.44 KBherom
#149 menu-link-1842858-149.patch136.41 KBherom
#154 menu-link-1842858-154.patch136.44 KBherom
#156 menu-link-1842858-156.patch135.3 KBpfrenssen
#163 interdiff-conflict-resolution.txt960 bytesYesCT
#163 menu-link-1842858-163.patch136.34 KBYesCT
#165 interdiff-1842858-163-165.txt1.52 KBYesCT
#165 menu-link-1842858-165.patch136.58 KBYesCT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

This is very needed to support translate menu titles (and paths). :)

fago’s picture

dsnopek’s picture

Assigned: Unassigned » dsnopek

I think I'm going to give this one a shot at the sprint tomorrow. Assigning to myself!

dsnopek’s picture

Status: Active » Needs work
FileSize
29.02 KB

Posting my initial patch to prove that I'm actually working on this. :-) It doesn't work yet and isn't ready for review!

I basically looked at the comment patch and applied similar changes to the menu_link module.

The real work begins now!

dsnopek’s picture

Assigned: dsnopek » Unassigned
FileSize
39.3 KB

I got the patch to the point where the menu link add form (ie. admin/structure/menu/manage/main/add) will display (with lots of notices and non-fatal errors). Unfortunately, there is some really heavy stuff in here and this issue is simply above my head... I'm stepping down so someone more knowledgable can tackle this!

Gábor Hojtsy’s picture

This is very needed to support translate menu titles (and paths), so I'd love if people can help by taking this on!

Berdir’s picture

Status: Needs work » Needs review

Let's ask the testbot, the patch will get sent to testbot anyway at some point.

The offset methods are there because this was recently converted from arrays to objects and that is the BC-Layer for old code.

Status: Needs review » Needs work

The last submitted patch, 1842858-menu-link-entityng-5.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy’s picture

Opened #1966398: [PP-1] Refactor menu link properties to multilingual which will be a followup to this. Postponed on this issue. If we want to have translatable menu items ever, working on this sooner than later would be important.

andypost’s picture

Probably options could mimic some from #1818560-61: Convert taxonomy entities to the new Entity Field API pathField

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -26,37 +26,40 @@ public function form(array $form, array &$form_state, EntityInterface $menu_link
+    // @todo: figure out options!
+    /*
     if (isset($menu_link->options['query'])) {
       $path .= '?' . drupal_http_build_query($menu_link->options['query']);
     }
     if (isset($menu_link->options['fragment'])) {
       $path .= '#' . $menu_link->options['fragment'];
     }
-    if ($menu_link->module == 'menu') {
+     */

@@ -70,33 +73,37 @@ public function form(array $form, array &$form_state, EntityInterface $menu_link
-      '#default_value' => isset($menu_link->options['attributes']['title']) ? $menu_link->options['attributes']['title'] : '',
+      // @todo: figure out options!
+      //'#default_value' => isset($menu_link->options['attributes']['title']) ? $menu_link->options['attributes']['title'] : '',

this hunk unresolved

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -70,33 +73,37 @@ public function form(array $form, array &$form_state, EntityInterface $menu_link
-        '#description' => l($menu_link->link_title, $menu_link->href, $menu_link->options),
+        // @todo: where does $menu_link->href come from?
+        // @todo: how to handle $menu_link->options?
+        //'#description' => l($menu_link->link_title->value, $menu_link->href, $menu_link->options),
+        '#description' => l($menu_link->link_title->value, '', array()),

how to deal with dynamic properties?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -125,7 +132,8 @@ public function form(array $form, array &$form_state, EntityInterface $menu_link
       '#type' => 'language_select',
       '#title' => t('Language'),
       '#languages' => LANGUAGE_ALL,
-      '#default_value' => $menu_link->langcode,
+      // @tode: but we didn't define a 'langcode' property!
+      '#default_value' => $menu_link->langcode->value,

language needs definition, there's LanguageItem

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -189,11 +200,14 @@ public function submit(array $form, array &$form_state) {
-    $menu_link->hidden = (int) !$menu_link->enabled;
+    $menu_link->hidden->value = (int) !$menu_link->enabled->value;
+    // @todo: how to make this happen?
     unset($menu_link->enabled);

suppose better to change form element, probably out of scope

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.phpundefined
@@ -189,11 +200,14 @@ public function submit(array $form, array &$form_state) {
-    $menu_link->options['attributes']['title'] = $menu_link->description;
-    list($menu_link->menu_name, $menu_link->plid) = explode(':', $menu_link->parent);
+    // @todo: figure out options!
+    //$menu_link->options['attributes']['title'] = $menu_link->description;
+    // @todo: where does $menu_link->parent come from?
+    list($menu_link->menu_name->value, $menu_link->plid->value) = explode(':', $menu_link->parent);

options...

xjm’s picture

Issue tags: +needs profiling
xjm’s picture

shanethehat’s picture

Status: Needs work » Needs review
FileSize
39.23 KB

#5 no longer applies. Rerolled, still generates many errors.

Status: Needs review » Needs work

The last submitted patch, entityng-menu-link-1842858-14.patch, failed testing.

Berdir’s picture

Tagging. Not a field blocker, but still needs to be converted, which is using the same tag.

amateescu’s picture

FileSize
34.31 KB

Here's what I have so far. This was done before #1893772: Move entity-type specific storage logic into entity classes so a lot of things won't apply anymore :( Leaving at NW for that.

Berdir’s picture

Yes, I did quickly try something similar to that, the problem for me was stuff like $link['options']['something'] = TRUE (don't remember the exact key), where you have by reference stuff. Something there just didn't work for me.

amateescu’s picture

Here's some more progress, rerolled on top of HEAD.

I'm currently stuck with the 'options' field, which needs a 'map_field' data type (we only have 'map' so far). Talked about this with @Berdir and we'll need to talk more with @fago tomorrow. Bot will fail in any way possible so leaving at NW still.

dsnopek’s picture

@amateescu: The 'options' field was basically the reason I stepped down from working on this patch - I couldn't figure out how to handle it. But I'm really excited to see how it gets handled ultimately! It will certainly be educational. :-)

effulgentsia’s picture

Priority: Normal » Critical

Raising to critical per #1818580-7: [Meta] Convert all entity types to the new Entity Field API. It's necessary for allowing contrib (or core, if that still manages to make it in) to make menu links either fieldable or translatable.

amateescu’s picture

Status: Needs work » Needs review
FileSize
17.74 KB
58.34 KB

Even more progress :) The installer works but I can't figure out for the life of me what to do to make that MapItem work. So consider me 'unassigned' for now as I hope @Berdir or @fago can figure that out.

Edit: installing minimal with drush works locally.

The last submitted patch, menu_links_ng-1842858-22.patch, failed testing.

Berdir’s picture

Ok, this should do a bit better. Some simple fixes for the installer, some magic on top of magic to make $link['localized_options']['something] = 'bla' work. Go testbot!

Status: Needs review » Needs work

The last submitted patch, menu_links_ng-1842858-24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
6.44 KB
67.5 KB

Ah, the user menu link presave hook hasn't been converted yet, so logout wasn't visible. Also fixed a bunch of other issues, editing menu links now kinda works.

Note that my hacky offset* methods on MapItem and the way those are currently accessed doesn't match. Currently MapItem defines a value property that's a map. But MapItem is already a Map, so I think we should be able to expose it directly on that level. We'll see.

Status: Needs review » Needs work

The last submitted patch, menu_links_ng-1842858-26.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +sprint
FileSize
637 bytes
67.54 KB

Ok, the alter thingy there messes it up. Commented out for now.

Status: Needs review » Needs work

The last submitted patch, menu_links_ng-1842858-28.patch, failed testing.

dcrocks’s picture

Shouldn't you be making the same changes on function user_menu_link_load() as you do on user_menu_link_presave()? And user__menu_breadcrumb_alter()? Could you explain what setting the 'ALTER' flag messed up?

Berdir’s picture

This patch is far from complete. All I was doing for now was pushing it to install correctly to be able to see the test result. I have no idea what the alter problem is related to, but we'll find out.

Berdir’s picture

Status: Needs work » Needs review
FileSize
4.39 KB
71.52 KB

Re-roll and fixing hopefully most of these exceptions, few actual fails, though.

Status: Needs review » Needs work

The last submitted patch, menu_links_ng-1842858-32.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.37 KB
7.39 KB

Another batch of exception fixes.

plach’s picture

Wrong patch :)

Berdir’s picture

D'oh. Wrong folder, just picking the latest files without reading them ;)

Status: Needs review » Needs work

The last submitted patch, menu_links_ng-1842858-34.patch, failed testing.

smiletrl’s picture

+/**
+ * Defines the 'string_field' entity field item.
+ *
+ * @DataType(
+ *   id = "map_field",
+ *   label = @Translation("Map field item"),
+ *   description = @Translation("An entity field containing a map value."),
+ *   list_class = "\Drupal\Core\Entity\Field\Field"
+ * )
+ */

'string_field' -> 'map_field' :)

xjm’s picture

This is an approved API change per #1983554-8: Remove BC-mode from EntityNG (tagging on behalf of @webchick).

Berdir’s picture

Status: Needs work » Needs review
FileSize
28.77 KB
86.21 KB

Ok, started cleaning this up a bit, added methods and removed the pseudo-public properties, used the methods in some parts, a lot still needs cleanup. Updated defaults value handling and some other things.

Let's see if this introduced any new failures.

Status: Needs review » Needs work

The last submitted patch, menu_links_ng-1842858-40.patch, failed testing.

jair’s picture

Issue tags: +Needs reroll
larowlan’s picture

Status: Needs work » Needs review
FileSize
85.78 KB

First a re-roll

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858.reroll.patch, failed testing.

larowlan’s picture

We're not getting p1-p9, plid populated in {menu_link} table, digging as that will resolve a lot of fails.

larowlan’s picture

Status: Needs work » Needs review
FileSize
87.74 KB
3.48 KB

This fixes the plid issue, and therefore a fair few tests.
Also fixes MenuLinkAccessController (which went in in the last re-roll and caused lots of exceptions).

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858.46.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
87.74 KB
612 bytes

Whoops

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858.48.patch, failed testing.

pwolanin’s picture

pfrenssen’s picture

Rerolled.

smiletrl’s picture

Status: Needs work » Needs review

see the latest patch performance.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858.51.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
701 bytes
85.76 KB

Fixed the invalid PHP syntax.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-54.patch, failed testing.

herom’s picture

Assigned: Unassigned » herom

Assigning to myself. I will post an updated patch very soon.

herom’s picture

Status: Needs work » Needs review
FileSize
17.47 KB
86.38 KB

This should fix the installation failure. Let's see how the tests will do.

pwolanin’s picture

I think we should split out the book module implementation and NOT use entities at all there: #2084421: Phase 2 - Decouple book module schema from menu links

The last submitted patch, menu-link-ng-1842858-57.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

#57: menu-link-ng-1842858-57.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-57.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

#57: menu-link-ng-1842858-57.patch queued for re-testing.

The last submitted patch, menu-link-ng-1842858-57.patch, failed testing.

herom’s picture

Status: Needs review » Needs work
FileSize
1.94 KB
86.41 KB

Improving the patch a bit.

unset($menu_link->options->value['<key>']) didn't seem to work, and generated a lot of exceptions:

Indirect modification of overloaded property Drupal\Core\Entity\Field\Field::$value has no effect

I replaced it with an empty string assignment. The filterEmptyValues function should get rid of the empty values automatically now.

Putting this up to see if I can get further help during the core mentoring hours today.

herom’s picture

Status: Needs work » Needs review

The last submitted patch, menu-link-ng-1842858-64.patch, failed testing.

smiletrl’s picture

Regarding

Indirect modification of overloaded property Drupal\Core\Entity\Field\Field::$value has no effect

Field has made use of __get() and __set(), so you get the Notice. Maybe you could try something like this:

//$this->{$p}->value = $parent->getMaterializedPathEntity($i);
$this->{$p}->setValue($parent->getMaterializedPathEntity($i));

For reference, see Field::setValue

smiletrl’s picture

Also, I'm not sure that unset() doesn't work. There's a possibility that this code/function doesn't get executed at all, but not something that it doesn't work, because as you see, there's a "else {}" clause around it:)

       else {
-        // Use unset() rather than setting to empty string
-        // to avoid redundant serialized data being stored.
-        unset($menu_link->options->value['query']);
+        $menu_link->options->value['query'] = '';
       }

But, based on the documention block here, if you REALLY need to get rid of poor "unset()", I think we could use

       // Use unset() rather than setting to empty string
        // to avoid redundant serialized data being stored.
    //    unset($menu_link->options->value['query']);
//        $menu_link->options->value['query'] = '';
 $menu_link->options->value['query'] = NULL;

Just as the doc says, we don't want redundant serialized data:) And, an empty string will still triger the serializtion for this property.

herom’s picture

Status: Needs work » Needs review
FileSize
22.09 KB
96.66 KB

Here's an updated patch.
I've rewritten MapItem to use its own Map variables, as noted in #26.

Now, the MapItem handles all data assigned to it as properties.
Accessing the "options" property using multiple indexes (like $link['options']['attributes']['href']) can only be used for reading now. Enabling it for writing conflicts with a lot of Field and ItemList stuff.
To write, the property syntax should be used ($link->options->attributes['href']).

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-69.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
17.58 KB
98.63 KB

Thanks for working on this. Fixed a few things, still a lot of crazy stuff to figure out, contextual links for example are exploding now, as are other things. This should have less fails, though.

Not sure if and how this can be finished before other parts in the menu/link/routing system are cleaned up.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-71.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
35.94 KB
109.71 KB

updated patch. should fix a lot of the exceptions.
@Berdir, I merged your changes in #71, but couldn't make the interdiff. attaching interdiff from #69 to #73.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-73.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
49.76 KB
128.64 KB

updated patch. this one should fix most of the fails.

The last submitted patch, interdiff-1842858-73-75.patch, failed testing.

Berdir’s picture

das-peter’s picture

FYI: I'll take a look at this right now at the extended sprint in Prague.

das-peter’s picture

I'm far from complete with the review (I'll check every fail and try to fix it) but here are my first findings. And I won't update the patch for now, to not interfere with @herom.

  1. +++ b/core/includes/menu.inc
    @@ -1738,15 +1748,8 @@ function theme_menu_local_task($variables) {
         $link['localized_options']['html'] = TRUE;
         $link_text = t('!local-task-title!active', array('!local-task-title' => $link['title'], '!active' => $active));
       }
    -  if (!empty($link['href'])) {
    -    // @todo - remove this once all pages are converted to routes.
    -    $a_tag = l($link_text, $link['href'], $link['localized_options']);
    -  }
    -  else {
    -    $a_tag = Drupal::l($link_text, $link['route_name'], $link['route_parameters'], $link['localized_options']);
    -  }
     
    -  return '<li' . (!empty($variables['element']['#active']) ? ' class="active"' : '') . '>' . $a_tag . '</li>';
    +  return '<li' . (!empty($variables['element']['#active']) ? ' class="active"' : '') . '>' . l($link_text, $link['href'], $link['localized_options']) . '</li>';
    

    Why is $link['href'] still used even if it looks like there should be only $link['route_name'] left?
    And why do we still use l() instead of Drupal::l()?
    Is this a re-roll that went wrong?

  2. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
    @@ -212,74 +213,65 @@ protected function actions(array $form, array &$form_state) {
    -    if ($menu_link->link_path != $normal_path) {
    +    $normal_path = $this->pathAliasManager->getSystemPath($menu_link->link_path->value);
    +    if ($menu_link->link_path->value != $normal_path) {
           drupal_set_message(t('The menu system stores system paths only, but will use the URL alias for display. %link_path has been stored as %normal_path', array('%link_path' => $menu_link->link_path, '%normal_path' => $normal_path)));
    -      $menu_link->link_path = $normal_path;
    

    '%link_path' => $menu_link->link_path should be '%link_path' => $menu_link->link_path->value

herom’s picture

updated patch.
@das-peter yes, I did mess up a re-roll. I did another one now, and tried to fix that. unfortunately, that means the interdiff is not all that changed; it's the interdiff after the reroll.

this should fix some of the 'undefined index' exceptions. specifically, entity_create would have missed setting values in oldRouterItem.

The last submitted patch, menu-link-ng-1842858-75-80.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

#80: menu-link-ng-1842858-75-80.patch queued for re-testing.

The last submitted patch, menu-link-ng-1842858-75-80.patch, failed testing.

herom’s picture

Assigned: herom » Unassigned

unassign-ing myself, so @das-peter can work on this.

das-peter’s picture

Assigned: Unassigned » das-peter
Status: Needs work » Needs review
FileSize
1.97 KB
127.23 KB

Tried to fix the handling of the special properties 'options', 'localized_options' and 'route_parameters'.
But it still doesn't look right.
Let's see if the testbot is happier at least a little bit.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-85.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
5.16 KB
129.72 KB

Next attempt.

Status: Needs review » Needs work

The last submitted patch, interdiff-1842858-85-87.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
127.94 KB

just a reroll.
the breadcrumb-test fails should disappear, now that #2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}. has got in.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-89.patch, failed testing.

das-peter’s picture

Status: Needs work » Needs review
FileSize
129.83 KB
2.97 KB

Next bunch of small fixes.

das-peter’s picture

Next bunch of fixes,

The last submitted patch, menu-link-ng-1842858-91.patch, failed testing.

dsnopek’s picture

das-peter’s picture

And the next one (added some new tests as well).
This really could pass, however I don't think it passes a review since there's to much ugly stuff around.
I'll start doing a review myself soon.

das-peter’s picture

Re-rolled to fix new MapItem class (use \Drupal\Core\Entity\Field\FieldItemList).
And quite some cleanup e.g. use methods of the MenuLinkInterface where applicable.

The last submitted patch, menu-link-ng-1842858-96.patch, failed testing.

das-peter’s picture

aspilicious’s picture

+++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/MapItem.php
@@ -0,0 +1,96 @@
+use Drupal\Core\TypedData\Annotation\DataType;

Isn't needed anymore

das-peter’s picture

Removed use Drupal\Core\TypedData\Annotation\DataType; from MapItem.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-100.patch, failed testing.

das-peter’s picture

Assigned: das-peter » Unassigned
Status: Needs work » Needs review

The failed test passes locally. I give the test-bot another go.

das-peter’s picture

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-100.patch, failed testing.

das-peter’s picture

Berdir’s picture

Cross-referencing https://drupal.org/node/2100753, you might want to check that too here to make sure the lazy-save check that we have here works as well.

Berdir’s picture

The last submitted patch, menu-link-ng-1842858-100.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
136.55 KB

Re-roll after EntityNG => ContentEntityBase landed.

aspilicious’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.phpundefined
@@ -321,9 +135,12 @@ public function preSaveRevision(EntityStorageControllerInterface $storage_contro
+>>>>>>> applied patch

:)

herom’s picture

just a tiny comment cleanup on patch #109. no need to run the whole test again.

effulgentsia’s picture

FileSize
136.53 KB

Straight reroll.

Berdir’s picture

Status: Needs review » Needs work

Ok, went through the whole patch for the first time.

Various questions/ideas below, a few things that are wrong (e.g. getRouteParams() and the changed tests).

Expected to see more if/else constructs but looks like we got rid of most of them? yay!

  1. +++ b/core/includes/menu.inc
    +++ b/core/includes/menu.inc
    @@ -643,7 +643,12 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
    

    Ok, so menu_link_localize() still gets both. What's going to happen with that function?

  2. +++ b/core/includes/menu.inc
    @@ -671,7 +676,12 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
           }
    
    @@ -707,8 +717,8 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
    -  if ($link_translate && isset($original_description) && isset($item['options']['attributes']['title']) && $item['options']['attributes']['title'] == $original_description) {
    -    $item['localized_options']['attributes']['title'] = $item['description'];
    +  if ($link_translate && isset($original_description) && isset($item['options']['attributes']['title']) && $item->options->attributes['title'] == $original_description) {
    +    $item->localized_options->setValue(array('title' => $item['description']) + $item['localized_options']['attributes']);
       }
     }
    

    Should we be able to set this in the same way as above now?

    Also interesting that this case apparently doesn't need an if/else.

  3. +++ b/core/includes/menu.inc
    @@ -872,21 +882,18 @@ function menu_tail_load($arg, &$map, $index) {
     function _menu_link_translate(&$item, $translate = FALSE) {
    

    Looks like menu_link_translate() only gets menu links. Not sure if it will stay, but do we want to change it to MenuLinkInterface $item (or $link?) just to confirm/enforce it? (note that all instanceof should be against the interface)

    Seem like all this stuff will go away when we remove the old routing stuff anyway?

  4. +++ b/core/includes/menu.inc
    @@ -872,21 +882,18 @@ function menu_tail_load($arg, &$map, $index) {
    -    $item['href'] = $item['link_path'];
    -    $item['title'] = $item['link_title'];
    -    $item['localized_options'] = $item['options'];
    +    $item['href'] = $item->getLinkPath();
    +    $item['title'] = $item->getLinkTitle();
    +    $item->localized_options->setValue($item->options->getValue());
    

    Ok, so here's the part where href/title and localized_options are set.

    Just wondering, can we comment href/title and see what still relies on that? If it's not that much, maybe we can get rid of it now?

  5. +++ b/core/includes/menu.inc
    @@ -1582,14 +1593,14 @@ function _menu_tree_check_access(&$tree) {
    -      $new_tree[(50000 + $item['weight']) . ' ' . $item['title'] . ' ' . $item['mlid']] = $tree[$key];
    +      $new_tree[(50000 + $item->getWeight()) . ' ' . $item['title'] . ' ' . $item->id()] = $tree[$key];
    

    Can we switch title here to getLinkTitle() that's what we set it to in the code above. Or could something possibly change it?

  6. +++ b/core/includes/menu.inc
    @@ -2600,7 +2611,7 @@ function menu_link_get_preferred($path = NULL, $selected_menu = NULL) {
    -            if ($candidate_item['access']) {
    +            if ($candidate_item->getAccess()) {
    

    Wondering what will happen with the access stuff in long-term and how it's separate from $link->access() (the entity access method9

  7. +++ b/core/includes/menu.inc
    @@ -2600,7 +2611,7 @@ function menu_link_get_preferred($path = NULL, $selected_menu = NULL) {
                     // Store the most specific link.
    
    @@ -2843,6 +2854,8 @@ function _menu_navigation_links_rebuild($menu) {
           // For performance reasons, do a straight query now and convert to a menu
           // link entity later.
           // @todo revisit before release.
    

    I'm not sure if this really still works like this as it's a typed class now, so we expect $entit->original to be the same.. But apparently doesn't break anything.

  8. +++ b/core/includes/menu.inc
    @@ -2897,11 +2909,11 @@ function _menu_navigation_links_rebuild($menu) {
           // If the router path and the link path matches, it's surely a working
           // item, so we clear the updated flag.
    -      $updated = $menu_link->updated && $router_path != $menu_link->link_path;
    +      $updated = $menu_link->getChangedTime() && $router_path != $menu_link->getLinkPath();
     
           $menu_link->router_path = $router_path;
           $menu_link->updated = (int) $updated;
    

    Do we actually still check the updated property?

  9. +++ b/core/includes/path.inc
    +++ b/core/includes/path.inc
    @@ -204,7 +204,7 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) {
    
    @@ -204,7 +204,7 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) {
           $item['link_path']  = $form_item['link_path'];
           $item['link_title'] = $form_item['link_title'];
           $item['external']   = FALSE;
    -      $item['options'] = '';
    +      $item['options'] = array();
           _menu_link_translate($item);
    

    $item here is "$item = db_query(...)->fetchObject(). But the code in that function uses methods, so how can work? Do we still call that function/code path?

  10. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -772,7 +772,7 @@ public function updateOriginalValues() {
    -   * Implements the magic method for setting object properties.
    +   * Implements the magic method for getting object properties.
    

    Valid fix but unrelated, we change nothing else in that class. Fix in a separate issue?

  11. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/MapItem.php
    @@ -0,0 +1,95 @@
    +class MapItem extends FieldItemBase {
    

    @amateescu mentioned that he would like to extend from \ArrayObject to avoid the by reference problem.

    Instead of extending TypedData, we could just extend this class and implement the interfaces ourself.

  12. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/MapItem.php
    @@ -0,0 +1,95 @@
    +  /**
    +   * Overrides \Drupal\Core\Entity\Field\FieldItemBase::setValue().
    +   *
    +   * @param array|null $values
    +   *   An array of property values.
    +   */
    +  public function setValue($values, $notify = TRUE) {
    

    All the methods in here need to us @inheritdoc or get proper documentation if they aren't inherited/overridden.

  13. +++ b/core/lib/Drupal/Core/Entity/Plugin/DataType/MapItem.php
    @@ -0,0 +1,95 @@
    +    $this->properties = array();
    

    This uses $this->properties to store the values. Usually, they are in $this->values and $this->properties contains the instantiated property classes, when requested.

  14. +++ b/core/modules/book/book.module
    @@ -58,7 +58,7 @@ function book_entity_bundle_info() {
       }
    @@ -904,7 +904,7 @@ function book_menu_subtree_data($link) {
    
    @@ -904,7 +904,7 @@ function book_menu_subtree_data($link) {
           }
           $links = array();
           foreach ($query->execute() as $item) {
    -        $links[] = $item;
    +        $links[] = entity_create('menu_link', $item);
    

    book.module is refactored to no longer integrate with the menu system so closely. Hopefully that will avoid things like this.

  15. +++ b/core/modules/menu/menu.module
    @@ -359,22 +359,24 @@ function menu_node_update(EntityInterface $node) {
           }
    
    +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -320,10 +134,10 @@ public function preSaveRevision(EntityStorageControllerInterface $storage_contro
    @@ -338,7 +152,8 @@ public function bundle() {
    
    @@ -338,7 +152,8 @@ public function bundle() {
    -    $duplicate->plid = NULL;
    +
    +    $duplicate->get('plid')->offsetGet(0)->set('value', NULL);
    

    This shouldn't really be necessary, the old code should work the same. Alternatively set('plid', NULL)

  16. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -402,37 +217,101 @@ public static function buildFromRouterItem(array $item) {
    +
    +  public function __set($name, $value) {
    

    Missing documentation.

  17. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -655,12 +537,493 @@ public function findParent(EntityStorageControllerInterface $storage_controller,
     
    +  /**
    +   * overrides \Drupal\Core\Entity\EntityNG::set()
    +   */
    

    @inheritdoc.

  18. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -655,12 +537,493 @@ public function findParent(EntityStorageControllerInterface $storage_controller,
    +  public function set($property_name, $value, $notify = TRUE) {
    +    // work-around entity_form_submit_build_entity() -> entity.set() trying to
    +    // set a non-existing fields into MenuLink.
    +    $definition = $this->getPropertyDefinition($property_name);
    +    if (!$definition) {
    +      $this->$property_name = $value;
    +    }
    +    else {
    +      parent::set($property_name, $value, $notify);
    +    }
    +  }
    

    entity_form_submit_build_entity() is gone. The content entity form controller no longer attempts to write arbitrary stuff.

  19. +++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
    @@ -655,12 +537,493 @@ public function findParent(EntityStorageControllerInterface $storage_controller,
    +   */
    +  public function getRouteParams() {
    +    return $this->get('route_parameters')->value;
    +  }
    

    Does this really return an array? Shouldn't it be getValue() ?

  20. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkFormController.php
    @@ -201,103 +203,65 @@ protected function actions(array $form, array &$form_state) {
    -    if ($saved) {
    +    if ($this->entity->save()) {
           drupal_set_message(t('The menu link has been saved.'));
    -      $form_state['redirect'] = 'admin/structure/menu/manage/' . $menu_link->menu_name;
    +      $form_state['redirect'] = 'admin/structure/menu/manage/' . $this->entity->getMenuName();
         }
         else {
           drupal_set_message(t('There was an error saving the menu link.'), 'error');
    

    This code path doesn't make sense. save() never return FALSE. it returns updated/new flags and throws an exception on error.

  21. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -80,6 +77,7 @@ public function create(array $values) {
         if (!isset($values['bundle']) && isset($values['menu_name'])) {
           $values['bundle'] = $values['menu_name'];
         }
    +
         return parent::create($values);
       }
     
    

    unnecessary change.

  22. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -109,51 +107,73 @@ protected function buildQuery($ids, $revision_id = FALSE) {
       /**
    ...
    +   * Overrides DatabaseStorageControllerNG::mapToStorageRecord().
    

    @inheritdoc.

  23. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -183,20 +206,46 @@ public function save(EntityInterface $entity) {
    +            // @todo, should a different value be returned when saving an entity
    +            // with $isDefaultRevision = FALSE?
    +            if (!$entity->isDefaultRevision()) {
    +              $return = FALSE;
    +            }
    +
    +            if ($this->revisionKey) {
    +              $record->{$this->revisionKey} = $this->saveRevision($entity);
    +            }
    +            if ($this->dataTable) {
    +              $this->savePropertyData($entity);
    +            }
    +            $this->resetCache(array($entity->id()));
                 $entity->postSave($this, TRUE);
    

    Not sure what to do about the revision stuff here. The code is copied from the base implementation but we don't have revisions. I also think this needs to be updated once #2057401: Make the node entity database schema sensible is committed again.

  24. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -238,6 +287,10 @@ public function getPreventReparenting() {
    +    // @todo This doesn't really make sense anymore with EntityNG.. and EFQ got
    +    // OR condition support in the meantime, so convert this query.
    

    @todo comments need to be indented two spaces on the second and following lines.

  25. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -281,13 +330,13 @@ public function loadModuleAdminTasks() {
    -    if ($entity->plid) {
    +    if ($entity->plid->target_id) {
           // Check if at least one visible child exists in the table.
           $query = \Drupal::entityQuery($this->entityType);
           $query
    -        ->condition('menu_name', $entity->menu_name)
    +        ->condition('menu_name', $entity->getMenuName())
             ->condition('hidden', 0)
    -        ->condition('plid', $entity->plid)
    +        ->condition('plid', $entity->plid->target_id)
             ->count();
     
           if ($exclude) {
    @@ -297,7 +346,7 @@ public function updateParentalStatus(EntityInterface $entity, $exclude = FALSE)
    
    @@ -297,7 +346,7 @@ public function updateParentalStatus(EntityInterface $entity, $exclude = FALSE)
           $parent_has_children = ((bool) $query->execute()) ? 1 : 0;
           $this->database->update('menu_links')
             ->fields(array('has_children' => $parent_has_children))
    -        ->condition('mlid', $entity->plid)
    +        ->condition('mlid', $entity->plid->target_id)
    

    Can we use the method here for plid?

  26. +++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.php
    @@ -310,20 +359,20 @@ public function findChildrenRelativeDepth(EntityInterface $entity) {
    -    while ($i <= MENU_MAX_DEPTH && $entity->{$p}) {
    -      $query->condition($p, $entity->{$p});
    +    while ($i <= MENU_MAX_DEPTH && $entity->{$p}->value) {
    +      $query->condition($p, $entity->{$p}->value);
    

    This could also use the method.

  27. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/LinkDelete.php
    @@ -68,10 +68,10 @@ public function buildForm(array $form, array &$form_state, MenuLink $menu_link =
    -    menu_link_delete($this->menuLink->mlid);
    -    $set_name = str_replace('shortcut-', '' , $this->menuLink->menu_name);
    +    menu_link_delete($this->menuLink->id());
    

    @amateescu is working on the shortcut/menulink stuff, how does this affect this code?

  28. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LinksTest.php
    @@ -246,19 +246,19 @@ public function testRouterIntegration() {
    -    $this->assertEqual($menu_link->route_name, 'router_test.1');
    -    $this->assertEqual($menu_link->route_parameters, array());
    +    $this->assertEqual($menu_link->getRouteName(), 'router_test.1');
    +    $this->assertEqual($menu_link->getRouteParams(), array());
    

    So we do have a test for this. Can we change this to assertIdentical? I have a feeling that might then no longe b TRUE

    NULL == array() => TRUE
    NULL === array() => FALSE.

  29. +++ b/core/modules/system/lib/Drupal/system/Tests/Menu/LinksTest.php
    @@ -246,19 +246,19 @@ public function testRouterIntegration() {
    -    $this->assertEqual($menu_link->route_name, 'router_test.3');
    -    $this->assertEqual($menu_link->route_parameters, array('value' => 'test'));
    +    $this->assertEqual($menu_link->getRouteName(), 'router_test.3');
    +    $this->assertEqual($menu_link->getRouteParams(), 'test');
     
         $menu_link = entity_load('menu_link', $menu_link->id());
    -    $this->assertEqual($menu_link->route_name, 'router_test.3');
    -    $this->assertEqual($menu_link->route_parameters, array('value' => 'test'));
    +    $this->assertEqual($menu_link->getRouteName(), 'router_test.3');
    +    $this->assertEqual($menu_link->getRouteParams(), 'test');
    

    oh and yes, these test changes are clearly wrong, the old format was correct.

herom’s picture

Status: Needs work » Needs review
FileSize
6.39 KB
135.79 KB

updated patch. fixed the simple stuff.

5) the 'title' and 'link_title' _could_ be different. at least, they were different sometimes, in _menu_item_localize(); but I couldn't trace back to where the difference came from.

9) yeah, it caused exceptions with just the right data (using a dynamic path from menu_router, when adding a menu link). I added a fix for the exceptions, but drupal_valid_path() still can't validate the dynamic paths. This issue is probably related: #876580: drupal_valid_path fails for dynamic paths (e.g. user/% cannot be added to menus)

10) moved to separate issue: #2105833: documentation fix in ContentEntityBase

13) Well, I supposed the main feature of MapItem was that its properties didn't have to be pre-defined. and that's how I designed it, so that every value would be considered as a property. get/setting the $values was already possible in other FieldItems.

17, 18) removed.

21, 22, 24) fixed.

25, 26) yeah, we can. fixed.

Berdir’s picture

13. Field item classes have two main class properties. $this->properties, which contains instantiated field property classes (done by default for computed things like TextItem->processed, on-demand for simple things. And $this->values to store the values, especially for the non-instantiated properties. You used $this->properties for the values, just switch to $this->values and you should be fine.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-114.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
135.8 KB

reroll.

herom’s picture

Status: Needs review » Needs work

and back to needs work, based on #113 - #115.

herom’s picture

Assigned: Unassigned » herom
Status: Needs work » Needs review
FileSize
135.79 KB

reroll again.
I'll post a patch soon. let's test this reroll in the meantime.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-119.patch, failed testing.

herom’s picture

Assigned: herom » Unassigned
Status: Needs work » Needs review
FileSize
9.35 KB
135.16 KB

if this passes, then points 2, 13, 15, 19 (from comment #113) are fixed.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-121.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
870 bytes
135.22 KB

trying again.

The last submitted patch, menu-link-ng-1842858-123.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

#123: menu-link-ng-1842858-123.patch queued for re-testing.

The last submitted patch, menu-link-ng-1842858-123.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
601 bytes
135.26 KB
10.93 KB

this should be green.

Berdir’s picture

+++ b/core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php
@@ -621,7 +610,8 @@ public function setRouterPath($router_path) {
   public function getOptions() {
-    return $this->get('options')->value;
+    $options = $this->get('options')->getValue();
+    return $options[0];
   }
 
   /**
@@ -794,7 +784,8 @@ public function setAccess($access) {

@@ -794,7 +784,8 @@ public function setAccess($access) {
    * {@inheritdoc}
    */
   public function getRouteParams() {
-    return $this->get('route_parameters')->value;
+    $route_params = $this->get('route_parameters')->getValue();
+    return $route_params[0];
   }

$this->get('route_parameters')->offsetGet(0)->getValue() should work.

herom’s picture

#128 fixed.

herom’s picture

herom’s picture

The last submitted patch, menu-link-ng-1842858-131.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

#131: menu-link-ng-1842858-131.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, menu-link-ng-1842858-131.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#131: menu-link-ng-1842858-131.patch queued for re-testing.

The last submitted patch, menu-link-ng-1842858-131.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
136.42 KB

I can't reproduce the fail locally, but it needed a reroll anyway (just conflicts on use statements)

Status: Needs review » Needs work

The last submitted patch, menu-link-1842858-137.patch, failed testing.

herom’s picture

Status: Needs work » Needs review
FileSize
1.73 KB
136.4 KB

also move MapItem to its new place.

herom’s picture

oh, bad interdiff.

The last submitted patch, menu-link-1842858-139.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

#139: menu-link-1842858-139.patch queued for re-testing.

The last submitted patch, menu-link-1842858-139.patch, failed testing.

smiletrl’s picture

Does anyone get this error?

Notice: Undefined index: und in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 389 of core\lib\Drupal\Core\Entity\ContentEntityBase.php).
InvalidArgumentException: The entity object refers to a removed translation (und) and cannot be manipulated. in Drupal\Core\Entity\ContentEntityBase->getTranslatedField() (line 391 of D:\Apache\drupal8\core\lib\Drupal\Core\Entity\ContentEntityBase.php).

I installled the latest Drupal core-> clear cache -> apply latest patch at #139 -> go to any page -> get above error.

herom’s picture

@smiletrl I think it should be "apply patch #139 -> install Drupal". can you test this way too?

smiletrl’s picture

@herom hah, yeah, thanks -- I missed that:)

smiletrl’s picture

Status: Needs work » Needs review

#139: menu-link-1842858-139.patch queued for re-testing.

markie’s picture

Status: Needs review » Needs work

All
I tried a clean install (new database, new files folder) after applying the patch and received:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: http://drupal8.localhost/core/install.php?langcode=en&profile=standard&id=1&op=do_nojs&op=do
StatusText: OK
ResponseText: 
( ! ) Fatal error: Call to a member function getMenuName() on a non-object in /Library/WebServer/Documents/drupal8/core/modules/menu/menu.install on line 21
Call Stack
#TimeMemoryFunctionLocation
10.0219649496{main}(  )../install.php:0
20.03161877416install_drupal(  )../install.php:47
34.407926751704install_run_tasks(  )../install.core.inc:94
46.473531114856install_run_task(  )../install.core.inc:563
56.474831252944_batch_page(  )../install.core.inc:680
66.485031338168_batch_do(  )../batch.inc:69
76.485031338168_batch_process(  )../batch.inc:93
86.486831424208call_user_func_array
(  )../batch.inc:231
96.486831424248_install_module_batch(  )../batch.inc:231
106.486931424600Drupal\Core\Extension\ModuleHandler->install(  )../install.core.inc:2200
1111.261138553576Drupal\Core\Extension\ModuleHandler->invoke(  )../ModuleHandler.php:650
1211.261138553920call_user_func_array
(  )../ModuleHandler.php:272
1311.261138554168menu_install(  )../ModuleHandler.php:272
herom’s picture

Status: Needs work » Needs review
FileSize
136.41 KB

rerolled. this issue needs more love... and more reviews!

@markie I couldn't reproduce the error you mentioned.

The last submitted patch, menu-link-1842858-149.patch, failed testing.

herom’s picture

saltednut’s picture

  1. Patch #149 applies cleanly to latest HEAD.
  2. D8 installs without failure using the patch.
  3. I did not run all tests locally - only ran the 'Menu' tests (15 total).*

*only 1 fail.

The test did not complete due to a fatal error.
Completion check
MenuRouterTest.php
line 493
Drupal\system\Tests\Menu\MenuRouterTest->testThemeIntegration()

May be a problem with the test? Could be a problem to my unique environment. Looks like the remote testbot likes it so I am not sure.

UPDATE: I re-ran the MenuRouterTest again locally and this time it passed.

I think this is looking pretty good but someone who has more familiarity with the Entity Field API should look over the patch before RTBC.

markie’s picture

Folks
I was able to replicate my issue again, using patch 149 on a fresh clone, on a second machine.

Steps:

  1. Clone drupal
  2. Apply Patch
  3. New database
  4. Install script.
  5. Sadness.
herom’s picture

Issue summary: View changes
FileSize
136.44 KB

another reroll.

@markie The patch from #149 wouldn't apply on HEAD since October 30. Are you using a specific commit/patch/branch, or the plain d8 HEAD? [Edit: noticed the "clone drupal" just now. still, no idea why it would fail. someone else might be able to help.]

pfrenssen’s picture

Assigned: Unassigned » pfrenssen
Status: Needs review » Needs work

Needs a reroll again!

$ git apply menu-link-1842858-149.patch
error: patch failed: core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php:63
error: core/modules/book/lib/Drupal/book/BookBreadcrumbBuilder.php: patch does not apply

Assigning to me for rerolling duty. Also going to test the installation with this patch applied.

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Status: Needs work » Needs review
FileSize
135.3 KB

Rerolled.

pfrenssen’s picture

Installed the patch on the latest 8.x HEAD and installed the site:

[pieter@archlinux drupal (1842858 %)] $ drush si --account-name=admin --account-pass=admin
You are about to empty any Config directories and DROP all tables in your 'drupal' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a few seconds ...                                                                                                                                               [ok]
sh: /usr/bin/sendmail: No such file or directory
WD mail: Error sending e-mail (from admin@example.com to admin@example.com).                                                                                                                             [error]
Installation complete.  User name: admin  User password: admin                                                                                                                                           [ok]
Your sites/default/files directory and its subdirectories are mode 0777 so that they can be cleared by both web and CLI users. If desired, you may now adjust to stricter permissions as per             [ok]
http://symfony.com/doc/current/book/installation.html#configuration-and-setup
Unable to send e-mail. Contact the site administrator if the problem persists.                                                                                                                           [error]
All necessary changes to sites/default and sites/default/settings.php have been made, so you should remove write permissions to them now in order to avoid security risks. If you are unsure how to do   [warning]
so, consult the online handbook.

Installation works fine, and saw no problems after a quick look through the site. @markie, can you test again with the latest HEAD + patch?

Status: Needs review » Needs work

The last submitted patch, 156: menu-link-1842858-156.patch, failed testing.

herom’s picture

Status: Needs work » Needs review

@pfrenssen, you used an old patch (menu-link-1842858-149.patch). the patch was rerolled at #154, and still applies on HEAD. since that patch is green, I'm going to hide your patch at #156, which btw only missed two lines in BookBreadcrumbBuilder.php.

to everyone else: the patch to review is at #154 right now.

pfrenssen’s picture

I wonder what I have been smoking. Thanks for the correction!

YesCT’s picture

I can do a review. (other reviews welcome)

YesCT’s picture

Assigned: Unassigned » YesCT
Status: Needs review » Needs work
Issue tags: +Needs reroll
  1. +++ b/core/includes/menu.inc
    @@ -729,8 +729,16 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
       // If the title and description are the same, use the translated description
       // as a localized title.
    -  if ($link_translate && isset($original_description) && isset($item['options']['attributes']['title']) && $item['options']['attributes']['title'] == $original_description) {
    -    $item['localized_options']['attributes']['title'] = $item['description'];
    +  if ($link_translate && isset($original_description) && isset($options['attributes']['title']) && $options['attributes']['title'] == $original_description) {
    +    $options = $options['attributes'];
    +    $options['title'] = $item['description'];
    +  }
    

    do we still have to do this (use the description if the title and description are the same)?

    maybe the title can be translated now?

  2. +++ b/core/includes/menu.inc
    @@ -729,8 +729,16 @@ function _menu_item_localize(&$item, $map, $link_translate = FALSE) {
    +  if ($item instanceof MenuLinkInterface) {
    +    $item->localized_options->setValue($options);
    +  }
    +  else {
    +    $item['localized_options'] = $options;
       }
    

    if $item can be MenuLinkInterface then we should probably update the @param docs.

    Also, this could use comments.

only got that far into reading the patch, then
... I went to go try to fix this, or investigate, and it does not apply.

I'll attempt the reroll. (using the date on #159 since we know it applied then)

YesCT’s picture

Status: Needs work » Needs review
FileSize
960 bytes
136.34 KB
+++ b/core/modules/menu/menu.module
@@ -338,7 +338,7 @@ function menu_block_view_system_menu_block_alter(array &$build, BlockPluginInter
-      $build['content']['#contextual_links']['menu'] = array('admin/structure/menu/manage', array($build['content'][$key]['#original_link']['menu_name']));
+      $build['content']['#contextual_links']['menu'] = array('admin/structure/menu/manage', array($build['content'][$key]['#original_link']->getMenuName()));

on the conflict lines, the patch was just changing ['menu_name'] to ->getMenuName().

so for this conflict, kept the lines from head, but did that change in them.
see the conflict interdiff for how exactly the conflict was resolved.

Keeping it assigned incase I fix things during the review.

YesCT’s picture

  1. +++ b/core/includes/menu.inc
    @@ -876,21 +884,18 @@ function menu_tail_load($arg, &$map, $index) {
    +    $item->localized_options->setValue($item->options->getValue());
    

    is the only thing in options, localized_options?

    this seems weird. is it future proofing? so that in the future, if we put something else in options, we can change this and get out just the localized bits?

  2. +++ b/core/includes/menu.inc
    @@ -1119,45 +1124,48 @@ function menu_tree_output($tree) {
    -    if ($data['link']['in_active_trail']) {
    +    $localized_attributes = $data['link']->localized_options->attributes;
    +    if ($data['link']->inActiveTrail()) {
           $class[] = 'active-trail';
    -      $data['link']['localized_options']['attributes']['class'][] = 'active-trail';
    +      $localized_attributes['class'][] = 'active-trail';
         }
         // Normally, l() compares the href of every link with the current path and
         // sets the active class accordingly. But local tasks do not appear in menu
         // trees, so if the current path is a local task, and this link is its
         // tab root, then we have to set the class manually.
         if ($data['link']['href'] == $router_item['tab_root_href'] && $data['link']['href'] != current_path()) {
    -      $data['link']['localized_options']['attributes']['class'][] = 'active';
    +      $localized_attributes['class'][] = 'active';
         }
    +    $data['link']->localized_options->attributes = $localized_attributes;
    

    why temporarily use $localized_attributes? why not directly set on $data['link']->localized_options->attributes ?

YesCT’s picture

  1. +++ b/core/includes/menu.inc
    @@ -1365,12 +1373,13 @@ function menu_tree_page_data($menu_name, $max_depth = NULL, $only_active_trail =
    -                if ($active_link['p' . $i]) {
    -                  $active_trail[$active_link['p' . $i]] = $active_link['p' . $i];
    +                $p_i = 'p' . $i;
    +                if ($active_link->{$p_i}->value) {
    +                  $active_trail[$active_link->{$p_i}->value] = $active_link->{$p_i}->value;
    

    this looked weird till @longwave in irc pointed out that {$p_i} is a property, there are a fixed number of them, 9, already declared on MenuLink class https://api.drupal.org/api/drupal/core%21modules%21menu_link%21lib%21Dru... I remember that fixed limit now.

  2. +++ b/core/includes/menu.inc
    @@ -1586,14 +1595,14 @@ function _menu_tree_check_access(&$tree) {
    -    if ($item['access'] || ($item['in_active_trail'] && strpos($item['href'], '%') !== FALSE)) {
    +    if ($item->getAccess() || ($item->inActiveTrail() && strpos($item['href'], '%') !== FALSE)) {
    ...
    -      $new_tree[(50000 + $item['weight']) . ' ' . $item['title'] . ' ' . $item['mlid']] = $tree[$key];
    

    why no method to get the href or the title?

    Better read previous comments/reviews... yep. @Berdir asked the same in #113 5.

    it's for backwards compatibility. it works because of ArrayAccess. class MenuLink extends ContentEntityBase implements \ArrayAccess, MenuLinkInterface {

    maybe we can list an @todo in the code (lines 891,892), or a section in the issue summary here of stuff (things that look suspicious when someone reviews this) that will go away when we remove the old routing stuff. I'll put a note in the issue summary when I update the issue summary (or someone else can).

  3. +++ b/core/includes/menu.inc
    @@ -2719,6 +2728,8 @@ function _menu_navigation_links_rebuild($menu) {
    +      // Add the path to the item.
    +      $router_item['path'] = $key;
    
    @@ -2726,9 +2737,8 @@ function _menu_navigation_links_rebuild($menu) {
    -        ->execute()->fetchAll();
    +        ->execute()->fetch();
           if ($existing_item) {
    -        $existing_item = reset($existing_item);
             $existing_item->options = unserialize($existing_item->options);
             $existing_item->route_parameters = unserialize($existing_item->route_parameters);
    

    why do we have to make these changes to _menu_navigation_links_rebuild()?

  4. +++ b/core/includes/path.inc
    @@ -201,10 +202,9 @@ function drupal_valid_path($path, $dynamic_allowed = FALSE) {
         if ($item = db_query("SELECT * FROM {menu_router} where path = :path", array(':path' => $path))->fetchAssoc()) {
    ...
    +      $item = MenuLink::buildFromRouterItem($item);
    

    this changes item from a router item to a menu link?

  5. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldType/MapItem.php
    @@ -0,0 +1,69 @@
    +   * Overrides \Drupal\Core\Entity\Field\FieldItemBase::setValue().
    +   *
    +   * @param array|null $values
    +   *   An array of property values.
    

    #1994890: Allow {@inheritdoc} and additional documentation is not decided yet, and @Berdir in #115 12 asked for inheritdoc or full docs. this case is full docs since we are changing the param.

    wait.

    there is a weird chain of messed up docs from MapItem up FieldItemBase, DataType/Map, TypedData, TypedDataInterface.

    Maybe FieldItemBase is being more specific than mixed|null and specifying array|null? array is probably a typo there, and mixed is what is meant, in which case that should just be inherit too.

    but we are not changing FieldItemBase, so not going to fix that. a separate issue to fix those docs will be ok. we can just do what we need and use {@inheritdoc} afterall.

  6. put a new use in alpha order

didn't get very far in. mostly only had questions. posting this for now. will pick it back up again later if no-one beats me to it.

Status: Needs review » Needs work

The last submitted patch, 163: menu-link-1842858-163.patch, failed testing.

msonnabaum’s picture

Issue tags: +Needs reroll

Was going to profile this, but it looks like it needs another reroll.

Berdir’s picture

Will re-roll after #2095283: Remove non-storage logic from the storage controllers is in (which I hope will happen soon), will just conflict again with that.

amateescu’s picture

Status: Needs work » Postponed

Just a note that we now have an issue for supporting multi-column (base) field types, and we should postpone this conversion on it because there's really no other way to solve the performance regression introduced here.

See my comment on #2142549-2: Support multi-column field types in base tables

pwolanin’s picture

Issue tags: +beta blocker

The conversion of menu links to entities is pretty pointless if we cannot at least translate the titles, and assuming this change is required, I think we need to have that in place before beta due to the APi and schema changes involved.

Gábor Hojtsy’s picture

@pwolanin: my understanding was that this issue would not in itself do multilingual properties or menu items (which would be required to translate titles). I marked #1966398: [PP-1] Refactor menu link properties to multilingual as a beta blocker as well as a consequence which is to cover that by my understanding. So we are even more steps away from that :/

effulgentsia’s picture

Status: Postponed » Needs work
Berdir’s picture

Assigned: YesCT » amateescu

@amateescu said he has a plan for making this happen ;)

amateescu’s picture

Status: Needs work » Postponed
Issue tags: -sprint, -Needs reroll

Yep, the first step is #2054011-7: Implement built-in support for internal URLs. So.. postponing again I guess.

xjm’s picture

Gábor Hojtsy’s picture

#2054011: Implement built-in support for internal URLs is not moving at all. Is that still a blocker for this? Any other blockers?

xjm’s picture

YesCT’s picture

amateescu’s picture

Another issue which will impact restarting work here has been opened: #2207893: Convert menu tree building to a service.

jessebeach’s picture

Title: Convert menu links to the new Entity Field API » [PP-2] Convert menu links to the new Entity Field API
Issue summary: View changes
tim.plunkett’s picture

Title: [PP-2] Convert menu links to the new Entity Field API » Convert menu links to the new Entity Field API
Status: Postponed » Active

Both blockers are in now!

effulgentsia’s picture

Hm. So there's still the p1-p9 columns to deal with. #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins plans to remove those. Not sure whether that means to:
- postpone this issue on that (not really happy about that though, since I don't know how long that one will take)
- do this issue with a temporary solution for p1-p9 that we'll remove once the other one lands (might be annoying in terms of having to do a bunch of profiling/optimizing of code that won't be final)
- create a separate issue for moving the hierarchy elsewhere independent of the rest of #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins (I think that's my favorite option)

@amateescu: what are your thoughts/plans?

amateescu’s picture

#182.3 is also my favorite option :) It's probably the only sane one since the plan to implement menu links as plugins wasn't approved by core maintainers (afaik) and, like you said, who knows how much it would take (especially the UI part). A temporary solution here is beyond useless..

I can start working in a couple of days on the generic hierarchy stuff that would've been part of this patch and I'll link the issue here when I have it.

dawehner’s picture

It's probably the only sane one since the plan to implement menu links as plugins wasn't approved by core maintainers (afaik)

Well, catch was part of the discussion, I guess this can be considered as approval.

We do already have separated the tree building, #2227179: Step 2: Move the menu tree storage to a separate service. will move out the tree storage from menu links directly ... this will
certainly allow us to get rid of the performance problems of content entities ... so I don't really see how your work on generic hierarchy stuff would be helpful. This is kind of implemented automatically given the current approach.

amateescu’s picture

Well, catch was part of the discussion, I guess this can be considered as approval.

Except for that call, I wasn't there so I can't know :)

We do already have separated the tree building, #2227179: Step 2: Move the menu tree storage to a separate service. will move out the tree storage from menu links directly ... this will certainly allow us to get rid of the performance problems of content entities ... so I don't really see how your work on generic hierarchy stuff would be helpful. This is kind of implemented automatically given the current approach.

I wanted to have something (1) generic, not tied to menus at all *but also available* as a field for easier integration into any entity type (including its usage as a base field) and (2) with swappable hierarchy implementations, not tied to the current materialized path algorithm. If everyone is ok with doing only #2227179: Step 2: Move the menu tree storage to a separate service. (and #2227441: New plan, Phase 1:Review the architecture and overall implementation proposal for menu links as plugins), I'm perfectly fine with that and this issue doesn't need to be assigned to me anymore.

amateescu’s picture

Assigned: amateescu » Unassigned

I meant to do this.

effulgentsia’s picture

Title: Convert menu links to the new Entity Field API » [PP-1] Convert menu links to the new Entity Field API
Status: Active » Postponed

Postponing on #2227179: Step 2: Move the menu tree storage to a separate service., unless someone has an idea on how to make progress without it.

Re #185: @amateescu: could your ideas on decoupling tree storage from menu tree storage, making the implementation swappable, and creating a field type around it for easier use by other entity types all be done as non-beta-blocking follow ups after #2227179: Step 2: Move the menu tree storage to a separate service.? Or is there something about that issue that is moving us backwards relative to HEAD? Perhaps leave a comment in that issue if you think it is?

amateescu’s picture

@effulgentsia, it can be done as a non-beta-blocking followup, but not while there are other patches moving code around in this area..

jessebeach’s picture

Issue summary: View changes
pwolanin’s picture

effulgentsia’s picture

Priority: Critical » Major
Issue tags: -beta blocker +beta target

From #2256497-28: [meta] Menu Links - New Plan for the Homestretch by Dries:

While the current menu link system implementation (neither a ConfigEntity nor a ContentEntity) isn’t pretty, and would require a contrib module to support translating custom link titles and integrating with REST, that is not a regression compared to Drupal 7... We can live with those limitations for one release.

Therefore, downgrading this to Major. It would still be great for it to happen; just not a release blocker.

Note, however, that this is based on there being no known critical bugs with MenuLink as a direct extendor of Entity rather than ContentEntity. If ones surface, that could push this back into being critical.

alexwriter2003’s picture

FileSize
41.82 KB

Thanks YesCT