Move the linkset functionality from the decoupled menus contributed module to core's system module

Would it be possible to add this to either jsonapi.module or as a core subsystem, rather than as another separate module? I don't think having an API to support menus on a decoupled site is a standalone feature that a site owner would want to enable or disable separately.

I think it is better to add it to jsonapi.module.

The thing is though that technically it doesn't relate to the jsonapi module. I wouldn't mind maintaining it but the output is a json linkset, not related to the json:api standard.

So I would say, it's a subsystem, not part of jsonapi.

I agree w/ @bbrala. What we've built is unrelated to the JSON:API spec. It's more analogous to the existing /session/token path or the user-related RPC paths like /user/login_status?_format=json.

Until we've finished use case #2 discussed in #3196329: [META] Use cases covered by the decoupled menus initiative and probably resolved by #3194274: Add the ability to configure which responses have menu links added to them, it's an isolated, one-off endpoint and would probably belong in Drupal\system\Controller alongside CsrfTokenController.

I think we ought to see this isolation as indicative a broader issue and as a reason to integrate decoupled systems more deeply into Drupal core. "A JSON:API" is not something to be bolted on, but something that should be out-of-the-box (at least if you choose something like the Decoupled vs the Standard install profile).

Long-term, I'd like to see something akin to Node.php with an api_json route_provider using sub-domain routing to disambiguate JSON:API vs. HTML requests to the canonical URL path (e.g. /node/{node}). Thus, creating nodes creates HTML and JSON:API canonical URLs. With that in place, we could attach menu links (or links to menu linksets) to responses in the same way that SystemMenuBlock gets attached to HTML pages somewhere in the call stack of EntityViewBuilder::view. So long as we serialize menu links with relative paths (i.e. /node/3 vs api.example.com/node/3), this would make support for decoupled path aliases trivial and absent of any weird Drupalisms like forcing clients to add/remove the ?_format=json query string to the URLs they request and to the URLs that they push into the browser history.

</rant> 😅

Edit: I started the subdomain bit in the decoupled_menus module already: #3205938: Make menus that use path aliases easy to consume

Agreed with @bbrala and @gabesullice.

What is this postponed on? 🤓

This might be failing because of this issue perhaps?

#3225328: Improve page performance by sorting cache contexts/tags on-demand

Think this is a flaky test, requeueud

As discussed on the initiative meeting last week, it would be REALLY useful to have a complete issue summary, I see a lot of work is going on but goals, etc are not explained and its hard to provide high level feedback :)

Updating title based on the current proposal and adding reference to the contrib module in the issue summary. Also making the "proposed linkest" format clearer that it is proposed to IETF, but a Drupal proposal per say.

If we're aiming at more headless solutions in the future we should provide standards-based machine-readable and interoperable endpoints.

Have we evaluated JSON-LD or adding a JSON-LD Context to the linkset to make it more interoperable?

