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.

Files: 
CommentFileSizeAuthor
#165 menu-link-1842858-165.patch136.58 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 57,850 pass(es), 27 fail(s), and 22 exception(s).
[ View ]
#165 interdiff-1842858-163-165.txt1.52 KBYesCT
#163 menu-link-1842858-163.patch136.34 KBYesCT
FAILED: [[SimpleTest]]: [MySQL] 58,094 pass(es), 28 fail(s), and 22 exception(s).
[ View ]
#163 interdiff-conflict-resolution.txt960 bytesYesCT
#154 menu-link-1842858-154.patch136.44 KBherom
PASSED: [[SimpleTest]]: [MySQL] 59,399 pass(es).
[ View ]
#149 menu-link-1842858-149.patch136.41 KBherom
PASSED: [[SimpleTest]]: [MySQL] 59,749 pass(es).
[ View ]
#140 interdiff-1842858-137-139.txt3.44 KBherom
#139 menu-link-1842858-139.patch136.4 KBherom
PASSED: [[SimpleTest]]: [MySQL] 59,068 pass(es).
[ View ]
#139 interdiff-1842858-137-139.txt1.73 KBherom
#137 menu-link-1842858-137.patch136.42 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#131 menu-link-ng-1842858-131.patch136.27 KBherom
FAILED: [[SimpleTest]]: [MySQL] 59,255 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#131 interdiff-1842858-130-131.txt3.42 KBherom
#130 menu-link-ng-1842858-130.patch136.23 KBherom
PASSED: [[SimpleTest]]: [MySQL] 58,633 pass(es).
[ View ]
#129 menu-link-ng-1842858-129.patch135.23 KBherom
PASSED: [[SimpleTest]]: [MySQL] 58,619 pass(es).
[ View ]
#129 interdiff-1842858-127-129.txt893 bytesherom
#127 interdiff-1842858-119-127.txt10.93 KBherom
#127 menu-link-ng-1842858-127.patch135.26 KBherom
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]
#127 interdiff-1842858-123-127.txt601 bytesherom
#123 menu-link-ng-1842858-123.patch135.22 KBherom
FAILED: [[SimpleTest]]: [MySQL] 58,846 pass(es), 7 fail(s), and 0 exception(s).
[ View ]
#123 interdiff-1842858-121-123.txt870 bytesherom
#121 menu-link-ng-1842858-121.patch135.16 KBherom
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php.
[ View ]
#121 interdiff-1842858-119-121.txt9.35 KBherom
#119 menu-link-ng-1842858-119.patch135.79 KBherom
FAILED: [[SimpleTest]]: [MySQL] 58,383 pass(es), 65 fail(s), and 8,994 exception(s).
[ View ]
#117 menu-link-ng-1842858-117.patch135.8 KBherom
PASSED: [[SimpleTest]]: [MySQL] 58,711 pass(es).
[ View ]
#114 menu-link-ng-1842858-114.patch135.79 KBherom
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-link-ng-1842858-114_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#114 interdiff-1842858-112-114.txt6.39 KBherom
#112 menu-link-ng-1842858-112.patch136.53 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 58,807 pass(es).
[ View ]
#111 menu-link-ng-1842858-110-do-not-test.patch136.53 KBherom
#111 interdiff-1842858-109-110.txt583 bytesherom
#109 menu-link-ng-1842858-109.patch136.55 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,738 pass(es).
[ View ]
#100 menu-link-ng-1842858-100.patch135.38 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-link-ng-1842858-100.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#100 interdiff-1842858-96-100-do-not-test.patch520 bytesdas-peter
#96 menu-link-ng-1842858-96.patch135.42 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 59,393 pass(es).
[ View ]
#96 interdiff-1842858-95-96-do-not-test.patch30.85 KBdas-peter
#95 interdiff-1842858-91-95-do-not-test.patch5.85 KBdas-peter
#95 menu-link-ng-1842858-95.patch134.8 KBdas-peter
PASSED: [[SimpleTest]]: [MySQL] 58,616 pass(es).
[ View ]
#92 interdiff-1842858-89-91-do-not-test.patch2.97 KBdas-peter
#92 menu-link-ng-1842858-91.patch129.83 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 58,605 pass(es), 26 fail(s), and 26 exception(s).
[ View ]
#91 interdiff-1842858-89-91-do-not-test.patch2.97 KBdas-peter
#91 menu-link-ng-1842858-91.patch129.83 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 58,415 pass(es), 26 fail(s), and 26 exception(s).
[ View ]
#89 menu-link-ng-1842858-89.patch127.94 KBherom
FAILED: [[SimpleTest]]: [MySQL] 58,497 pass(es), 126 fail(s), and 468 exception(s).
[ View ]
#87 menu-link-ng-1842858-87.patch129.72 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 58,532 pass(es), 42 fail(s), and 26 exception(s).
[ View ]
#87 interdiff-1842858-85-87.patch5.16 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1842858-85-87.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#85 menu-link-ng-1842858-85.patch127.23 KBdas-peter
FAILED: [[SimpleTest]]: [MySQL] 58,299 pass(es), 89 fail(s), and 105 exception(s).
[ View ]
#85 interdiff-1842858-80-85-do-not-test.diff1.97 KBdas-peter
#80 menu-link-ng-1842858-75-80.patch127.19 KBherom
FAILED: [[SimpleTest]]: [MySQL] 58,280 pass(es), 101 fail(s), and 38 exception(s).
[ View ]
#80 interdiff-1842858-75-80.txt7.73 KBherom
#75 menu-link-ng-1842858-75.patch128.64 KBherom
FAILED: [[SimpleTest]]: [MySQL] 57,453 pass(es), 168 fail(s), and 247 exception(s).
[ View ]
#75 interdiff-1842858-73-75.patch49.76 KBherom
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1842858-73-75.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#73 menu-link-ng-1842858-73.patch109.71 KBherom
FAILED: [[SimpleTest]]: [MySQL] 58,307 pass(es), 451 fail(s), and 357 exception(s).
[ View ]
#73 interdiff-1842858-69-73.txt35.94 KBherom
#71 menu-link-ng-1842858-71.patch98.63 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 58,875 pass(es), 208 fail(s), and 560 exception(s).
[ View ]
#71 menu-link-ng-1842858-71-interdiff.txt17.58 KBBerdir
#69 menu-link-ng-1842858-69.patch96.66 KBherom
FAILED: [[SimpleTest]]: [MySQL] 58,244 pass(es), 252 fail(s), and 8,185 exception(s).
[ View ]
#69 interdiff.txt22.09 KBherom
#64 menu-link-ng-1842858-64.patch86.41 KBherom
FAILED: [[SimpleTest]]: [MySQL] 55,160 pass(es), 935 fail(s), and 808 exception(s).
[ View ]
#64 interdiff.txt1.94 KBherom
#57 menu-link-ng-1842858-57.patch86.38 KBherom
FAILED: [[SimpleTest]]: [MySQL] 56,255 pass(es), 1,239 fail(s), and 2,186 exception(s).
[ View ]
#57 interdiff.txt17.47 KBherom
#54 menu-link-ng-1842858-54.patch85.76 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#54 interdiff.txt701 bytespfrenssen
#51 menu-link-ng-1842858.51.patch85.77 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php.
[ View ]
#48 menu-link-ng-1842858.48.interdiff.txt612 byteslarowlan
#48 menu-link-ng-1842858.48.patch87.74 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,520 pass(es), 366 fail(s), and 1,328 exception(s).
[ View ]
#46 menu-link-ng-1842858.46.interdiff.txt3.48 KBlarowlan
#46 menu-link-ng-1842858.46.patch87.74 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#43 menu-link-ng-1842858.reroll.patch85.78 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 57,479 pass(es), 367 fail(s), and 7,417 exception(s).
[ View ]
#40 menu_links_ng-1842858-40.patch86.21 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,117 pass(es), 338 fail(s), and 383 exception(s).
[ View ]
#40 menu_links_ng-1842858-40-interdiff.txt28.77 KBBerdir
#36 menu_links_ng-1842858-34.patch74.91 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,851 pass(es), 109 fail(s), and 349 exception(s).
[ View ]
#36 menu_links_ng-1842858-34-interdiff.txt5.04 KBBerdir
#34 fix-redirect-handling-2056093-15.patch7.39 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-redirect-handling-2056093-15_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#34 fix-redirect-handling-2056093-15-interdiff.txt5.37 KBBerdir
#32 menu_links_ng-1842858-32.patch71.52 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 56,897 pass(es), 155 fail(s), and 1,258 exception(s).
[ View ]
#32 menu_links_ng-1842858-32-interdiff.txt4.39 KBBerdir
#28 menu_links_ng-1842858-28.patch67.54 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 57,040 pass(es), 256 fail(s), and 20,469 exception(s).
[ View ]
#28 menu_links_ng-1842858-28-interdiff.txt637 bytesBerdir
#26 menu_links_ng-1842858-26.patch67.5 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#26 menu_links_ng-1842858-26-interdiff.txt6.44 KBBerdir
#24 menu_links_ng-1842858-24.patch63.69 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]
#24 menu_links_ng-1842858-24-interdiff.txt7.08 KBBerdir
#22 menu_links_ng-1842858-22.patch58.34 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#22 interdiff.txt17.74 KBamateescu
#19 menu_links_ng-1842858-19.patch49.1 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links_ng-1842858-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#17 menu_link_ng.patch34.31 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_link_ng.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 entityng-menu-link-1842858-14.patch39.23 KBshanethehat
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#5 1842858-menu-link-entityng-5.patch39.3 KBdsnopek
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#4 1842858-menu-link-entityng-4.patch29.02 KBdsnopek
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

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

