Remove MenuLinkContentForm::doValidate() since the logic was moved to LinkWidget::validateUriElement()

This is already covered by #2403823: Menu link content entity validation misses form validation logic, thus marking as duplicate.

Reopened per #2403823-21: Menu link content entity validation misses form validation logic.

MenuLinkContentForm is quite confusing. It subclasses ContentEntityForm because it's a content entity. But since MenuLinkContent also is a menu link plugin, MenuLinkContentForm also implements MenuLinkFormInterface.

And that MenuLinkFormInterface implements PluginFormInterface, which requires the following methods:

1. buildConfigurationForm()
2. validateConfigurationForm()
3. submitConfigurationForm()

It's not clear to me at all when these 3 methods are called. In fact, no matter what I try, I can't get them to be called. For the simple reason that \Drupal\Core\Menu\MenuLinkInterface::getEditRoute() ensures that we use the entity.menu_link_content.canonical route, which uses the MenuLinkContentForm entity form. Hence we never hit the menu_ui.link_edit route, which uses MenuLinkEditForm, which calls the given menu link plugin's form class (which is confusingly but understandably also MenuLinkContentForm).

IOW: I believe it's safe to no longer implement MenuLinkFormInterface for the MenuLinkContent menu link plugin. If that's true, then no additional test coverage is necessary.

@Wim Leers - yes, I think we zig zagged a bit on this one, and since it probably makes sense to use the entity form, we might indeed ditch the implementation of MenuLinkFormInterface

However - the counter-argument is that it would be nice to be able to edit every menu link plugin in a standard way, so maybe we are doing it backwards currently?

#7: if the current way is backwards and we want to ditch subclassing ContentEntityForm, then we'd have to reimplement a lot. I don't think that's desirable? Or do you mean something else than that?

@Wim Leers - if we'd have to reimplement a lot, then lets keep it as-is and add tests or ditch MenuLinkFormInterface?

That's what #5 does! :)

re @Wim #5

MenuLinkContentForm is quite confusing. It subclasses ContentEntityForm because it's a content entity. But since MenuLinkContent also is a menu link plugin, MenuLinkContentForm also implements MenuLinkFormInterface

+ a lot, that confused me a lot as well - I opened a rename proposal in #2417799: Clarify method names in MenuLinkFormInterface

If MenuLinkContentForm doesn't need to implement MenuLinkFormInterface, that's even better :-)

Taking a look at this - the fails suggest something was removed that was needed in processing form values.

This puts code back necessary for processing the form values.

Should possibly replace the use of the path validator here with Url::fromUri()? Or in a separate issue?

Nice cleanup, looks good. I agree with the removal of the MenuLinkFormInterface implementation. I think Url::fromUri() is out of scope for this issue, let's just do away with the unnecessary code here. One minor nitpick then this is IMO RTBC:

+++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
@@ -241,13 +143,13 @@ protected function actions(array $form, FormStateInterface $form_state) {
-  /**
-   * {@inheritdoc}
-   */
+ /**
+  * {@inheritdoc}
+  */

That indentation change is wrong.

Corrected the indentation.

Cool, assuming the bot will come back green.

Yay, lots of '-' in that patch :-)

Can't we just inline extractFormValues() into buildEntity() ? No real point in splitting the logic of buildEntity in two methods, scattered apart several methods away in the class ? Would also make the method order in the class more intuitive (define form, extract values into entity, save)

Then, we should make the code use the $entity built with parent::buildEntity() rather than $form_state->getValue(['link', 0, 'uri'] - but we can also keep that for #2417783: Remove widget specific logic in MenuLinkContentForm :-p

#13 reintroduces ::extractFormValues(). But that method returns "a new definition", i.e. a new menu link definition. That doesn't make sense for a content entity form. That only makes sense for a MenuLinkFormInterface implementation, which it no longer is. I think we should revert #13's interdiff, and just make it work without that. Otherwise, MenuLinkContentForm still is very hard to comprehend.

@Wim Leers - so we need to be able to get a definition from the entity - you think this should be reworked in terms of just the entity values? it's the naming that bothers you?

Yes, exactly, this should just use entity values. Since it doesn't implement MenuLinkFormInterface anymore, none of the $definition stuff should be necessary anymore.

In other words: none of ::extractFormValues() should be necessary anymore. #13 reintroduced it, but even though I don't see yet what exactly is missing if we just omit it like #5 did, it should only be a tiny bit from ::extractFormValues() that's still necessary, and then we can keep just that bit, but just do it in ::buildEntity().

(Basically I think #5 is what should happen, it worked fine for me, don't know why it's failing.)

EDIT: grammar fixes.

I'm going to work on updating the patch based on @Wim Leers comments.

Updated patch to remove ::extractFormValues() and move the necessary pieces to ::buildEntity().

My first attempt at writing a test for Drupal, please be kind :)

Bad commit in HEAD broke testbot. Resetting status.

Excellent, this is almost ready!

+++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentFormTest.php
@@ -0,0 +1,71 @@
+class MenuLinkContentFormTest extends WebTestBase {
+  /**

empty line missing.

Breaks my heart to set this back to needs work for this minor nitpick, but I promise to RTBC this once you fix it :-)

Fixed missing missing line.

Looks good, assuming that the bot comes back green.

Yes! So much better! :) Thanks for your awesome patch :)

+++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
@@ -248,12 +105,13 @@ protected function actions(array $form, FormStateInterface $form_state) {
+    $entity->parent->value = $parent;
+    $entity->menu_name->value = $menu_name;
+    $entity->enabled->value = (!$form_state->isValueEmpty(array('enabled', 'value')));
+    $entity->expanded->value = (!$form_state->isValueEmpty(array('expanded', 'value')));

Can we add a @todo here to use helper setter methods with link to the issue?

re #32 that's existing code and removing the magic setters would require @todo's everywhere.

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 45c425a and pushed to 8.0.x. Thanks!

It failed retest because I committed.