We could specify SiteNavigationElement (see https://developers.google.com/gmail/markup/reference/types/SiteNavigatio... & https://schema.org/SiteNavigationElement).

The SiteNavigationElement markup can help increase search engines’ understanding of your site structure and navigation and can be used to influence organic sitelinks.
source: https://searchengineland.com/schema-markup-structured-data-seo-opportuni...

> The format of the response follows IETF's proposed linkset format to avoid creating too many drupalism.

Avoiding drupalisms is good, but the current patch defines the hierarchy of the links using a custom 'drupal-menu-hierarchy' property. Inventing a drupalism for the hierarchy seems like a pretty big deal to me -- the hierarchy is one of the defining properties of a menu.

Is https://datatracker.ietf.org/doc/html/draft-ietf-httpapi-linkset-00 really the best format for this? It says "This specification defines two document formats and respective media types for representing sets of links". But a menu is not a *set* of links -- a set has no ordering or hierarchy. A menu is a *tree* of links.

Is there no better format available than this? (Googling for JSON decoupled menus just brings me back to Drupal...)

The initial work on this was done before I was closely involved with this initiative, but I tried to go back through the original issues to find some additional context.

The issue for the initial use case:
https://www.drupal.org/project/decoupled_menus_initiative/issues/3189459

Which spawned this issue which introduced the use of the linkset format:
https://www.drupal.org/project/decoupled_menus/issues/3196342#comment-14...

And resulted in discussion between @gabesullice, @Deciphered and @nod_ getting into some of the impact of using the linkset format
https://www.drupal.org/project/decoupled_menus/issues/3196342#comment-14...

This probably won't fully address the concerns raised in the most recent comments, but I'm hoping it will be helpful context.

Patch applied cleanly to 9.3.7 and works great!

I've been reading through the documentation, issues, and code from the initiative for past couple days, and have some opinions.

First, it doesn't look like there's a JSON-based standard out there that is better suited for a menu tree. Linkset isn't perfect, but it seems to be the closest fit, and I can appreciate that we're trying reduce the amount of drupalisms. I've seen mention in a few places that by including 'drupal-menu-hierarchy' we're effectively still inventing a new drupalism, or that the attributes are "non-standard" and "custom." I had the same feeling as well till I read the spec:

Extension target attributes are attributes that are not defined by
Section 3.4.1 of [RFC8288] (as listed in Section 4.2.4.1), but are
nevertheless used to qualify links. They can be defined by
communities in any way deemed necessary, and it is up to them to make
sure their usage is understood by target applications.

This tells "me" that they are welcoming community extensions to the spec; that creating a hierarchy attribute isn't a "hack." I'd go even farther by saying we should not prepend the hierarchy attribute with "drupal-menu-", and suggest that Drupal could promote the hierarchy extension to the greater web community as a standardized way to serve JSON based menu structures.

Second, I think the hierarchy attribute format has a blocking issue with the limitation of 1000 menu items per level. I've outlined my arguments on this issue.

#25 is a compelling argument.

> I'd go even farther by saying we should not prepend the hierarchy attribute with "drupal-menu-", and suggest that Drupal could promote the hierarchy extension to the greater web community as a standardized way to serve JSON based menu structures.

+1 to this in particular. Given there's nothing better out there, let's try to extend and enhance rather than drupalize.

Has anyone contacted the group responsible for that spec?

+1 to #25 as well. I think that removing `drupal-menu-` follows the sprit of the spec better.

> Has anyone contacted the group responsible for that spec?

Not that I am aware of.

It appears to be temporary, but the spec has a section for known implementations. We could contact them to to share our work here. I'd be open to taking that on if it helps.

> Second, I think the hierarchy attribute format has a blocking issue with the limitation of 1000 menu items per level. I've outlined my arguments on this issue.

I provided some thoughts on the issue as well.

Removing the `drupal-menu-` prefix from the response, along with potentially some of the options discussed in issue 3204132 also would have an impact on the previously created linkset-menu package: https://www.npmjs.com/package/linkset-menu. I created a quick archive based on an npm install here: https://github.com/backlineint/linkset-menu, and I'd like to give that a permanent home as a general project on drupal.org. I reached out to @gabesullice to see if the original source repo still exists somewhere.

I think I found the source repo for the linkset-menu here: https://github.com/jsdrupal/js-client.
I spent an hour or so last week trying to build it, but ran into some problems. I should have some time to try again later this week.

@johnny5th - I believe https://github.com/jsdrupal/js-client is a separate and inactive/deprecated project. It doesn't seem to have the linkset related code.

Just to add; +1 to #25 as well. Removing the drupal prefix seems like the right thing to do.

Ok, i was looking into getting this and i notices one more thing. If we will remove the drupal- prefix should this also be done for the machine name? I would personally assume not, since that is technically only an helpfull reference. The actual archor already contains the machine name.

@bbrala I was assuming that we'd want to remove the 'drupal-' prefix for the machine name as well. I personally prefer thinking of this extension as a generic representation of hierarchy and machine name data that Drupal happens to be using, rather than something specific to Drupal.

Pushed up changes to remove `drupal-menu-` prefix from related attributes. Made an individual commit for each attribute in case we come to a different conclusion about machine-name.

One thing that caught my eye when making recent updates is that endpoint is automatically exposed whenever the system module is enabled. Was this considered previously? I would have expected this to be opt in (similar to JSON:API) since only a subset of sites will make use of this endpoint.

Reply to #23, #25, #28,

@joachim, @johnny5th and @bbrala

Keeping "drupal-" prefix makes sense.

Regarding the topic about to use "drupal-*" prefix or not, even if we don't use the prefix, we are not going to get rid of Drupalism. The way hierarchy data is encoded is an implementation which is specific to Drupal.

Instead of "what could be" I suggest we look at "what it is" right now.

---

Point A: Developer UX

Any developer (non Drupal developer) looking at the response could tell the following if we use drupal-* prefix

- Ah that thing name start with Drupal, I should probably check for documentation about it in drupal ecosystem
- Searching for the attribute would help you get better results (e.g. "drupal-menu-hierarchy" vs "hierarchy")

Point B: Avoid collision/confusion in case IETF proposal gets extended to handle hierarchy

What if the community outside Drupal decides to bring in a specification where, instead of "hierarchy" they call it "tree". OR the specification says it could be same attribute, but different data type like an "array" for example?

(Custom attributes must be strings based on specs, hence we can't use array, if we wan't to stick to specification, however, is is highly likely that it would be an array if IETF decides to include it in specs. It make sense for it to be an array to avoid deciding on a delimiter )

EDIT NOTE: Based on discussion in slack johnny5th pointed out that array of strings is still valid. Also we already have a patch to support this here.

Point C: Name space within Drupal Ecosystem

- Custom attributes could also be provided via contrib modules in the future, using "drupal-" prefix could at least help avoid those name collisions.
- This also means, if world community decides actually bring a hierarchy specification in the IETF Link Sets spec , somebody could later write a contrib module to support that. Shouldn't matter if IETF decided to called "hierarchy" or otherwise. (Highly unlikely that it would be called "drupal-menu-hierarchy")

---

All my comments are mute if we get this patch into Drupal, after we drive IETF Linkset to include hierarchy as well :) Which I would love to see as well. Does that mean we keep this on on hold until then?

I've asked @johnny5th to create a new branch on this issue fork that contains the changes to remove the existing limitation on the number of menu links outlined in https://www.drupal.org/project/decoupled_menus/issues/3204132

My hope is that will make it easier to review and incorporate here if we decide to move in that direction.

Created a new branch as requested with the changes. Interdiff attached.

Sorry, I attached the wrong patch file.