Assigned:Unassigned» dsnopek

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

Status:Active» Needs work
StatusFileSize
new29.02 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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!

Assigned:dsnopek» Unassigned
StatusFileSize
new39.3 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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!

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

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.

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.

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...

Issue tags:+Needs profiling

Status:Needs work» Needs review
StatusFileSize
new39.23 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

#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.

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

StatusFileSize
new34.31 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_link_ng.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

StatusFileSize
new49.1 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu_links_ng-1842858-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

@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. :-)

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.

Status:Needs work» Needs review
StatusFileSize
new17.74 KB
new58.34 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

StatusFileSize
new7.08 KB
new63.69 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to login to test site.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new6.44 KB
new67.5 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
Issue tags:+sprint
StatusFileSize
new637 bytes
new67.54 KB
FAILED: [[SimpleTest]]: [MySQL] 57,040 pass(es), 256 fail(s), and 20,469 exception(s).
[ View ]

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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new4.39 KB
new71.52 KB
FAILED: [[SimpleTest]]: [MySQL] 56,897 pass(es), 155 fail(s), and 1,258 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new5.37 KB
new7.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fix-redirect-handling-2056093-15_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another batch of exception fixes.

Wrong patch :)

StatusFileSize
new5.04 KB
new74.91 KB
FAILED: [[SimpleTest]]: [MySQL] 56,851 pass(es), 109 fail(s), and 349 exception(s).
[ View ]

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.

