MenuLinkNG part3 (no conversions): MenuLinkContent UI

Is this an accurate depiction of the UI improvements?

1. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,206 @@
   +  /**
   +   * The menu tree service.
   +   *
   +   * @var \Drupal\Core\Menu\MenuParentFormSelectorInterface
   +   */
   +  protected $menuParentSelector;
   

   the doc is wrong, it is the parent form selector service.

2. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,206 @@
   +  /**
   +   * Constructs a new MenuLinkDefaultForm.
   +   *
   

   (minor) absolute ...

3. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,206 @@
   +    $new_definition['hidden'] = $form_state['values']['enabled'] ? 0 : 1;
   ...
   +    $new_definition['expanded'] = $form_state['values']['expanded'] ? 1 : 0;
   

   We do have a follow up for this magic values already, yeah!

4. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,206 @@
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function validateConfigurationForm(array &$form, array &$form_state) {
   +  }
   

   MenuLinkForm in head has a couple of validations related to the path but this does not matter here as we cannot change the path, yeah!!

5. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,206 @@
   +   * This function is horrible, but core has nothing better until we add a
   +   * a method to the ModuleHandler that handles this nicely.
   +   * @see https://drupal.org/node/2281989
   

   Can we convert this straight into a @todo, so there is a lower change to be forgotten in the future

6. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,206 @@
   +  protected function getModuleName($module) {
   

   (minor) missing docs

7. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkDefaultForm.php
   @@ -0,0 +1,206 @@
   +      return $this->t($this->moduleData[$module]['name']);
   

   Just a sitenode: it is okay to call t() here as it is part of a yml file as well and so scannable by POTX

8. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkFormInterface.php
   @@ -0,0 +1,43 @@
   + * Defines an interface for edit forms for menu links.
   

   i am not native but ... of menu links ... sounds better

9. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkFormInterface.php
   @@ -0,0 +1,43 @@
   + * editing form, but the implementations may differ.
   + * @see \Drupal\Core\Menu\MenuLinkInterface::getFormClass()
   + */
   

   newline needed before @see

10. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkFormInterface.php
    @@ -0,0 +1,43 @@
    +   * Injects the menu link plugin.
    

    maybe "Injects the menu link instance"?

11. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkFormInterface.php
    @@ -0,0 +1,43 @@
    +  /**
    +   * Form plugin helper.
    +   *
    

    This should tell what the method is supposed to do.

12. +++ b/core/lib/Drupal/Core/Menu/Form/MenuLinkFormInterface.php
    --- /dev/null
    +++ b/core/modules/menu_link_content/menu_link_content.local_tasks.yml
    

    renamed to .links.task

13. +++ b/core/modules/menu_link_content/src/Controller/MenuController.php
    @@ -0,0 +1,34 @@
    +
    +class MenuController extends ControllerBase {
    

    needs some docs

14. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentDeleteForm.php
    @@ -0,0 +1,46 @@
    +  public function getCancelRoute() {
    

    do we have an issue to rename this method? this is no longer a route but a Url

15. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentDeleteForm.php
    @@ -0,0 +1,46 @@
    +    $storage = $this->entityManager->getStorage('menu_link_content');
    +    $storage->delete(array($this->entity));
    

    I wonder whether we should rather go with $this->entity->delete instead?

16. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentDeleteForm.php
    @@ -0,0 +1,46 @@
    +    watchdog('menu', 'Deleted menu link %title.', $t_args, WATCHDOG_NOTICE);
    

    we could replace it by a logger already

17. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentDeleteForm.php
    @@ -0,0 +1,46 @@
    +    $form_state['redirect_route'] = array(
    +      'route_name' => '<front>',
    +    );
    

    We could add a follow up to redirect to the menu form instead.

18. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +   *
    +   * @var \Drupal\menu_link_content\Entity\MenuLinkContent
    +   */
    

    Let's use the MenuLinkContentInterface instead

19. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +  /**
    +   * The menu tree service.
    +   *
    +   * @var \Drupal\Core\Menu\MenuParentFormSelectorInterface
    +   */
    +  protected $menuParentSelector;
    

    some doc problem as before.

20. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +    // Load the entity for the entity form.
    +    $metadata = $menu_link->getMetaData();
    +    if (!empty($metadata['entity_id'])) {
    +      $this->entity = $this->entityManager->getStorage('menu_link_content')->load($metadata['entity_id']);
    +    }
    +    else {
    +      // Fallback to the loading by the uuid.
    +      $links = $this->entityManager->getStorage('menu_link_content')->loadByProperties(array('uuid' => $menu_link->getDerivativeId()));
    +      $this->entity = reset($links);
    +    }
    

    Let's document that loading by entity_id is faster

21. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +   * Break up a user-entered URL or path into all the relevant parts.
    

    (minor) Break => Breaks

22. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +      // If the path doesn't match a Drupal path, the route should end up empty.
    

    Nice: 80 chars comment!

23. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +  public function extractFormValues(array &$form, array &$form_state) {
    +
    

    it is a bit sad that we have similar code in both the default menu form and the content one, but this is okay, given that it is not complex code.

24. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +    }
    +    $new_definition['title'] = $form_state['values']['title'][0]['value'];
    +    $new_definition['description'] = $form_state['values']['description'][0]['value'];
    +    $new_definition['weight'] = (int) $form_state['values']['weight'][0]['value'];
    

    Is there a way to get this values from some entity form helpers? This should be a follow up

25. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +   */
    +  public function form(array $form, array &$form_state) {
    +
    +    $form = parent::form($form, $form_state);
    

    let's drop this newline

26. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +      drupal_set_message(t('The menu link has been saved.'));
    ...
    +    else {
    +      drupal_set_message(t('There was an error saving the menu link.'), 'error');
    

    Let's use $this->t() instead

27. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
    @@ -0,0 +1,410 @@
    +      // Users are not allowed to add a link to a page they cannot access.
    +      $valid = \Drupal::service('access_manager')->checkNamedRoute($extracted['route_name'], $extracted['route_parameters'], \Drupal::currentUser());
    +    }
    

    Let's avoid troubles and inject the service

28. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentUITest.php
    @@ -0,0 +1,68 @@
    +class MenuLinkContentUITest extends ContentTranslationUITest {
    

    ContentTranslationUITest is the best, seriously!

29. +++ b/core/modules/menu_link_content/src/Tests/MenuLinkContentUITest.php
    @@ -0,0 +1,68 @@
    +
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public static function getInfo() {
    +    return array(
    +      'name' => 'Menu link content translation UI',
    +      'description' => 'Tests the basic menu link content translation UI.',
    +      'group' => 'Menu',
    +    );
    +  }
    

    Needs to be replace by a @group Menu

30. +++ b/core/modules/menu_ui/src/Form/MenuLinkEditForm.php
    @@ -0,0 +1,102 @@
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function getFormId() {
    +    return 'menu_link_edit';
    +  }
    

    I wonder whether each form should rather implement their own, given that the generated HTML looks totally different so could the styling!

31. +++ b/core/modules/menu_ui/src/Form/MenuLinkEditForm.php
    @@ -0,0 +1,102 @@
    +      '#value' => t('Save'),
    

    (minor) $this->t()

32. +++ b/core/modules/menu_ui/src/Form/MenuLinkEditForm.php
    --- /dev/null
    +++ b/core/modules/system/config/install/menu_link.static.overrides.yml
    
    +++ b/core/modules/system/config/install/menu_link.static.overrides.yml
    +++ b/core/modules/system/config/install/menu_link.static.overrides.yml
    @@ -0,0 +1 @@
    
    @@ -0,0 +1 @@
    +definitions: []
    
    +++ b/core/modules/system/config/schema/system.schema.yml
    @@ -391,3 +380,30 @@ condition.plugin.request_path:
    +
    +menu_link.static.overrides:
    

    We could or even should move it to core/config

33. +++ b/core/modules/system/config/install/menu_link.static.overrides.yml
    index 5375524..f524376 100644
    --- a/core/modules/system/config/schema/system.schema.yml
    

    schemas!!!

rebased on 8.x, with conversion of MenuLinkElement to use methods (needs a little more cleanup)

Here's a new patch for this current step, and increment since the 1st patch

looking at #2304363: Make MenuLinkTreeElement's properties protected, add getters, add limited setters The method conversion isn't fully working, so rolling it back out of this patch for now.

IGNORE #7 please.

This increment is relative to the original patch posted by Alex.

pwolanin has pushed a commit to the sandbox that fixes all feedback in #6, but hadn't yet uploaded the patch here. So doing just that, so we can continue with the review process.

I also did a very quick, very high-level review (I posted the above patch but didn't work on this part at all).

Also, since this apparently (see #5) introduces an updated UI, I think we need usability review for this? Or was that evaluated already?

1. +++ b/core/modules/menu_link_content/src/Controller/MenuController.php
   @@ -0,0 +1,37 @@
   + * Define a route controller for the form for adding a menu link content entity.
   

   "Defines." But then this'll go beyond 80 chars.

   What about:
   "Defines a route controller for the menu link content entity creation form."

2. +++ b/core/modules/menu_link_content/src/Controller/MenuController.php
   @@ -0,0 +1,37 @@
   +   * Provides the menu link submission form.
   ...
   +   *   Returns the menu link submission form.
   

   s/submission/creation/

3. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -0,0 +1,433 @@
   +      // Fallback to the loading by the uuid.
   

   s/the//

4. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -0,0 +1,433 @@
   +    // @TODO For whatever reason the expanded widget is not autogenerated.
   ...
   +      // @TODO Maybe support options in
   +      // \Drupal\Core\Routing\UrlGeneratorInterface::getInternalPath().
   +      // or a helper method to render just options?
   

   I think we still need to address these?

5. +++ b/core/modules/menu_ui/src/Form/MenuLinkEditForm.php
   @@ -0,0 +1,102 @@
   +  public function buildForm(array $form, array &$form_state, MenuLinkInterface $menu_link_plugin = NULL) {
   +
   +    $form['menu_link_id'] = array(
   

   Extraneous newline.

Fixed most of these, looks like someone got a few before.

@dawehner
re: #6.14, it seems to be correct, it implements
ConfirmFormInterface::getCancelRoute()

re: #6.17 - no, this form does not depend on menu_ui module, so it should NOT redirect to a route provided by that module. I this we use a destination query string to override this redirect route.

re: #6.30 - not sure I understand what you mean

re: #6.32 - given that really nothing else in in the core config, I don't know that it makes sense - menu links are more part of system module so I think leave this where it is.

@Wim Leers

re: #11

Yes Bojhan, Gábor Hojtsy, and larowlan have done usability and UI review already. See:

https://www.drupal.org/node/2227441#comment-8803533 & https://www.drupal.org/node/2256521#comment-8958175

Added a follow-up issue for rendering options, a linked t existing bug reports about the form element for the boolean fields.

@dawehner
re: #6.14, it seems to be correct, it implements
ConfirmFormInterface::getCancelRoute()

Opened a followup: #2305023: Rename getCancelRoute to getCancelUrl

re: #6.30 - not sure I understand what you mean

I got the impression that 'menu_link_edit' was somewhere else. Not sure what I thought exactly here.

re: #6.32 - given that really nothing else in in the core config, I don't know that it makes sense - menu links are more part of system module so I think leave this where it is.

Well, then please move the static menu override service into the system module.

per Berdir, as of this morning, with boolean and list_boolean merged, the boolean should have a widget

So, we should probably test and re-roll to use that.

Possibly a last chance to change the field to "status" instead of "hidden"

I'm not able to get the boolean form field to work so far, but trying to get feedback from Berdir about why.

@dawehner - other Core classes have config but it's not under core

for example:

core/lib/Drupal/Core/Theme/DefaultNegotiator.php

$this->config = $config_factory->get('system.theme');

core/lib/Drupal/Core/Routing/UrlGenerator.php

$allowed_protocols = $config->get('system.filter')->get('protocols') ?: array('http', 'https');

so I think having Core services config under system module is the norm

The reason it is not there is because it used to be not possible to add a config to /core

@dawehner - I'm agnostic about where it lives, if it doesn't' disrupt the patch

a couple issues in @todos are in the referenced by.
adding the other two, created from this issue.

update first pass for UI review info

updating the issue summary also
----------
1. with only part #3 patch from #12, the links do not actually get created on save... but with the whole patch #5 (from github: https://github.com/pwolanin/drupal/tree/2256521-step5-2301319) it does work.

2. screenshot in #5 was of the views ui for adding a menu link, and that is the same in head as with patch #5.

3. the weight uses the core html5 number element

4. The order of the fields (right now) is different in the add and edit. Might try to change the patch so that the path required field is up at the top.
head

patch #3 (and #5)

5. the page menu settings fieldset thing is the same with and without the patch (well, almost the same. with the patch, uses the core html5 number element)

6. the add menu link link (not the + add link button/primarylink) does not have the ? destination redirect, so a save on the add page using that link, just reloads the edit form.

7. drupal installed in a subdir of the webroot causes reveals a bug with the destination so a save from the add or edit, or the delete is an error

8. mostly not related to changes here, but noticed while working on menu link things, is the suggestion to remove the weight from the menu link add/edit since the weight is out of context without the other menu links. (and the html5 number element does not have limits on it like the drop down used to). an old issue from 2009: #491022: Remove weight field from menu item entity forms

9. when adding a menu link to a menu, and then picking a different menu in the menu add form,
or when editing a menu link, and changing it's parent to another menu:
after saving get redirected to the original menu, not the new parent.
(in head, get redirected to the new parent menu)
#2305353: Make redirect after saving a menu link into another parent menu, go to the parent menu admin form

in the sandbox, I moved the path up, with weight. Might look later at reordering the other fields to match head. but have to go for now.

The reason it is not there is because it used to be not possible to add a config to /core

Yeah, the config should be moved out of system now that this has been fixed.

Nice UI review YesCT!

Wow, fantastic work on that UI review! :D And thanks for fixing the path field weight; that would drive me absolutely nuts.

Why did #12 add a @todo referring to #2226493: Apply formatters and widgets to Node base fields? Because that issue will also have to solve the problems seen here? That's the only way this could possibly be considered "related" — I am confused by #2226493-105: Apply formatters and widgets to Node base fields.

@Wim Leers - apparently those are the wrong issues about booleans - this went into core, so we should try to remove the hack and todo now: #2226063: Merge ListBooleanItem from options module into BooleanItem

The destination fix is in part 4, here's the other fixes for part 3, mostly moving the yml, and includes the 1st form fix from YesCT, fix to use the core boolean widget, and SafeMarkup fix for the admin form

Just tried testing this with that handy Simplytest.me over there. Here's a screenshot pointing out a stray checkbox that appeared when I created a new menu with a new menu item:

And here's the markup for it.

Looks like the checkbox needs the visually-hidden class or something. This checkbox shouldn't be here.

@cosmicdreams - please don't test the UI of this patch - it's not functional for the plugins and still tries to use the old system. You need to test the next parts 4 or 5 to have a functional UI.

I have a patch coming in a bit to reorder the fields on the menu link add/edit form.
Also, the show as expanded checkbox is missing after the Boolean element change,
and the operation link from the menu listing page, the operation to add a menu link.. goes to add a menu.
I'll look at fixing those also.

@YesCt - I think we should do that in part 4 so people can actually test it? Let's mar this one rtbc?

At Drupalcorn sprint

added instructions on how to try out the new work.

Added usage note about need to enable the custom menu link module

Here's a now patch with some small cleanups including the form re-ordering from YesCT. Also moves a change to core/modules/menu_ui/menu_ui.admin.inc out that was premature and I added it back to part 4. This might have been the cause of the stray checkbox noted by @cosmicdreams.

There are for sure some nitpicks available here and there, but this is not a reason to block this
the progress. The moving of the form elements looks fine.

One more trivial fix from YesCT to align wording and remove a blank line.

Also, rebased on 8.x since part4 conflicts.

1. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -0,0 +1,429 @@
   +      // @todo add a helper method to Url to render just the query string and
   +      // fragment. https://www.drupal.org/node/2305013
   +      $default_value = $url->getInternalPath();
   

   getInternalPath() is deprecated - should we be using it here?

2. +++ b/core/modules/menu_link_content/src/Form/MenuLinkContentForm.php
   @@ -0,0 +1,429 @@
   +  /**
   +   * Validates the form, both on the menu link edit and content menu link form.
   +   */
   +  protected function doValidate(array $form, array &$form_state) {
   

   Missing params from doc block - plus $form is never used do we need to pass it?

3. 
   
   Seems like we're missing a delete button.

Opened a follow up for getPathFromRoute(): https://www.drupal.org/node/2307061 For now there is no better alternative to deal with destinations.

getPathFromRoute() might also be needed in part 4 for use while generating the destination for the add link link and delete links

ok, so the delete link was missing since the "delete-form" link was not in the entity links annotation and that's used by the access check. We must have missed a core change for that. The delete link now appears on the edit form.

Added a further @todo for getInternalPath() and links to the issue dawehner opened.

Fixed the doValidate() function doc.

Also, pulled in the MenuLinkPluginConverter since it should also be here as part of the purely new code.

A bunch of other doc fixes from YesCT

Thank you for the fixes!

Okay let's get to the meat/core (depending on your dietary requirements) of the patch - part 4!

Committed ef4e932 and pushed to 8.x. Thanks!

Tagging the child issues retroactively.