Thanks for your comments @D34dMan. I can see valid points on each side, but I still personally lean in the direction of removing the prefix.

I know that the two specs aren't really directly comparable, but I was curious if JSON:API did any similar prefixing. Looking at a standard content type I see things like drupal_interternal__nid, and drupal_internal__target_id but no other prefixing. Looking at the attributes added here, machine-name fits that description more closely (although it is not the same type of ID,) but hierarchy feels different since it is being derived for this linkset response.

I also like the idea of thinking of the response here, and the utility that we're developing to parse it (https://www.drupal.org/project/decoupled_menu_parser,) as a generic representation of a linkset that describes a menu. This is probably me being a little too optimistic though - a whole lot would have to happen for this utility to be used with something other than Drupal.

Since I can still see arguments on both sides, the biggest thing for me is probably still that we have had more feedback expressing concern about the Drupal specific prefix than we have had in support of it.

Your point about potential naming collisions is a good one though. I've had it on my list to reach out to the spec authors to alert them to our planned implementation. I'll try to do that as soon as possible.

Setting this back to needs review. Wondering if the attached patch failing was what caused @apaderno to change the status. As I understand it, that patch was intended to show an interdiff - sorry if that caused some confusion.

Currently I think this just needs review. Personally I'm tracking the following three things:

1. Review for the changes that were made to remove the 'drupal-menu' prefix from the response. I know that there has still been some back and forth on this topic, but thus far I have seen more feedback in support of removing it.

2. Consensus around integrating the changes in the `3227824-hierarchy-as-array` branch into the main MR here. Thus far I have only seen support expressed for this change. Unless people come out of the woodwork with strong arguments against this, I'd like to see this change incorporated before we merge this. Integrating this would move it back to 'needs work' but hopefully only briefly.

3. I'd love to hear thoughts on if the current behavior of this endpoint being enabled by default makes sense (https://www.drupal.org/project/drupal/issues/3227824#comment-14442648.) That seems wrong to me, but thus far I haven't heard anyone else share this concern.

I've reached out to the Linkset working group and let them know about our efforts here.

I did hear back from a number of the Linkset spec authors. In general, responses were positive, and they were excited that we were implementing the spec.

One reply encouraged us to use absolute URIs as recommended in the spec. I explained a bit more about the decoupled use case to support why that won't always be possible for us.

Another reply worth noting was:

===

> As shown in the response, we're implementing two extended attributes -
> one representing the menu hierarchy and another representing the machine
> name. If this endpoint finds successful adoption within the Drupal
> community, hierarchy is a concept that we would see value in being
> formally incorporated as an enhancement to the linkset spec.

this is just me responding personally: it seems to me that menu
hierarchy is something that clearly matters for your context, but that
is hard to make sense of as a linkset concept. while rendering links in
hierarchical menus clearly benefits from it, there aren't general or API
linking models out there that support this concept. and personally, i
have a hard time figuring how this could be used apart from the specific
UI rendering use case.

let's keep this conversation open. i am of course happy to see some
adoption. it is just that i am struggling a bit with the idea of how
hierarchy would be useful for the general linkset model. but there very
well may be other use cases that i simply cannot think of right now.

===

Completely reasonable if hierarchy is beyond the scope of the spec from their perspective. But if we can think of additional supporting use cases I can share them.

I've integrated the changes to convert the Linkset hierarchy to array of strings into the main MR branch, and also resolved a merge conflict.

The next (last?) thing that needs to be done here as far as I'm concerned, is making this new endpoint opt in.

Wondering where it would be best to add the configuration for this. Under /admin/config/system since this is provided by the system module, or under admin/config/services?

Would love to see this resolved as part of contribution at DrupalCon.

Thanks for contacting the spec authors. I think that was a really useful thing to do.

I think this part of the response is particularly relevant:

> this is just me responding personally: it seems to me that menu
hierarchy is something that clearly matters for your context, but that
is hard to make sense of as a linkset concept

It's saying that:

a. Whatever extensions we make to the spec are highly unlikely to be accepted upstream into the spec
b. We're using the spec for things it's not designed to do.

I think we should consider this, and the total absence of any JSON menu spec as permission to design our own JSON menu spec (and share it!). We've done our best to find something elsewhere, and it doesn't exist.

A spec designed as an actual *menu* rather than a *linkset* would serve the requirements better, I think. It would mean that the hiearchy can be represented by nesting arrays rather than by properties, which means that clients don't need to traverse the whole thing to re-construct the hierarchy for themselves.

> I think we should consider this, and the total absence of any JSON menu spec as permission to design our own JSON menu spec (and share it!). We've done our best to find something elsewhere, and it doesn't exist.

I'm hesitant to abandon linkset, but also admit that it may be difficult for me to be impartial at this point. But if that did happen, I'd want to invent as little as possible, favoring a solution that was more in line with other core JSON:API endpoints. I assume that was evaluated previously, but I was not closely involved with the initiative at the time.

Updated the issue tag for DrupalCon Portland

After discussing a bit with Brian around this issue I think that the linkset spec is still our best bet. I understand the argument that we might be extending the spec in a way that might not entirely be how the original spec authors envisioned how it should work. But technically it does allow us to do so.

There are two alternatives;