+/**
+ * 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' :)

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

Status:Needs work» Needs review
StatusFileSize
new28.77 KB
new86.21 KB
FAILED: [[SimpleTest]]: [MySQL] 57,117 pass(es), 338 fail(s), and 383 exception(s).
[ View ]

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.

Issue tags:+Needs reroll

Status:Needs work» Needs review
StatusFileSize
new85.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,479 pass(es), 367 fail(s), and 7,417 exception(s).
[ View ]

First a re-roll

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new87.74 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new3.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.

Status:Needs work» Needs review
StatusFileSize
new87.74 KB
FAILED: [[SimpleTest]]: [MySQL] 57,520 pass(es), 366 fail(s), and 1,328 exception(s).
[ View ]
new612 bytes

Whoops

Status:Needs review» Needs work

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

StatusFileSize
new85.77 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php.
[ View ]

Rerolled.

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.

Status:Needs work» Needs review
StatusFileSize
new701 bytes
new85.76 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Fixed the invalid PHP syntax.

Status:Needs review» Needs work

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

Assigned:Unassigned» herom

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

Status:Needs work» Needs review
StatusFileSize
new17.47 KB
new86.38 KB
FAILED: [[SimpleTest]]: [MySQL] 56,255 pass(es), 1,239 fail(s), and 2,186 exception(s).
[ View ]

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

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.

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.

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.

Status:Needs review» Needs work
StatusFileSize
new1.94 KB
new86.41 KB
FAILED: [[SimpleTest]]: [MySQL] 55,160 pass(es), 935 fail(s), and 808 exception(s).
[ View ]

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.

Status:Needs work» Needs review

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

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:

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

For reference, see Field::setValue

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:)

<?php
      
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
<?php
      
// 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.

Status:Needs work» Needs review
StatusFileSize
new22.09 KB
new96.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,244 pass(es), 252 fail(s), and 8,185 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new17.58 KB
new98.63 KB
FAILED: [[SimpleTest]]: [MySQL] 58,875 pass(es), 208 fail(s), and 560 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new35.94 KB
new109.71 KB
FAILED: [[SimpleTest]]: [MySQL] 58,307 pass(es), 451 fail(s), and 357 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new49.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1842858-73-75.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new128.64 KB
FAILED: [[SimpleTest]]: [MySQL] 57,453 pass(es), 168 fail(s), and 247 exception(s).
[ View ]

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

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

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

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

StatusFileSize
new7.73 KB
new127.19 KB
FAILED: [[SimpleTest]]: [MySQL] 58,280 pass(es), 101 fail(s), and 38 exception(s).
[ View ]

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.

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.

Assigned:herom» Unassigned

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

Assigned:Unassigned» das-peter
Status:Needs work» Needs review
StatusFileSize
new1.97 KB
new127.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,299 pass(es), 89 fail(s), and 105 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new5.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-1842858-85-87.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new129.72 KB
FAILED: [[SimpleTest]]: [MySQL] 58,532 pass(es), 42 fail(s), and 26 exception(s).
[ View ]

Next attempt.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new127.94 KB
FAILED: [[SimpleTest]]: [MySQL] 58,497 pass(es), 126 fail(s), and 468 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new129.83 KB
FAILED: [[SimpleTest]]: [MySQL] 58,415 pass(es), 26 fail(s), and 26 exception(s).
[ View ]
new2.97 KB

Next bunch of small fixes.

StatusFileSize
new129.83 KB
FAILED: [[SimpleTest]]: [MySQL] 58,605 pass(es), 26 fail(s), and 26 exception(s).
[ View ]
new2.97 KB

Next bunch of fixes,

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

StatusFileSize
new134.8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,616 pass(es).
[ View ]
new5.85 KB

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.

StatusFileSize
new30.85 KB
new135.42 KB
PASSED: [[SimpleTest]]: [MySQL] 59,393 pass(es).
[ View ]

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.

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

Isn't needed anymore

StatusFileSize
new520 bytes
new135.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-link-ng-1842858-100.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

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

Status:Needs review» Needs work

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

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.

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

Status:Needs work» Needs review
StatusFileSize
new136.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,738 pass(es).
[ View ]

Re-roll after EntityNG => ContentEntityBase landed.

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

:)

Issue tags:-Needs reroll
StatusFileSize
new583 bytes
new136.53 KB

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

StatusFileSize
new136.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,807 pass(es).
[ View ]

Straight reroll.

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.

Status:Needs work» Needs review
StatusFileSize
new6.39 KB
new135.79 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch menu-link-ng-1842858-114_1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new135.8 KB
PASSED: [[SimpleTest]]: [MySQL] 58,711 pass(es).
[ View ]

reroll.

Status:Needs review» Needs work

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

Assigned:Unassigned» herom
Status:Needs work» Needs review
StatusFileSize
new135.79 KB
FAILED: [[SimpleTest]]: [MySQL] 58,383 pass(es), 65 fail(s), and 8,994 exception(s).
[ View ]

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.

Assigned:herom» Unassigned
Status:Needs work» Needs review
StatusFileSize
new9.35 KB
new135.16 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/menu_link/lib/Drupal/menu_link/Entity/MenuLink.php.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new870 bytes
new135.22 KB
FAILED: [[SimpleTest]]: [MySQL] 58,846 pass(es), 7 fail(s), and 0 exception(s).
[ View ]

trying again.

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

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.

Status:Needs work» Needs review
StatusFileSize
new601 bytes
new135.26 KB
PASSED: [[SimpleTest]]: [MySQL] 58,632 pass(es).
[ View ]
new10.93 KB

this should be green.

+++ 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.

StatusFileSize
new893 bytes
new135.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,619 pass(es).
[ View ]

#128 fixed.

StatusFileSize
new136.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,633 pass(es).
[ View ]

StatusFileSize
new3.42 KB
new136.27 KB
FAILED: [[SimpleTest]]: [MySQL] 59,255 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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.

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.

Status:Needs work» Needs review
StatusFileSize
new136.42 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.73 KB
new136.4 KB
PASSED: [[SimpleTest]]: [MySQL] 59,068 pass(es).
[ View ]

also move MapItem to its new place.

StatusFileSize
new3.44 KB

oh, bad interdiff.

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

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.

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.

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

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

Status:Needs work» Needs review

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

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

Status:Needs work» Needs review
StatusFileSize
new136.41 KB
PASSED: [[SimpleTest]]: [MySQL] 59,749 pass(es).
[ View ]

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.

  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.

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.

Issue summary:View changes
StatusFileSize
new136.44 KB
PASSED: [[SimpleTest]]: [MySQL] 59,399 pass(es).
[ View ]

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.]

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.

Assigned:pfrenssen» Unassigned
Status:Needs work» Needs review
StatusFileSize
new135.3 KB
FAILED: [[SimpleTest]]: [MySQL] 59,581 pass(es), 6 fail(s), and 336 exception(s).
[ View ]

Rerolled.

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.

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.

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

I can do a review. (other reviews welcome)

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)

Status:Needs work» Needs review
StatusFileSize
new960 bytes
new136.34 KB
FAILED: [[SimpleTest]]: [MySQL] 58,094 pass(es), 28 fail(s), and 22 exception(s).
[ View ]

+++ 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.

  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 ?

Issue tags:-Needs reroll+Needs issue summary update
StatusFileSize
new1.52 KB
new136.58 KB
FAILED: [[SimpleTest]]: [MySQL] 57,850 pass(es), 27 fail(s), and 22 exception(s).
[ View ]
  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.

Issue tags:+Needs reroll

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

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.

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

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.

@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 :/

Status:Postponed» Needs work

Assigned:YesCT» amateescu

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

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.

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

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

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

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

Both blockers are in now!

Hm. So there's still the p1-p9 columns to deal with. #2227441: Step 3: Implement menu links as actual 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: Step 3: Implement menu links as actual plugins (I think that's my favorite option)

@amateescu: what are your thoughts/plans?

#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.

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.

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: Step 3: Implement menu links as actual plugins), I'm perfectly fine with that and this issue doesn't need to be assigned to me anymore.

Assigned:amateescu» Unassigned

I meant to do this.

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?

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