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.
Comment | File | Size | Author |
---|---|---|---|
#193 | 1842858-165.gz | 41.82 KB | alexwriter2003 |
#4 | 1842858-menu-link-entityng-4.patch | 29.02 KB | dsnopek |
#5 | 1842858-menu-link-entityng-5.patch | 39.3 KB | dsnopek |
#14 | entityng-menu-link-1842858-14.patch | 39.23 KB | shanethehat |
#17 | menu_link_ng.patch | 34.31 KB | amateescu |
Comments
Comment #1
Gábor HojtsyThis is very needed to support translate menu titles (and paths). :)
Comment #2
fagoadding to #1818580: [Meta] Convert all entity types to the new Entity Field API
Comment #3
dsnopekI think I'm going to give this one a shot at the sprint tomorrow. Assigning to myself!
Comment #4
dsnopekPosting 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!
Comment #5
dsnopekI 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!
Comment #6
Gábor HojtsyThis is very needed to support translate menu titles (and paths), so I'd love if people can help by taking this on!
Comment #7
BerdirLet'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.
Comment #9
Gábor HojtsyTagging.
Comment #10
Gábor HojtsyOpened #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.
Comment #11
andypostProbably options could mimic some from #1818560-61: Convert taxonomy entities to the new Entity Field API pathField
this hunk unresolved
how to deal with dynamic properties?
language needs definition, there's
LanguageItem
suppose better to change form element, probably out of scope
options...
Comment #12
xjmComment #13
xjmComment #14
shanethehat CreditAttribution: shanethehat commented#5 no longer applies. Rerolled, still generates many errors.
Comment #16
BerdirTagging. Not a field blocker, but still needs to be converted, which is using the same tag.
Comment #17
amateescu CreditAttribution: amateescu commentedHere'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.
Comment #18
BerdirYes, 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.
Comment #19
amateescu CreditAttribution: amateescu commentedHere'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.
Comment #20
dsnopek@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. :-)
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedRaising 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.
Comment #22
amateescu CreditAttribution: amateescu commentedEven 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.
Comment #24
BerdirOk, 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!
Comment #26
BerdirAh, 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.
Comment #28
BerdirOk, the alter thingy there messes it up. Commented out for now.
Comment #30
dcrocks CreditAttribution: dcrocks commentedShouldn'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?
Comment #31
BerdirThis 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.
Comment #32
BerdirRe-roll and fixing hopefully most of these exceptions, few actual fails, though.
Comment #34
BerdirAnother batch of exception fixes.
Comment #35
plachWrong patch :)
Comment #36
BerdirD'oh. Wrong folder, just picking the latest files without reading them ;)
Comment #38
smiletrl CreditAttribution: smiletrl commented'string_field' -> 'map_field' :)
Comment #39
xjmThis is an approved API change per #1983554-8: Remove BC-mode from EntityNG (tagging on behalf of @webchick).
Comment #40
BerdirOk, 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.
Comment #42
jair CreditAttribution: jair commentedComment #43
larowlanFirst a re-roll
Comment #45
larowlanWe're not getting p1-p9, plid populated in {menu_link} table, digging as that will resolve a lot of fails.
Comment #46
larowlanThis 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).
Comment #48
larowlanWhoops
Comment #50
pwolanin CreditAttribution: pwolanin commentedThis is going to have overlap with #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit, though maybe not horrible
Comment #51
pfrenssenRerolled.
Comment #52
smiletrl CreditAttribution: smiletrl commentedsee the latest patch performance.
Comment #54
pfrenssenFixed the invalid PHP syntax.
Comment #56
herom CreditAttribution: herom commentedAssigning to myself. I will post an updated patch very soon.
Comment #57
herom CreditAttribution: herom commentedThis should fix the installation failure. Let's see how the tests will do.
Comment #58
pwolanin CreditAttribution: pwolanin commentedI 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
Comment #60
herom CreditAttribution: herom commented#57: menu-link-ng-1842858-57.patch queued for re-testing.
Comment #62
herom CreditAttribution: herom commented#57: menu-link-ng-1842858-57.patch queued for re-testing.
Comment #64
herom CreditAttribution: herom commentedImproving the patch a bit.
unset($menu_link->options->value['<key>'])
didn't seem to work, and generated a lot of exceptions: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.
Comment #65
herom CreditAttribution: herom commentedComment #67
smiletrl CreditAttribution: smiletrl commentedRegarding
Field has made use of __get() and __set(), so you get the Notice. Maybe you could try something like this:
For reference, see Field::setValue
Comment #68
smiletrl CreditAttribution: smiletrl commentedAlso, 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:)
But, based on the documention block here, if you REALLY need to get rid of poor "unset()", I think we could use
Just as the doc says, we don't want redundant serialized data:) And, an empty string will still triger the serializtion for this property.
Comment #69
herom CreditAttribution: herom commentedHere'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']
).Comment #71
BerdirThanks 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.
Comment #73
herom CreditAttribution: herom commentedupdated 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.
Comment #75
herom CreditAttribution: herom commentedupdated patch. this one should fix most of the fails.
Comment #77
Berdir#75: menu-link-ng-1842858-75.patch queued for re-testing.
Comment #78
das-peter CreditAttribution: das-peter commentedFYI: I'll take a look at this right now at the extended sprint in Prague.
Comment #79
das-peter CreditAttribution: das-peter commentedI'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.
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 ofDrupal::l()
?Is this a re-roll that went wrong?
'%link_path' => $menu_link->link_path
should be'%link_path' => $menu_link->link_path->value
Comment #80
herom CreditAttribution: herom commentedupdated 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.
Comment #82
herom CreditAttribution: herom commented#80: menu-link-ng-1842858-75-80.patch queued for re-testing.
Comment #84
herom CreditAttribution: herom commentedunassign-ing myself, so @das-peter can work on this.
Comment #85
das-peter CreditAttribution: das-peter commentedTried 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.
Comment #87
das-peter CreditAttribution: das-peter commentedNext attempt.
Comment #89
herom CreditAttribution: herom commentedjust 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.
Comment #91
das-peter CreditAttribution: das-peter commentedNext bunch of small fixes.
Comment #92
das-peter CreditAttribution: das-peter commentedNext bunch of fixes,
Comment #94
dsnopek#92: menu-link-ng-1842858-91.patch queued for re-testing.
Comment #95
das-peter CreditAttribution: das-peter commentedAnd 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.
Comment #96
das-peter CreditAttribution: das-peter commentedRe-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.
Comment #98
das-peter CreditAttribution: das-peter commented#96: menu-link-ng-1842858-96.patch queued for re-testing.
Comment #99
aspilicious CreditAttribution: aspilicious commentedIsn't needed anymore
Comment #100
das-peter CreditAttribution: das-peter commentedRemoved
use Drupal\Core\TypedData\Annotation\DataType;
from MapItem.Comment #102
das-peter CreditAttribution: das-peter commentedThe failed test passes locally. I give the test-bot another go.
Comment #103
das-peter CreditAttribution: das-peter commented#100: menu-link-ng-1842858-100.patch queued for re-testing.
Comment #105
das-peter CreditAttribution: das-peter commented#100: menu-link-ng-1842858-100.patch queued for re-testing.
Comment #106
BerdirCross-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.
Comment #107
Berdir#100: menu-link-ng-1842858-100.patch queued for re-testing.
Comment #109
BerdirRe-roll after EntityNG => ContentEntityBase landed.
Comment #110
aspilicious CreditAttribution: aspilicious commented:)
Comment #111
herom CreditAttribution: herom commentedjust a tiny comment cleanup on patch #109. no need to run the whole test again.
Comment #112
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #113
BerdirOk, 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!
Ok, so menu_link_localize() still gets both. What's going to happen with that function?
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.
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?
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?
Can we switch title here to getLinkTitle() that's what we set it to in the code above. Or could something possibly change it?
Wondering what will happen with the access stuff in long-term and how it's separate from $link->access() (the entity access method9
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.
Do we actually still check the updated property?
$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?
Valid fix but unrelated, we change nothing else in that class. Fix in a separate issue?
@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.
All the methods in here need to us @inheritdoc or get proper documentation if they aren't inherited/overridden.
This uses $this->properties to store the values. Usually, they are in $this->values and $this->properties contains the instantiated property classes, when requested.
book.module is refactored to no longer integrate with the menu system so closely. Hopefully that will avoid things like this.
This shouldn't really be necessary, the old code should work the same. Alternatively set('plid', NULL)
Missing documentation.
@inheritdoc.
entity_form_submit_build_entity() is gone. The content entity form controller no longer attempts to write arbitrary stuff.
Does this really return an array? Shouldn't it be getValue() ?
This code path doesn't make sense. save() never return FALSE. it returns updated/new flags and throws an exception on error.
unnecessary change.
@inheritdoc.
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.
@todo comments need to be indented two spaces on the second and following lines.
Can we use the method here for plid?
This could also use the method.
@amateescu is working on the shortcut/menulink stuff, how does this affect this code?
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.
oh and yes, these test changes are clearly wrong, the old format was correct.
Comment #114
herom CreditAttribution: herom commentedupdated 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.
Comment #115
Berdir13. 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.
Comment #117
herom CreditAttribution: herom commentedreroll.
Comment #118
herom CreditAttribution: herom commentedand back to needs work, based on #113 - #115.
Comment #119
herom CreditAttribution: herom commentedreroll again.
I'll post a patch soon. let's test this reroll in the meantime.
Comment #121
herom CreditAttribution: herom commentedif this passes, then points 2, 13, 15, 19 (from comment #113) are fixed.
Comment #123
herom CreditAttribution: herom commentedtrying again.
Comment #125
herom CreditAttribution: herom commented#123: menu-link-ng-1842858-123.patch queued for re-testing.
Comment #127
herom CreditAttribution: herom commentedthis should be green.
Comment #128
Berdir$this->get('route_parameters')->offsetGet(0)->getValue() should work.
Comment #129
herom CreditAttribution: herom commented#128 fixed.
Comment #130
herom CreditAttribution: herom commentedreroll, after #2095399: Merge DatabaseStorageController and DatabaseStorageControllerNG
Comment #131
herom CreditAttribution: herom commentedupdating MapItem for #2023563: Convert entity field types to the field_type plugin.
Comment #133
herom CreditAttribution: herom commented#131: menu-link-ng-1842858-131.patch queued for re-testing.
Comment #135
Berdir#131: menu-link-ng-1842858-131.patch queued for re-testing.
Comment #137
tim.plunkettI can't reproduce the fail locally, but it needed a reroll anyway (just conflicts on use statements)
Comment #139
herom CreditAttribution: herom commentedalso move MapItem to its new place.
Comment #140
herom CreditAttribution: herom commentedoh, bad interdiff.
Comment #142
herom CreditAttribution: herom commented#139: menu-link-1842858-139.patch queued for re-testing.
Comment #144
smiletrl CreditAttribution: smiletrl commentedDoes anyone get this error?
I installled the latest Drupal core-> clear cache -> apply latest patch at #139 -> go to any page -> get above error.
Comment #145
herom CreditAttribution: herom commented@smiletrl I think it should be "apply patch #139 -> install Drupal". can you test this way too?
Comment #146
smiletrl CreditAttribution: smiletrl commented@herom hah, yeah, thanks -- I missed that:)
Comment #147
smiletrl CreditAttribution: smiletrl commented#139: menu-link-1842858-139.patch queued for re-testing.
Comment #148
markie CreditAttribution: markie commentedAll
I tried a clean install (new database, new files folder) after applying the patch and received:
Comment #149
herom CreditAttribution: herom commentedrerolled. this issue needs more love... and more reviews!
@markie I couldn't reproduce the error you mentioned.
Comment #151
herom CreditAttribution: herom commented#149: menu-link-1842858-149.patch queued for re-testing.
Comment #152
saltednut*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.
Comment #153
markie CreditAttribution: markie commentedFolks
I was able to replicate my issue again, using patch 149 on a fresh clone, on a second machine.
Steps:
Comment #154
herom CreditAttribution: herom commentedanother 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.]Comment #155
pfrenssenNeeds a reroll again!
Assigning to me for rerolling duty. Also going to test the installation with this patch applied.
Comment #156
pfrenssenRerolled.
Comment #157
pfrenssenInstalled the patch on the latest 8.x HEAD and installed the site:
Installation works fine, and saw no problems after a quick look through the site. @markie, can you test again with the latest HEAD + patch?
Comment #159
herom CreditAttribution: herom commented@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 inBookBreadcrumbBuilder.php
.to everyone else: the patch to review is at #154 right now.
Comment #160
pfrenssenI wonder what I have been smoking. Thanks for the correction!
Comment #161
YesCT CreditAttribution: YesCT commentedI can do a review. (other reviews welcome)
Comment #162
YesCT CreditAttribution: YesCT commenteddo we still have to do this (use the description if the title and description are the same)?
maybe the title can be translated now?
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)
Comment #163
YesCT CreditAttribution: YesCT commentedon 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.
Comment #164
YesCT CreditAttribution: YesCT commentedis 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?
why temporarily use $localized_attributes? why not directly set on $data['link']->localized_options->attributes ?
Comment #165
YesCT CreditAttribution: YesCT commentedthis 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.
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).
why do we have to make these changes to _menu_navigation_links_rebuild()?
this changes item from a router item to a menu link?
#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.
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.
Comment #167
msonnabaum CreditAttribution: msonnabaum commentedWas going to profile this, but it looks like it needs another reroll.
Comment #168
BerdirWill 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.
Comment #169
amateescu CreditAttribution: amateescu commentedJust 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
Comment #170
pwolanin CreditAttribution: pwolanin commentedThe 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.
Comment #171
Gábor Hojtsy@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 :/
Comment #172
effulgentsia CreditAttribution: effulgentsia commentedUnpostponing because #2142549: Support multi-column field types in base tables landed.
Comment #173
Berdir@amateescu said he has a plan for making this happen ;)
Comment #174
amateescu CreditAttribution: amateescu commentedYep, the first step is #2054011-7: Implement built-in support for internal URLs. So.. postponing again I guess.
Comment #175
xjmSee #2084421-34: Phase 2 - Decouple book module schema from menu links; does that issue block this one?
Comment #176
Gábor Hojtsy#2054011: Implement built-in support for internal URLs is not moving at all. Is that still a blocker for this? Any other blockers?
Comment #177
xjmComment #178
YesCT CreditAttribution: YesCT commented#2084421: Phase 2 - Decouple book module schema from menu links is in
Comment #179
amateescu CreditAttribution: amateescu commentedAnother issue which will impact restarting work here has been opened: #2207893: Convert menu tree building to a service.
Comment #180
jessebeach CreditAttribution: jessebeach commentedComment #181
tim.plunkettBoth blockers are in now!
Comment #182
effulgentsia CreditAttribution: effulgentsia commentedHm. 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?
Comment #183
amateescu CreditAttribution: amateescu commented#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.
Comment #184
dawehnerWell, 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.
Comment #185
amateescu CreditAttribution: amateescu commentedExcept for that call, I wasn't there so I can't know :)
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.
Comment #186
amateescu CreditAttribution: amateescu commentedI meant to do this.
Comment #187
effulgentsia CreditAttribution: effulgentsia commentedPostponing 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?
Comment #188
amateescu CreditAttribution: amateescu commented@effulgentsia, it can be done as a non-beta-blocking followup, but not while there are other patches moving code around in this area..
Comment #189
jessebeach CreditAttribution: jessebeach commentedComment #190
pwolanin CreditAttribution: pwolanin commentedTrying to do this via #2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module with a slightly different approach
Comment #191
effulgentsia CreditAttribution: effulgentsia commentedFrom #2256497-28: [meta] Menu Links - New Plan for the Homestretch by Dries:
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.
Comment #192
YesCT CreditAttribution: YesCT commented#2256521: [META] New plan, Phase 2: Implement menu links as plugins, including static admin links and views, and custom links with menu_link_content entity, all managed via menu_ui module is doing this for sure. so it is ok to stay closed.
Comment #193
alexwriter2003 CreditAttribution: alexwriter2003 commentedThanks YesCT