1. Use JSON:API for the menu payload; But i think the verbosity is not really helpful in this context. Also parsing the data into an menu is not that straightforward. Of course a client is possible, but i think it wont really help adoption to (mis?) use JSON:API for that.
2. Create our own format; I really don't feel going back to the drawing board to create our own format is the right way to go if there is something so close to what we need. Defining a complete format is hard.

Basically, I would argue that the proposal and current implementation using the linkset format is the best way to ship this functionality. I don't think we are using the spec in a way it was not meant to so much to have to abandon this path and rethink everything.

I've updated the MR branch to introduce a Menu Linkset Settings config form under Web Services. Here, the menu endpoint can be enabled or disabled. It is disabled by default.

Tests had to be updated to enable the endpoint, and an additional test was added to ensure that the api route does not exist if this new option is disabled.

From my perspective, this was the last change needed for this issue, so this should be ready for a (hopefully) final review.

Helping with coordination of getting this together in the decoupled menu initiative. Also helping with the conversation at DrupalCon Portland and getting people together.

Assuming the tests are green, which they were locally here in Portland, I'd consider this good. :) Great work @brianperry!

I've been thinking about this a little more. There should be a way to fix the event subscriber. I'll have a look.

I've tried, but i see no way to use injection in the event subscriber.

I committed a very small comment fix, which i assume doesn't exclude me from RTBC. So back to RTBC it is.

I added a couple of review comments to the MR - I think there might be some dead code in the issue.

Queued a retest which passed :) yay! Thanks @laurii

The removal has been done as requested by @nod_. Also the merge of 9.4.x-dev has not affected files within this MR so that seems like it makes sense.

When tests are green, all is great <3

Hopefully we don't get a random test failure on quickedit this time ;)

Left a review on the MR

Needs work for at least the permission and the missing tests for the settings form

Posted some questions on you comments @larowlan so the changes would be good right away hopefully ;)

Thanks @larowlan, think that clears up how the feedback can be fixed.

Pushed up a couple of commits that should address the recent round of feedback. Updating status to needs review.

Think there is one unresolved comment by larowlan about the double save for the linkset

Sorry - forgot to add a comment about the $link_content->save(); It seems to be required for some reason. With it removed the necessary links don't seem to be created and three of the tests in LinksetControllerTest fail.

Reverted the most recent commit in order to resolve test failures.

Then i feel we can go back to RTBC. Appearanly it is needed.

Updated the settings form test to use fieldExists as requested.

I've checked. All but one calls was changed. I don't think the last one needs fixing since that seems like its not a field.

Setting RTBC again, thanks for the work @brianperry :)

Took another look at this - see comments in the MR but in summary:

* we need a config/install version of the config (default to false for the enabled flag)
* we need a post update hook to create the new config object (defaulting to false for the enabled flag)
* we need an update path test for the update hook

Pushed up an update to the MR that includes a config/install version of the menu endpoint config and an update hook to set this for existing sites. I also added a test for the update hook following the approach in core/modules/system/tests/src/Functional/Update/DeleteAuthorizeSettingsUpdateTest.php The catch is that I wasn't able to run that test locally, nor am I able to run any similar UpdatePath tests. It seems to die either when importing the database dump, or when it first tries to use the site:

1) Drupal\Tests\system\Functional\Update\DeleteAuthorizeSettingsUpdateTest::testSystemAuthorizeRemoval
Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: You have requested a non-existent service "file_system".

I pushed up my new test in a separate commit. If it fails in CI as well I'll revert it.

Test does appear to be running in CI, but it needs some adjustment. Pushed up an update.

Tests passed in CI after some adjustment. Setting status to needs review.

The requested config/install has been added with the default false. The test tests pre-installation state, post-installation state and default setting. This is looking good.

Only found a ubernit in system.post_update.php s/settings/setting/g

Saving issue credits for folks who shifted the patch direction in some way.

I think we could use some #description text on this checkbox to explain what enabling it will do.

I think we should put this one before the usability team at a future meeting.

In manual testing I got this error

The website encountered an unexpected error. Please try again later.
Error: Call to undefined method Drupal\Core\Cache\CacheableJsonResponse::create() in Drupal\system\Controller\Linkset->process() (line 87 of core/modules/system/src/Controller/Linkset.php).

Drupal\system\Controller\Linkset->process()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 161)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 79)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 58)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle() (Line: 670)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Other than those two items, this is looking great and I'm keen to get it into 10.1.x asap.

@larowlan

> I think we could use some #description text on this checkbox to explain what enabling it will do.

That is a good idea. I added this to the agenda for the next usability meeting. https://www.drupal.org/project/drupal/issues/3299065

> In manual testing I got this error

Had a little trouble duplicating this, but my guess is that I'm not seeing it on this branch because it is currently based on 9.x, but you're seeing it because you applied the patch to 10.x

If these api docs are to believed, there was a major change in CacheableJsonResponse between 9.x:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21C...

And 10.x:

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Cache%21C...

I'll see if I can find a related issue, or changelog for that to understand how that is supposed to be handled in 10.x. In the meantime if you or others have guidance that would be helpful.

Looks like we were depending on a deprecated Symfony method https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/HttpFo... that was removed in Symfony 6. That would explain why this error is being thrown in 10.0.x, but not 9.x

I've pushed up a change to the MR branch that changes to use __construct() instead.

