Convert MenuLinkContent to use a link widget

Can't help with the issue summary update, but since this is a corollary to #2406749: Use a link field for custom menu link it should inherit the tag.

we are going to try and get this done.

Here is do not test patch without #2417333: Add support for user-path: scheme to Url class for review.

#2417333: Add support for user-path: scheme to Url class just got committed so this probably needs some adjustment.

I will manually test. and add screenshots.

there is slight changes in the help text which I think is ok.
before:
The path for this menu link. This can be an internal Drupal path such as node/add or an external URL such as http://drupal.org. Enter to link to the front page.

after:
This can be an internal Drupal path such as node/add or an external URL such as http://drupal.org. Enter to link to the front page.
The location this menu link points to.

but strange reorder in show as expanded and enabled checkboxes.

there is a field set around the one with the patch. which I think is fine.

+++ b/core/modules/menu_link_content/src/Entity/MenuLinkContent.php
@@ -261,7 +261,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
-      ->setDescription(t('Shown when hovering over the menu link.'))

Why are we removing this? But, no need to hold this up on that. Can always add it back later if needed, the rest of the patch looks good, and getting this in will help with #2417423: Re-process the user-entered-paths for custom menu links when there is a menu rebuild and #2416987: Fix UI regression in the menu link form, so RTBC.

I tested the patch manually and everything seems to work properly. Also fixing #17.

Well, adding this description cripples the UI a bit more. If you are fine with it, sure, that is fine.

ah, that description was in head.
nice catch. we should keep it and not make that change accidentally.

updating issue summary with UI changes and new screenshot.

do we need to add any change to change record: Configurable link field, short cut, menu links store user entered paths as URI (not routes or paths): https://www.drupal.org/node/2417421 or a different change record?

On the UI, things look a bit backwards/redundant for me. Looking at the very handy widgetorderfix.png file in #21, I see:

• Link fieldset
• URL label just below, within the fieldset

The name of the page is already Add menu link and the first field is Menu link title. We know we're manipulating a link, don't we? Why wouldn't we simply drop the Link fieldset and replace it by URL fieldset only? In Drupal we know a link is composed of a link title and an associated URL. We shouldn't need this extra clarification.

So that was the redundant part. Now for the backwards part:

When you read the help text, it seems to me The location this menu links points to. should come first, just below the field. Why? Simply because it generally describes the action we want the user to perform. The This can be an internal Drupal path such as node/add or an external URL such as http://drupal.org. Enter to link to the front page." text is the detailed version that was originally (before the patch) after The path for this menu link.. And it made more sense IMO. The location this menu links points to. could simply come before the same paragraph, or if we really want to, in its own paragraph before the detailed instructions.

+++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
@@ -193,7 +193,7 @@ public function extractFormValues(array &$form, FormStateInterface $form_state)
-    $extracted = $this->pathValidator->getUrlIfValid($form_state->getValue('url'));
+    $extracted = $this->pathValidator->getUrlIfValid($form_state->getValue(['link', 0, 'uri']));

@@ -312,10 +299,10 @@ public function doValidate(array $form, FormStateInterface $form_state) {
-    $extracted = $this->pathValidator->getUrlIfValid($form_state->getValue('url'));
+    $extracted = $this->pathValidator->getUrlIfValid($form_state->getValue(['link', 0, 'uri']));
...
-      $form_state->setErrorByName('url', $this->t("The path '@link_path' is either invalid or you do not have access to it.", array('@link_path' => $form_state->getValue('url'))));
+      $form_state->setErrorByName('link][0][uri', $this->t("The path '@link_path' is either invalid or you do not have access to it.", array('@link_path' => $form_state->getValue(['link', 0, 'uri']))));

This now hardcodes logic on the inner form structure of the widget, which is not ideal.

Not sure how doable that is, but it would be best to use the $entity that already has been built from the form values in both buildEntity() (which calls extractFormValues()) and validate() (which calls doValidate()).

I don't really grasp what extractFormValues() is about, it seems to be much more specific (it's from MenuLinkFormInterface, and "Extracts a plugin definition from form values" - not clear which kind plugin we're talking about) than what it's very generic name seems to imply in the context of an EntityForm. Not introduced by this patch but not helping figuring out the above :-)

I agree with both #23 and #24, but suggest that we open follow ups for them, since that can be worked on in parallel or after the issues that are blocked by this.

Sure, followup is fine by me

Opened #2417783: Remove widget specific logic in MenuLinkContentForm

@anavarre Good points. That will be done in #2416987: Fix UI regression in the menu link form.

Hm. So I will say of the sprint patches I've committed so far, this one makes me the most nervous, since it introduces very obvious regressions in the UX that we can't really ship with, as well as questionable logic per #24. However, my understanding is this is needed in order to make progress on the next round of blockers, and it does fulfill the "fix the data model" portion of this effort. So as long as those follow-ups get done, I think we're ok here.

Committed and pushed to 8.0.x. Thanks!

updated the change record

the other UI follow-up is critical.