Suggested commit message
Issue #2301239 by pwolanin, dawehner, Wim Leers, effulgentsia, joelpittet, larowlan, xjm, YesCT, kgoel, victoru, berdir, likin, and plach: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage.
The commit list includes everyone who has contributed a patch or a commit in the sandbox repo which was incorporated into patches here or on #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
Problem/Motivation
This issue is the first step of #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. For a full background, read the summary of that issue.
Menu links in Drupal may be defined in any number of ways:
- In code (as for the administrative links for a module, local tasks, and local actions).
- As configuration (when site owners override the administrative links for a module, changing their weight, position, etc.)
- As content (when site administrators create links in menus of their site).
All these different types of menu links need be rendered together in menus, so they need to present the same API to developers.
Proposed resolution
This patch adds a new interface for interacting with menu links, as well as a new plugin manager for managing menu link plugins of all types (including static links, menu links created by editors as wel as links defined by views, etc.).
The patch is only code additions (it does not yet make any changes in the user interface nor convert existing code), so it should be reviewed for its architecture. It adds the following:
- Four services: a menu link manager service, a menu cache service, a menu tree storage service, and a menu link override service (for example keeps track of changes like moving a menu that a module provided as a plugin definition (in yml))
- A menu link plugin interface, base class, default implementation, and a content menu link implementation (for adding new custom links in the UI).
- An implementation for static configuration overrides of menu links.
- A menu link plugin manager
- An API for interacting with menu tree parameters (depth, parentage, etc.), as well as an interface for the storage of the menu tree data and a default implementation.
- A menu_link_content.module that allows administrators to create menu links.
- Some test coverage for menu_link_content.module. (much more test coverage comes in later parts. Note that a combined patch with all parts is passing tests.) See #2256521-144: [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 for an endorsement of the test coverage.
- A fix for PluginManagerPass so that it does not add cached discovery for plugin managers like the MenuLinkManager that do not implement
\Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface
.
This patch is worked on in a sandbox:
git clone --branch 2256521-step1-2301239 http://git.drupal.org:sandbox/dereine/2031809.git menu_links
When updating the patch, be sure to create interdiffs. Changes will be applied to both the 2256521-step1-2301239
and 2256521
branches of the sandbox.
Remaining tasks
Task | Novice task? | Contributor instructions | Complete? |
---|---|---|---|
Update the issue summary | Novice | Instructions | done |
Add automated tests | Instructions | done | |
Improve patch documentation or standards (for just lines changed by the patch) | Novice | Instructions | done |
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards | Instructions | done | |
File followup issues for @todo | Novice | done | |
Change notice will be done at step #4 or 5. Draft at https://www.drupal.org/node/2302069 | drafted | ||
Doc changes linked from change notice will be done at step #4 or 5 at https://www.drupal.org/node/2118147 | drafted |
User interface changes
None
API changes
None - only API additions
#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 a >600k patch. Trying to break it up into manageable chunks. This is chunk 1.
Comment | File | Size | Author |
---|---|---|---|
#76 | Responses to xjm on step1 2301239.pdf | 46.05 KB | pwolanin |
#67 | 2301239-67.patch | 158.4 KB | pwolanin |
#67 | increment.txt | 7.39 KB | pwolanin |
#67 | 2301319-the-full-kahuna-9.patch | 628.81 KB | pwolanin |
#54 | 2301239-54.patch | 158.23 KB | pwolanin |
Comments
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedComment #3
effulgentsia CreditAttribution: effulgentsia commentedComment #4
kim.pepperI had a quick scan and only found a wrong class in the MenuLinkManager constructor.
Comment #5
xjmSo that there's an easy warm-up novice task for sprinters to start fixing right off the bat: All these one-line summaries need to be adjusted to conform to the standards: https://drupal.org/node/1354#classes and https://drupal.org/node/1354#functions
Carefully read the class, function, or method, and write a one-line summary describing what it does that follows the standard. (What's there already is probably a good start.) The word "helper" is not really needed/meaningful and shouldn't be used in the one-line summary. (However, it is a good flag that maybe the method should possibly be protected? Check in each case before making that change.)
Be sure to provide an interdiff when creating a new patch. dawehner will be pushing a branch to the sandbox shortly with just the patch for this issue.
I'll have some more meaningful review and improve the issue summary a bit in an hour or so. :)
Comment #6
xjmAll these @todos should be reformatted slightly to match our standards:
https://www.drupal.org/node/1354#todo
They should start with a capital letter, there should be no hyphen, they should be the last thing in any docblock other than an annotation, the subsequent lines should be intented by two characters, they should wrap before but as close as possible to 80 characters.
Most importantly, though, each @todo should either be fixed in the patch, or should have a followup issue to address it. Make sure the link to the followup issue is referenced in the @todo, and also add this as the parent issue on the followup issue itself. Work with @pwolanin or @dawehner to file followup issues about the problem (issues that will be clear to someone not familiar with this patch) or to fix them in the patch if it doesn't make sense to file a followup.
Comment #7
xjmLet's also update the issue summary of this issue to explain the self-contained architecture we're adding in this patch a bit. Another sprinter could take this on. Start by looking at the parent issue and getting an idea of the high-level from there, then explain what this patch adds and what's important about it.
Comment #8
dawehnerPushed a branch called
2256521-step1-2301239
to the sandbox. All changes should be applied to the branch as well as the main one:2256521
Comment #9
dawehnerComment #10
dawehnerComment #11
kgoel CreditAttribution: kgoel commentedYesCT and I am going to work on doc and ToDo lists and see if we can work on it and open other issue if we need to.
Comment #12
xjmI next reviewed the @param @return and @throws docs. Kind of rushed at the end a bit but there's plenty here.
This should indicate that it is optional and defaults to TRUE.
Is there a reason we don't provide a default for $persist here?
This protected method is missing parameter documentation.
This protected method is missing return value documentation.
This protected method is missing parameter and @throws documentation.
And what happens when it's null? Does it count links for all menus?
A sample array structure would be helpful here.
I don't think specifying @code/@encode inline like this works for api.d.o. I also don't know what this means.
I guess this second instance clarifies it a little. I'd say "or an empty string to use the real root" instead.
Looks like the argument is optional and defaults to NULL. The docs should be updated to indicate that. And what happens when it's null?
What does "1 or 2" mean here?
This protected method is missing parameter documentation.
I think our standards indicate that MenuTreeParameters needs to be fully namespaced here, even if it's the current namespace or used in the file.
It's not clear which class these methods are on (the MenuTreeParameters parameter or this class). Should "::loadLinks()" simply be "this method"? Also, let's put the appropriate class name on ClassNameHere::doBuildTreeData().
These could use more detail. Where should I look to see what a "menu tree" array looks like? What is the structure of the array of accumulated definitions (which is updated by reference, it appears)? And why is it being passed by reference?
Same as above. Let's add the class name here, and also add a proper @see at the bottom of the docblock.
These protected methods are missing return value documentation.
"The menu link definition, or FALSE if..."
what definitions? Is it MenuLinkInterface plugin definitions? (Everywhere we talk about definitions we should clarify that; see multiple other places in the patch.)
Weird capitalization. Also, what definitions? Is it MenuLinkInterface plugin definitions? (Everywhere we talk about definitions we should clarify that; see multiple other places in the patch.)
Wait a second, I've seen this "1 or 2" before. Is something else somewhere duplicating this documentation that should be @inheritdoc?
Looks like it is optional and defaults to NULL. And what happens when it's NULL?
Also, nitpick: It looks like this might be 81 characters. (I've noticed this a couple other places as well but forgot to comment.) Double-check.
What happens if it's NULL? Also I'm having déja vu again... is this documentation repeated somewhere else?
Or NULL if.... what?
When is it NULL? If it's an internal link?
What route parameters? What is the structure of the array? Reference what the structure of this array is somewhere, or some other documentation for it. Also missing a period.
Looks like "The route provider" on the @return description is not correct.
Comment #13
pwolanin CreditAttribution: pwolanin commentedNote - this issue includes a bug fix to PluginManagerPass because it was trying to add a method call addCachedDiscovery even through the MenuLinkManager does not implement \Drupal\Component\Plugin\Discovery\CachedDiscoveryInterface
so, even though this looks unrelated, it's needed as part of the patch.
Comment #14
Anonymous (not verified) CreditAttribution: Anonymous commentedI'm also reviewing this...
Line 204 of MenuLinkManager.php has an incorrect doc:
201 /**
202 * {@inheritdoc}
203 *
204 * @return \Drupal\Core\Menu\MenuLinkInterface
205 * A menu link instance.
206 */
... but we cannot inherit and add additional items at the same time due to the way docs are generated.
Comment #15
victoru CreditAttribution: victoru commentedSmall change to fix one comment
Comment #16
pwolanin CreditAttribution: pwolanin commentedAdded #15 to the sandbox branch
Comment #17
xjmThanks @victoru!
Comment #18
xjmSo, this patch adds:
Is all of that correct?
Another question:
Why is this change necessary here? Edit: #13 totally answers this:
Comment #19
pwolanin CreditAttribution: pwolanin commentedWe are making a lot of progress at the sprint - please check with me in IRC if you are looking at this.
Comment #20
xjmComment #21
YesCT CreditAttribution: YesCT commentedYep. kgoel and I are (with guidance from pwolanin) going through some of the cleanups xjm posted earlier.
Comment #22
xjmHm, this docblock actually looks entirely wrong? It's an interface.
Comment #23
xjmMiscellaneous other review/questions:
So, I had to read this method several times before it made sense. The updateLink() and createLink() methods on the plugin manager aren't actually creating a menu link item -- the CRUD operation that invoked this already did that. What they are doing, if I understood @effulgentsia's explanation, is "pushing" their new or updated definitions onto the plugin manager, integrating the content link plugin definitions with definitions discovered in yaml etc. and allowing them to be materialized together with those links. This way the whole discovery doesn't need to be re-done every time a content link is added. (Menu links don't use derivatives for their definitions because of the sheer quantity of definitions that are needed, which is orders of magnitude greater than e.g. custom block definitions.) Right?
I wonder if there's some way we could improve both the documentation of this method and the class docblocks for the manager to explain this a little better, since I would have been completely at a loss if @effulgentsia hadn't explained it. Also, I think the method names are maybe misleading? Like it seems like they should be createLinkDefinition() updateLinkDefinition() etc., or something, to disambiguate it from actual CRUD operations done by the content menu link module or other implementations.
It seems odd for a public method to be explained as "useful for testing"? OTOH I guess a method needed for testing might have to be public... but it still seems odd for that to be the only reason the methods exist. Maybe I'm overthinking it.
I don't see a corresponding decodeId() method? Why is that never needed? I guess because we never need to go from an override to the original link, since an override can't exist without an original link?
Also, what happens when an ID actually contains a double underscore? Seems like a totally legitimate case that would make overrides blow up. Can/should we throw an exception if there's a double underscore since it would be incompatible with this?
Given the issues that have cropped up with dots, it seems like this should have test coverage somewhere. Mostly the patch provides interfaces and plugin functionality that maybe doesn't make sense to test on its own, but I'd say at least an integration test is in order once the configuration system is actually exercised in step 4. (Need to check whether that specific test coverage is already there or not.)
The exception for
__
would want test coverage too, if it's appropriate.Per discussion with Dries, let's change this to "Custom Menu Links".
Nitpick: This should end in a period.
Minor, but I think the description from the .info.yml is better and could be reused here.
If it builds the menu link definition, why is it called
getMenuDefinition()
?Comment #24
pwolanin CreditAttribution: pwolanin commentedre: #12.18 the definition are not actually specific to menu link plugins - I think the storage could be potentially used for books as well.
re: #12.19, the similar doxygen is on a protected menthod on MenuTreeStorage
re: code/endcode, there are lots of example in core:
Comment #25
xjmYes, and look how the rendering for that is goofed up:
https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/functi...
@code/@endcode are not just like
<code>
tags; they make a code block.Comment #26
xjmHere's an interdiff showing what @kgoel, @YesCT, and @pwolanin have improved so far in the sandbox.
Comment #27
xjmComment #28
xjmA few nitpicky fixes. Meanwhile:
It throws... something! :)
Under what circumstances would there be no route?
Comment #29
pwolanin CreditAttribution: pwolanin commentedRE: #23.1 regarding createLink() and updateLink()
these truly do create and update menu link plugins, and are used when the plugin implementation does not participate in discovery.
However, based on discussion renaming to addDefinition(), updateDefinition(), and removeDefinition()
Comment #30
YesCT CreditAttribution: YesCT commentedWe think all the things brought up in #5, #6, #12, #14, #22, #23 1., #25, #28 have been updated (in the sandbox). A patch (and an interdiff) coming here soon.
#23 2. Is fine the way it is. They need to be public, and the comments are accurate and useful about ... how they are useful. :)
#23 3. about the __ encode thing. fixed by also replacing __ with ___ (which works since that makes three turn into four and four turn into six etc.)
#23 4-7 also are addressed.
Comment #31
pwolanin CreditAttribution: pwolanin commentedIan made draft change notice - we'll need to put most of the detail in the handbook, however.
Comment #32
YesCT CreditAttribution: YesCT commentedupdated issue summary to update the proposed statements in #18, with some corrections. it's correct in the issue summary now.
Comment #33
pwolanin CreditAttribution: pwolanin commentedstarted change notice (need this for step 4) https://www.drupal.org/node/2302069
Comment #34
pwolanin CreditAttribution: pwolanin commentedHere's an incremental diff since the 1st patch at #2 and the #1 patch relative to 8.x and the full patch for step #5
Comment #35
xjmSo the first half of my probably-final review; down through MenuLinkParameters. I've tried to indicate with parentheticals what comments needn't block an RTBC/commit (though feel free to clean them up if they're simple, particularly the docs nitpicks). For the stuff I haven't indicated that way, though, it would be good to post a reply on issue if appropriate.
I'll do the second half of the patch in a bit.
This seems odd. If we only care about a flat list of definition names, why don't we return a flat list of definition names? Is it to avoid having to use
in_array()
? (Doesn't need to block commit; just curious).Edit: I found why; added to the snippet. Because
$new_definition_values
is keyed by those key names, and so that works for the intersect here. It seems a little magical/hacky but I'm not too concerned.Shouldn't this be @inheritdoc? (At first I was going to ask why it wasn't on the interface, but it is.) :)
Where are
title_context
andtitle_arguments
documented?Also, I don't entirely follow the inline comment. It doesn't seem related to the code that follows it.
Is it saying that specific implementations might rely on the request or specific attributes to define the title? (Along the lines of the examples given in the interface docs, i.e., a link title might include dynamic information).
Edit: I found where the rest of the definition is documented, inline in the manager
$defaults
. I think that's a pattern used elsewhere (documenting the definition inline in the defaults), though I think we might want to find a better way to document all of it.... which would be okay as a followup.However, even leaving it as is, there still is no documentation of
title_context
ortitle_arguments
here, even though there is for the rest of the definition keys. :)Not a big deal, but: any particular reason we are falling back to
sprintf()
here rather thanString::format()
? (On my mind because of #1825952: Turn on twig autoescape by default, not that it actually would make a difference if this string gets escaped or not because I don't think plugin IDs can contain anything that would get escaped to begin with.)Or maybe the answer is "We're deliberately not adding a dependency on
String
. But I'm not sure that makes sense since plenty of other Drupal/Core/ classes do (including ones in this patch). :)I'm wondering if a better name for this implementation might be StaticMenuLink? Would be more parallel with StaticMenuLinkOverride, which is closely related (we even inject the service and provide a default list of allowed overrides.)
Maybe an inline comment above this
array_intersect_key()
would be good? "Filter the list of overrides to only those that are allowed."So this is where we have the behavior of silently ignoring disallowed overrides, and where we would throw an exception if we decided that was better DX. What's the usecase for letting calling code pass in disallowed overrides and silently ignoring them?
(Okay as a followup) Should this throw an exception if the link is not updateable, like we throw an exception if it's not deletable?
(Okay as a followup) Should we throw a less generic exception here than PluginException?
(Okay as a followup) This would be easier to understand with sample code; is there an implementation we could add an @see for?
I think these should be "Returns route name and parameters", not just "parameters"?
Also, the word "link" in the summary is ambiguous... I think we are talking about the link to delete a link, or the link to translate a link, etc.? If so maybe this would be more clear:
Or something? What do you think?
Why are these integers instead of boolean?
(Docs standards nitpick) ID, not id.
(Docs standards nitpick) Missing period at the end of the line.
No, it constructs a MenuLinkManager object. Right?
(Docs standards nitpick) "They" is ungrammatical; it should maybe instead be:
Why the string casting? To turn NULL into empty string, or? Comment wouldn't be amiss.
So, hm. Is there a reason we're silently ignoring invalid plugin definitions here?
(Okay as followup) I think this should probably use
String::format()
instead of printing the variable straight in the string. (Also see comment elsewhere aboutsprintf()
use in an exception message.)(Docs standards nickpick) Typo: "meu".
La vache dit meu!
This is all much easier to understand. Should updateDefinition() have a similar note about being used for plugins not found through discovery?
Also, I still think a paragraph on the interface docblock explaining that plugins can use these three methods as an alterative to participating in discovery would be good.
Finally, maybe the interface summary should say "managing menu links and storing their definitions" rather than "creating"?
Is it worth mentioning here that it includes applying overrides? (Which it appears to in the main implementation.)
I thought I pointed this out before, but maybe not. Is it "The route parameters, if any"? I.e., not all routes have parameters.
What would happen if I passed a route name for a route that needed parameters, but no parameters? (Maybe this isn't a relevant question.)
(Docs standards nitpick) This is going over 80 characters.
(Okay as a followup) Same question; do we want a less generic exception?
So, I can totally imagine myself using these methods in a module that does fancy things with menus, not just for testing. Or would I be abusing the API if I did?
This docblock is hugely helpful. +1.
(Docs standards nitpick) This docblock is missing a one-line summary. I think we could probably just insert the blank line between the first sentence and the rest of the paragraph.
(Docs standards nitpick) These also need one-line summaries, as above.
Comment #36
YesCT CreditAttribution: YesCT commenteddidn't need a fix: #35 1
fixed: #35 2,
#35 3: the array that defines menu links is defined in two places,
on the $defaults property on MenuLinkManager, and MenuTreeStorage. both of those places link to #2302085: Keep MenuTreeStorage field definition in sync with MenuLinkManager which is to figure out how to keep them in sync (but not necessarily how to make them documented and obvious.) Some suggestions were:
a) it would be nice if php allowed a const that was an array, then we could put it on the interface and reference that.
b) we could make a method that returns the array
c) .. I forget.
This is a pattern we use already in core, so if we make an issue to figure out how to make the bits documented, we should then apply *that* pattern to the other places this default property on classes is, like: LocalActionManager
I didn't change anything here.
#35 4. why using sprintf instead of String::format()...
I'm not sure, but I will say:
ag "throw .*Exception" core/modules | grep format | wc -l
40
ag "throw .*Exception" core/modules | grep sprintf | wc -l
7
ag "throw .*Exception" core/lib/Drupal | grep format | wc -l
57
ag "throw .*Exception" core/lib/Drupal | grep sprintf | wc -l
100
I didn't change anything here.
----
moving locations. back in a bit.
Comment #37
xjmThanks @YesCT.
My point is though that at a minimum we should add an inline comment explaining what
title_context
andtitle_arguments
are, like there already is for the other definition keys, because nothing anywhere gives the developer a clue what their purpose is. The docmentation-in-defaults pattern exists elsewhere, but we are documenting some but not all of the definition keys. That's a docs gate concern.Edit: also the point about the inline comment at the top of the hunk still stands, I think. The method's code needs to be better documented, as do the title definition keys used in it.
Comment #38
xjmPhew. Just reviewed the storage code. Heavy stuff. I'll review the remaining two implementations in a bit.
(Docs nitpick) Not a big deal, but I guess this should probably be "Provides a menu tree storage using the database."
Have I mentioned yet that I really like the way we've abstracted the storage?
I read every line of this class but didn't do that much code-parsing or deep review of some of the protected methods, since I am sure this is mostly taken from our existing menu management API.
So how about, for now, we add an @see to MenuLinkManager::$defaults for an explanation of the keys?
I think maybe the second sentence of this inline comment might now belong inside
purgeMultiple()
(if it's even still relevant)? Can someone clarify? Because the code here certainly isn't doing what that sentence says by itself.Also, ixnay on the word "controller", I think. We've (mostly) made "controller" refer only to page/form controllers which doesn't make sense in this context. What is it that actually does the re-parenting?
Huh. This seems like a weird method to have on this specific storage; it's super generic. Why do we need it? And if the answer to that question makes sense, should there be a followup to move this to something more generic in the DB API somewhere?
So the docs are better here now, but I still want to know why it's only 1-2 names. :)
(Okay as followup) Why? Could use an inline comment. (Assuming the code works because HEAD and the combined patch). ;)
Ah, so this might be related to my earlier question of why the defaults for these were ints instead of booleans.
(Docs nitpick) "Boolean" should be capitalized (it's from a dude's name).
Uh. Moves the children where?
(No change needed) OT: I heard one of these today. ;)
Why does
load()
but notloadMultiple()
return early with definitions from$this->definitions
?I also notice that we're not statically caching the loaded links like we do in a lot of entity storages. Probably it's a case of "Don't add static caches until you have proof you need them" but I thought it was worth pointing out.
I think the one-line summary here for
loadFullMultiple()
should probably have the same clarificationloadFull()
does (that it's all table fields, not just those in the definition)?(Docs standards nitpick) "ID", not "id".
(Docs standards nitpick) There is a missing word after the semicolon: "that's how ignore the rest of the tree."
(Okay for followup) Same question about whether we should use a more specific exception type.
What does "in active development" mean in this inline comment?
So I believe I wrote this docblock so it's my fault (there wasn't one before), but I think we should explain a little more what a "menu tree storage" is here.
Same question as before about the route parameters array.
What kind of a thing is a fully built menu tree? Array? Object implementing interface FooInterface?
Whoa. An example array would be really helpful here.
(Docs standards nitpick) "rooted by of".
(Docs standards nitpick) "The the".
(Docs standards nitpick) I think this should be "A specific menu name" (not "named")?
Comment #39
xjm(No change needed) So all the overrides get stored in a single config file, correct? I think this makes sense but it also might be good to have @alexpott spot-check this.
What's the reason for silently ignoring bogus override keys? (It's probably fine; just want to check whether we should be throwing an exception instead.)
Oh, I get it now. This is clever.
It probably wants explicit test coverage at some point (probably in step 4?). I guess we can add "Needs tests" on that issue with a note about it.
(Docs nitpick) This should have a one-line summary.
I think this should probably be changed to "Custom menu link" as well? (Correct?)
(No change needed) Whoa. They're fieldable? Whoa. Neat.
(No change needed) Interesting that the canonical link is the edit link. I guess it wouldn't really make sense to have a page to view a single menu link entity. ;)
The inline comment confuses me. The two lines following seem to not agree with it? I mean, we're not "saving" anything here either way.
(No change needed) OT: Huh. Surprised that this still isn't fixed. :(
(Okay as followup) Why magic integers? :(
(No change needed) Yes, +1 for these docs.
(Okay as followup) Why isn't this injected?
(No change needed) Interesting. So this is presumably to allow contrib to do whatever with content links?
(Okay for followup) Exception. Generic. :)
(Docs nitpick) I guess "UUID" should probably be uppercase in text, same as ID.
(No change needed) For the most part I just assumed the tests are fine rather than trying to execute tree logic in my brain. (Look at my faith in you!)
So, this needs to go away now, and be updated for: https://www.drupal.org/node/2301125
And that's all! Once all of these things from #35-#39 are addressed, I will RTBC this. ("Addressed" here might mean: fixed in patch, followup filed, or simply documenting on issue "this is fine as-is").
Comment #40
kgoel CreditAttribution: kgoel commentedI am adding all the follow up issues which I created for the todo's and all the follow up issues are child of https://www.drupal.org/node/2256497 [meta] Menu Links - New Plan for the Homestretch
Comment #41
YesCT CreditAttribution: YesCT commented#35 5 rename MenuLinkDefault to StaticMenuLink?
I feel like there could have been something about default being able to be used for more than just "static"...
@pwolanin clarified that MenuLinkDefault is better than StaticMenuLink since
a) if you don't provide a class it's the defualt
b) it's parallel with other link things: Drupal\Core\Menu\LocalActionDefault, Drupal\Core\Menu\LocalTaskDefault
#35 6 and 7, An exception when the intersect of the values to change and values allowed to change seems like a good idea #2302581: When intersect of menu link new definition values attempts to update those not allowed to be overridden throw an exception but might be too complicated.
Added the inline comment about what that is doing also. More discussion should be on 2302581. Might rescope it.
Comment #42
YesCT CreditAttribution: YesCT commented#35 17 #2302623: Do something instead of silently ignoring invalid plugin in MenuLinkManager (also adding another related)
Comment #43
pwolanin CreditAttribution: pwolanin commentedWe are tracking responses to xjm in a spreadsheet
See PDF version below
Comment #44
dawehnerWell, this executes ensureTableExists() so it is not that generic. Added an issue for a trait: #2302635: Add a schema ensuring trait
Well, ANSI SQL does not really define a boolean type, so small integer is used.
I bet this is not done anywhere else.
I think it would be okay to cache them statically.
At least in the menu tree storage it is NOT a plugin so I would just not catch other exceptions TBH.
Given that the schema will not change for MenuTreeStorage this method could just hardcode all serialized fields. This method
was quite helpful during the development of all that code.
Yes it does. Note: static links are mostly admin ones, so there aren't that many overrides needed. On top of that this makes it possible to use a config schema.
At least for now, maybe someone comes up with a more clever idea. Rest would probably though like a route without an '/edit' suffix.
This is actually wrong. For translatable titles/descriptions we do use them. Removing the comment.
I do agree that we should: #2302641: Use constants instead of magic ints for expanded/hidden in MenuLinkContent
Just changed it. Not even worth to wait on all the HTTP requests of drupal.org
Comment #45
xjmSweet, @dawehner already took care of almost everything. Taking care of some of the last few points.
No pretty tables needed, just so long as we document the things that aren't obvious here. :)
:)
Comment #46
xjmComment #47
xjmFrom @pwolanin re: #35.22:
I pushed several more cleanups. Only a few things left that I'm not sure how to address, so I made them bright yellow in the spreadsheet.
Comment #48
xjmComment #49
xjmComment #50
YesCT CreditAttribution: YesCT commentedI think was removed in a cross post accidentally.
Comment #51
YesCT CreditAttribution: YesCT commentedFor #35 9.
#2302849: Add a code example to documentation to MenuLinkInterface::getFormClass()
Comment #52
pwolanin CreditAttribution: pwolanin commentedThis should have all the requested fixes and the increment since the last patch
Comment #54
pwolanin CreditAttribution: pwolanin commentedtrivial conflict in core.services.yml
Comment #55
YesCT CreditAttribution: YesCT commentedAdd the points so far have been addressed. the google spreadsheet was helpful.
I looked at the changes made by others overnight and they look great too.
Comment #56
YesCT CreditAttribution: YesCT commentedtypo
Comment #57
xjmAlright, all 104 (!) of my points of feedback have been addressed, from architectural questions to unresolved @todos in the code, to docs gate issues down to tiny typos. 104 thank-yous especially to @pwolanin, @YesCT, @kgoel, and @dawehner for their patience and help getting this patch ready for commit.
I think the overall architecture makes a ton of sense; menu link plugins are an excellent solution given the varied implementations that exist in core, and they also offer a lot of opportunity for contrib to do interesting things. I also think the abstracted, decoupled, materialized menu tree storage is a great solution that allows materializing links from multiple sources together in a consistent, easily queried way that also will support other storage backends.
Be sure to see the suggested commit message; many people worked on this who didn't post patches here!
Issue #2301239 by pwolanin, dawehner, Wim Leers, effulgentsia, joelpittet, larowlan, xjm, YesCT, kgoel, victoru, berdir, likin, and plach: MenuLinkNG part1 (no UI or conversions): plugins (static + MenuLinkContent) + MenuLinkManager + MenuTreeStorage.
Comment #58
xjmComment #59
xjmComment #60
xjmUh. :)
Comment #61
alexpottThis looks really good. Some tiny comments.
This is confusing because parent refers to the parent plugin id.
This should be
FieldDefinition::create('uri')
for semantic correctness.Minor nit - these are booleans let's set their default values to booleans.
Why are we so cautious here - if it is not set something has gone wrong in the constructor no? Or is it possible that the entity id has changed?
Very minor nit. Are we loading links or entities? Seems inconsistent.
This means we're expecting a mix of empty strings and nulls in the table. This is potentially confusing. In my mind the only values that should be in this column are valid urls. An empty string is not that.
Comment #62
alexpottVery very minor - not used.
Comment #63
xjmFollowup: #2303113: Explicitly document all instances of calling t($variable)
Comment #64
xjmAlso see in the plugin definition defaults -- they are being defaulted to ints there as well.
Comment #65
alexpottHere are some other thoughts that occurred whilst reading the patch - not actionable within the scope of this patch AFAICS.
This is a new config file - I was told in IRC that the schema file is coming in a later patch.
We should file a follow up to just loop through all the services and check the interface and remove the tag - less to know.
Is this really the canonical representation of a menu link entity?
We need to make sure #2303113: Explicitly document all instances of calling t($variable) covers this instance too
Comment #66
pwolanin CreditAttribution: pwolanin commented@alexpott re: #61.6
The 'uri' field is created with a NOT NULL schema definition, so we will have empty strings in some cases, so I think the code is correct as-is to give us consistent return values from the method.
re: #65.3 - the Translate tab appears next to the "canonical" route page. I can't think of a useful canonical view of the data other than the edit form that would also appear with Edit and Translate tabs visible on it.
@xjm re: #64. No, I don't think that makes sense.
They are saved to the DB as ints and loaded that way from the storage. So, it doesn't make no sense to default the definition to booleans. The field definition is a "boolean" and we handle the conversion to int when building up the plugin definition from the entity field values.
Comment #67
pwolanin CreditAttribution: pwolanin commentedHere's a patch incorporating the requested changes other than those addressed in #66, plus incremental diff and the full part5 patch with all the changes (to show it's all still working together).
Comment #68
effulgentsia CreditAttribution: effulgentsia commentedI'm not entirely convinced by #66 (any of the 3 points), but I also think those are very minor issues. If that's all that's left, I think this can be RTBC again. The increment in #67 looks great.
Comment #70
aspilicious CreditAttribution: aspilicious commentedSo this is fixed now with the referenced commit?
Comment #71
Wim LeersAFAIK, yes. It's indeed in the 8.x branch.
Next step: #2301273: MenuLinkNG part2 (no UI or conversions): MenuLinkTree API and unit tests :)
Comment #72
alexpottComment #73
Wim Leers:D
Comment #74
dawehnerThere should be a lama version of cowsay.
Comment #75
Gábor HojtsyYay, amazing. Closed down #1966398: [PP-1] Refactor menu link properties to multilingual as duplicate, since the new menu_link_content module is multilingual.
Comment #76
pwolanin CreditAttribution: pwolanin commentedHere's a PDF version to memorialize the responses we made to each point from xjm
Comment #77
xjmTagging the child issues retroactively.
Comment #78
xjm