Possible draft copy for the checkbox description text (pulled from https://www.drupal.org/docs/develop/decoupled-drupal/decoupled-menus/dec...):

"Enabling the menu linkset endpoint will make menu data available at /system/menu/[menu name]/linkset, where "[menu name]" is the machine name of the menu."

Why not:
"Enabling the menu linkset endpoint will make menu data available at /system/menu/[menu machine name]/linkset."

Less text :) And the other one kinda reads like 'menu menu menu menu' in my head. ^^

x-post:

I am adding a "User interface changes" section to the issue summary, with the screenshot form #85.

I am not even sure where that shows up in the admin UI.

Before I bring this up at a usability meeting, I would like to have some draft text. I do not know if this is accurate, but we have to start somewhere:

Make menu data available in JSON format at /system/menu/{menu}/linkset.

If someone adds a stub documentation page, then we might provide a link. If not, then we might add a second sentence, explaining that this is intended for decoupled sites.

If someone who understands this issue would like to join the meeting, then we can talk about it without advance preparation. Today's meeting starts in 30 minutes. See the #ux channel in Drupal Slack.

I do not think we need to start the description with "Enabling the menu linkset endpoint", since that pretty much repeats the label.

We discussed this issue at #3299065: Drupal Usability Meeting 2022-07-29. That issue will have a link to a recording of the meeting.

For the record, the attendees at today's usability meeting were @AaronMcHale, @Antoniya, @DyanneNova, @brianperry, @rkoller, @shaal, @simohell, @worldlinemine, and me.

In the usability meeting we concluded that the description text should be:

See the [decoupled menus documentation](link) for more information.

As for the link itself, during the meeting we were referring to the decoupled menus overview page: https://www.drupal.org/docs/develop/decoupled-drupal/decoupled-menus/dec... but I realized that there is another option - the parent Decoupled Menus page: https://www.drupal.org/docs/develop/decoupled-drupal/decoupled-menus

I think https://www.drupal.org/docs/develop/decoupled-drupal/decoupled-menus is the better option as it is more concise and will serve more of the possible use cases of users looking for more information.

[edit] fixed a copy and paste issue above.

Core normally links UI like to the handbook pushes, not issues in contrib

@larowlan that was a copy and paste mistake. I previously edited the comment above and corrected the link.

No worries

Updated the MR with the agreed upon description text. Setting issue back to needs review as a result.

Created a branch that re-rolls the patch on 10.1.x since I'm assuming we'll need this at some point - linkset-menu-10.1.x

The issue fork doesn't pass tests (or better, the pre-tests).

Think needs review was not set by accident.

There is still a super nit present in the code, but seems everything is in order now. I can RTBC, but we need the 10.1.x MR to be openened for that since that should be the new target from what i've read in multiple discussions.

Removing "Needs usability review" since that has been done.

Added a patch for Drupal 10.1.x and also addressed this thread on MR "Super nit, this is a single setting, not multiple settingS"

Added interdiff between MR and patch.

Re-rolled this as 10.1.x-patch and opened an MR: https://git.drupalcode.org/project/drupal/-/merge_requests/2663

@ravi.shankar thanks for the patch - I made one small change, there were duplicate route_callback keys in system.routing.yml somehow.

Setting back to needs review.

A couple issues with the tests on 10.1.x to resolve.

Tests are passing again for 10.1.x - one of the fixtures we were depending on moved. Setting back to needs review.

Seems all requested changes have been resolved except my little nit, which is fine to ignore. But... It seems there was an extra few files committed by accident.

tmp/db.sqlite
tmp/db.sqlite-test97456245

I don't think those should be in here.

What I tested this iteration:

1. Linkset disable works
2. update does run and insert setting
3. Nested linkset works
4. External link works
5. Can access multiple menu's without a problem.

Offending files have been removed. Sorry about that!

Update looks right. Tests green. Rtbc again.

Left a couple of minor nits

Addressed feedback from @larowlan - setting back to needs review.

It seems like this is actually done. I would love to see this committed. I have created a first change record with the basic information on this change. One things that still needs work is the documentation. The current documentation still has the old format for nesting which needs fixing.

I think this could be arranged in a follow up I think so i opened #3310954: Update decoupled menus documentation.

RTBC i think :)

Thanks @bbrala! I noticed that issue with the documentation as well today. My hope is that we can also make the necessary documentation updates this week as well. We also have the following issue to finalize the docs: https://www.drupal.org/project/decoupled_menus_initiative/issues/3263181

Just added some review points in gitlab. I think this is looking really close - nice test coverage and I like how the functionality is optional. Nice work.

Updated the MR addressing @alexpott's feedback.

I'm shaking here. Went through the code, looking good especially after the micro fixes. I've tested basic functionality again manually on my machine, which work as expected.

o_O RTBC

Ran tests locally and found they failed because it was using the Symfony request object which assumes localhost and does not respect SIMPLETEST_BASE_URL. To fix this I've moved a JSON API test trait into core... because also we'd be unable to use xdebug in the tests and this fixes that. All my recent changes are in the test system and unrelated to the runtime feature. It'd be great to have another round of review.

The code changes look great all as expected. The added trait also is a good improvement. The change to jsonapi does mean we gonna need to see green tests to be sure. Set a reminder, will RTBC when green.

I combed through the changes and did have few pieces of feedback, could be follow ups I think.

Ok, that testfail is REALLY weird... When I run locally (php 8.1, mysql 5.7) tests are green.

23 Sep 2022 at 17:47 CEST was green. the one after wasn't.

So basically, since now localhost is not the url it runs on a whole url and there is a subdirectory in there? I have no clue.

Core tests are always done in a sub directory :) - will work on a fix.

Ah, then it makes sense:)

I've fixed the test by adding the subdirectory on as necessary. I'm not sure if this is correct on not. I think it makes sense that the URLs include GLOBALS['base_url'].

While thinking about this I started to ponder if and how multilingual worked and then saw that we're lacking multilingual tests - like how would you requests menus in different languages. I think we should add some multilingual test coverage too. It'd be great if someone (probably not me) could have a look at adding that.

Hi @alexpott,

I think we should add some multilingual test coverage too.

I had worked on this test https://git.drupalcode.org/project/decoupled_menus/-/blob/1.0.x/tests/sr...
so maybe I can work on this.

Idea would be to create some multilingual menu items in 'es' and then request them via `base:/es/system/menu/main/linkset`, would that work?

Probably. I'd personally do a quick umami install and test it out.

Hmm, didn't awnser your question it seems. Yes I think something like that would be great. Perhaps add, what you get when you requests without a language in the url. Is that the default (preferably not English I guess).

I'll keep an eye out for a review :)

Manually tested the output by creating a few main menu items as follows,

Followed more-or-less following documentation for setting up menu link translation https://www.drupal.org/docs/multilingual-guide/menu-translation-in-drupal-8.

Following languages were added to the system. Language detection is based on "path" prefix.

- English
- Malayalam (default)
- Hindi
- Spanish

Created Menu items on main menu as follows,

- Home ()
- EN Art B (/node/2)
- (en) Art A (/node/1)

node/1 and node/2 entries were prefixed with language code name for easy identification.

After this, visiting menu linkset endpoint for various languages based on path prefix were observed.

OUTPUT.

Language "ES"
https://drupal.ddev.site/es/system/menu/main/linkset

Language "EN"
https://drupal.ddev.site/en/system/menu/main/linkset

Language "default" (ML - malayalam, was configureed to be the default language )
https://drupal.ddev.site/system/menu/main/linkset

@D34dMan really nice manual testing - do you think it'll be possible to add an automated multilingual test? Having at least on e multilingual test will make sure we don't break functionality and if multilingual bugs are found it'll make them quicker to solve and add tests for in the future.

In slack by d34dman:
Updated the comments on D.O. after few hours I will try to convert that process into test :slightly_smiling_face:

@alexpott,

I guess i was able to more or less automate what I did manually, and run that specific test using following code in terminal

 ./vendor/bin/phpunit -c core core/modules/system/tests/src/Functional/Menu/LinksetControllerMultiLingualTest.php

The commits are available here https://git.drupalcode.org/issue/drupal-3227824/-/commits/drupal-3227824...

I was expecting it to update some old MR automatically. But that didn't happen, so do I go about creating a new MR against 10.1.x branch?

(I don't mind if anybody else could do that for me as its quite late for me now)

And thanks bbrala, brian and alex for helping me out :) you guys are awesome!

Addressed a couple of Drupal CS issues of the MR.

Great work guys! Test are a very very good start. I've attached some bits of feedback to the MR which would finetune them to be close to perfect.

Got to fix the test from subdirectory thing...

fixed test for subdirectory

There is some follow ups we need when this get merged. But that's OK. I've gone through the changed made by @D34dMan and those cleaned the MR up nicely, well done! This is looking great. I'd personally prefer a data providers over fixtures, but i dont think that is really something we need to change.

Great work, this looks like it is done to me. RTBC!

Follow ups I feel we need:

1. We introduced a feature flags config object, are there other places where we can use this?
2. We introducted ApiTestTrait to replace duplicated code for doing requests in decoupled menus and jsonapi.
   • This needs replacing in Rest also.
   • I would also prefer not to rename the methode in JsonApiTestTrait

Thanks for writing the multilingual tests. I've reviewed them and by playing with them I think we need to do a little bit more work on the underlying code to deal with the multilingual case. The most important bit of the review is about what happens when I remove the 'cc' version of the menu link item.

If I remove the 'cc' from the array and re-run the tests, it fails. The problem is how it fails - the linkset that does not match looks like:
'href' => '/d8/cc/node/1'
'title' => 'aa|A multi-lingual-node'
So we have cc link and aa node title. I'm not sure what I expect to happen in this case but I'm pretty sure what is happening is wrong.

The behaviour is consistent with how Menu works right now.

---
Test Setup (based on Comment#129)

- Created a node (node/3) in default language (ML)
- Went over to menu admin and added new link in the menu.
- Added a translation for menu item in English
- Visiting the main menu as anonymous user in "HI", displays the link with title from "english" (Please note that english is not the default language of the site :/)
- Also the link points to '/hi/node/3'

- Added another translation for menu item in spanish
- Visiting home page in "HI" as anonymous user still shows the link title from English translation.

---
Summary : The behaviour of linkset endpoint is consistent with how Drupal menu is displayed.

---

@alexpott,

Question : Do we add more test to ensure this behaviour (which seems odd) Or let it slide?

I think if we deem this behaviour consistent with the menu system then we should from the cc menu link and test that we conform to this expectation. I wonder what the multilingual menu tests say we should expect.

I think if we deem this behaviour consistent with the menu system then we should from the cc menu link and test that we conform to this expectation. I wonder what the multilingual menu tests say we should expect.

Thanks again for quick response. I will add a new test link in the setup update the fixtures accordingly.

While writing test, the title returned for missing translation appeared to be random.

During initial run, i was getting the title in language 'aa', and later i am getting title from language 'dd'. So am obviously not writing a good test.

While checking the implementation for where the title is being picked up, I noticed we have a followup issue regarding that as well https://www.drupal.org/project/decoupled_menus/issues/3280735

@see Linkset::toLinkTargetObjects

---

Am wondering if I should compare the link, title and hierarchy against a menu that is rendered on a page :P

Am stuck, need help and guidance please.

Addressed Drupal CS issue and rebased the #146 MR as that was not mergable.

I also continue to be stuck here. I'm able to duplicate the inconsistent results during failing tests that @D34dMan and @alexpott are seeing. Not only is the title wrong, it is seemingly random. Manually testing an equivalent use case (creating translated menu links, leaving one untranslated, and requesting the menu endpoints in a browser) I see expected results.

I tried looking for similar tests, but am struggling to find something comparable. JSON:API doesn't seem to have a similar test. Also can't quite find something comparable in the multilingual tests. Many entity related translation tests, but struggling to see something similar to menu items.

I still don't believe that this is something specific to or introduced by this endpoint, but I also can't prove it. Not sure where to go from here.

Ok, I did a deep dive into why this was happening and found out why.

When a menu link with content is called upon the MenuLinkContent handles this request. This plugin asks the language manager if the site is multilangual in then decides it wants the content from the true entitiy to build the title

if ($this->languageManager->isMultilingual()) {
  return $this->getEntity()->getTitle();
}

The getEntity method does some magic to get the right entity from the storage, which is great. And ends up getting the translation of the entity based o nthe content.

$the_entity = $this->entityRepository->getTranslationFromContext($this->entity);
/** @var \Drupal\menu_link_content\MenuLinkContentInterface $the_entity */
$this->entity = $the_entity;

This is great, since this will go through translation manager and try and get the proper translation. When a translation is not found it will try and find the best alternative. This is done by getting an array of the available languages and looping through them, returning the first one that has a translation.

        $context['data'] = $entity;
        $context += ['operation' => 'entity_view', 'langcode' => $langcode];
        $candidates = $this->languageManager->getFallbackCandidates($context);

        // Ensure the default language has the proper language code.
        $default_language = $entity->getUntranslated()->language();
        $candidates[$default_language->getId()] = LanguageInterface::LANGCODE_DEFAULT;
        
        // Return the most fitting entity translation.
        foreach ($candidates as $candidate) {
          if ($entity->hasTranslation($candidate)) {
            $translation = $entity->getTranslation($candidate);
            break;
          }
        }

This is where things go wrong. When MySQL gets a set of data without a set order, this could be any order. This means return is inconsistent, and since there are multiple alternative languages in the test this breaks.

To be sure of the order of the alternatives it is REALLY important to set the weight of the languages, since that is in the end the way to be sure how things are returned. This is a simple fix!

First we create the languages with the correct weight, making the default language the top result:

    foreach (['dd', 'aa', 'bb', 'cc'] as $index => $language_code) {
      ConfigurableLanguage::create([
        'id' => $language_code,
        'label' => $this->randomMachineName(),
        'weight' => $index,
      ])->save();
    }

Then we have languages set in the right order. (sidenote: whats interesting though is that by default english is installed. We are not using it in these tests, but it is available. Perhaps this is the reason why in most language tests i've seen real languages are used.)

We also need to make sure that the fallback is configured, since that is normally how things work if you install Drupal. For this we need to do 2 things. First off, remove the old initialisation of the setting:

Remove this code:

    // Enable URL language detection and selection.
    $edit = ['language_interface[enabled][language-url]' => '1'];
    $this->drupalGet('admin/config/regional/language/detection');
    $this->submitForm($edit, 'Save settings');

    // Check if we can change the default language.
    $this->drupalGet('admin/config/regional/language');
    // Change the default language to a custom one.
    $edit = [
      'site_default_language' => 'dd',
    ];
    $this->submitForm($edit, 'Save configuration');

And then setup multilanguage as done in the language tests in other parts of core. Explicitly configuring the different parts.

    // Enable URL language detection and selection.
    $this->drupalGet('/admin/config/regional/language/detection');
    $this->submitForm([
      'language_interface[enabled][language-url]' => TRUE,
      'language_interface[enabled][language-selected]' => TRUE,
    ], 'Save settings');

    // Set prefixes to en and fr.
    $this->drupalGet('/admin/config/regional/language/detection/url');
    $this->submitForm([
      'prefix[aa]' => 'aa',
      'prefix[bb]' => 'bb',
      'prefix[cc]' => 'cc',
      'prefix[dd]' => '',
    ], 'Save configuration');

    $this->drupalGet('/admin/config/regional/language/detection/selected');
    $this->submitForm(['edit-selected-langcode' => 'dd'], 'Save configuration');

Now that that is set up. We need to make sure we have a node without a translation, so we remove cc from the translations created.

    foreach (['aa', 'bb'] as $language_code) {
      $multi_lingual_menu_item->addTranslation($language_code, [
        'title' => $language_code . '|' . 'A multi-lingual-node',
      ]);
      $multi_lingual_menu_item->save();
    }

Now we run tests and we should see failures! But expected failures that make sense. The multilangual test should fail since cc returns the default language, and the default should fail since the default language now maps to /.

Then you should changes the expected result and dance around a little since everything is finally working is expected :)

Also added a small piece of feedback in the MR regarding the controllername, its inconsistent with the rest of the controllers in system.

Adding casey to credits, he worked with me on my travels through the translation system.

@D34dMan found this issue which is apparently exactly what is confusing here right now: #2951294: Sort out and fix language fallback inconsistencies

@bbrala,

I have implemented almost all from Comment#150 with a slight exception. I kept language `aa` with lowest weight. Rationale being, thats how the core behaves.

Let me know If i should set `dd` to be the one with lowest weight.

I couldn't find the reason why we are seeing `/node/x` path instead of path alaises. This seems to be un-related to the addition of weights into the test.

While testing various scenario, I added test for `internal` routes as well.

When I introduce the following code back,

    // Change the default language to a custom one.
    $this->drupalGet('admin/config/regional/language');
    $edit = [
      'site_default_language' => 'dd',
    ];
    $this->submitForm($edit, 'Save configuration');
    $this->rebuildContainer();

Then the test starts to use `multi-lingual-node` path, but it then starts to use prefix `dd` for default as well. Please let me know if it make sense to do so.

Made a few updates to address lingering feedback. Tests are passing and when adjusted to fail, failing consistently. This should be ready for another round of review.

So happy the multilingual issues are resolved. Had a proper look at the changes and checked all threads if they have been resolved. I think we are ready again. :fingers-crossed:

When this lands we should create a followup to use ApiRequestTrait in \Drupal\Tests\rest\Functional\ResourceTestBase.

One of the threads on the MR still needs a better answer for me as @bbrala's reply implied it has been fixed but as far as can I see the behaviour is the same.

I've tried to explain a little more on why I think the last open issue is not a problem.

Also opened the required follow up (posponing on this), see #3320670: Refactor rest\Functional\ResourceTestBase to use ApiRequestTrait .

Ran through the failure scenario a few more times and thought I was on to something, but it turned out to be a dead end :(

When set to fail, the current branch will fail like so:

                 1 => Array &7 (
                     'href' => '/cc/node/1'
-                    'title' => 'cc|A multi-lingual-node'
+                    'title' => 'aa|A multi-lingual-node'
                     'hierarchy' => Array &8 (
                         0 => '1'
                     )

Based on what @bbrala explained this is the 'expected' behavior given the weights that are being set.

However, earlier in the thread, @bbrala suggested the following:

First we create the languages with the correct weight, making the default language the top result:

    foreach (['dd', 'aa', 'bb', 'cc'] as $index => $language_code) {

It doesn't look like we're doing that today. The array currently is `['aa', 'bb', 'cc', 'dd']` and that is why it is failing with the results shown above. @alexpott - is ensuring that it fails with the default language in this case more in line with what you're expecting here? My brain admittedly does expect it to fall back on what the tests consider as the default language. Changing that feels a little like us shaping the data to match our expectations rather than the unintuitive (imho) current core behavior. But perhaps that change would make this a little easier to reason about if there are actual failures in the future.

If not that, is there anything else within our control here that we could change that would make this case more in line with what we expect?

I think we're good here we're discussed this quite a bit and the consensus is that this is how core works. We've definitely put the effort in to try understand what is going on. I would accept another rtbc here :)

i feel the same. There have been no changes since last RTBC, so setting it back to RTBC <3

Committed 10ea35d and pushed to 10.1.x. Thanks!

Congratulations on getting this issue done! It has been more than 3 months since we reviewed this issue at the weekly usability meeting.

I had a quick look at the change record (CR), and I think it would help to make this more explicit:

It is now possible to enable the linkset endpoint in site settings under Webservices

Can we add the menu path and/or a screenshot? I think (but I am not sure) that "site settings" refers to /admin/config/system/site-information. That page now has the title "Basic site settings", not just "Site settings". (When did that change? I missed it.) I guess there is a new "Webservices" section on that page.

The CR also has a link to a documentation page, but I do not see this information there. I must be at least one more click away from finding it.

Does this mean https://www.drupal.org/project/rest_menu_tree is no longer needed?

I read #164 and then read the change record. The change record does need work as described in that comment. I am adding the tag and setting this back to needs work for the updates to the change record.

I have made the followup and removed the tag, #3322640: Use ApiRequestTrait in \Drupal\Tests\rest\Functional\ResourceTestBase.

Updated the CR.

1. Added screenshots of the settings.
2. Updates the documentation to show how to enable the endpoint.
3. Added an example of the output of the module.

@bbrala, awesome! Thanks. The CR is much easier to understand now!

I am restoring the Fixed status.

@davidwbarratt the format of the response will be different, so I don't think this is a 1:1 replacement. But I think a note on the project page encouraging users to consider this endpoint instead in 10.1 could make sense.

Glad to see this in and documented; thanks all.

I forgot to remove the 'needs change record update' tag. Doing so now.