Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Assigned: Unassigned » joelpittet
joelpittet’s picture

Status: Active » Needs work
FileSize
5.86 KB

I tried my hand at this one and got so close... the orders aren't saving and the field labels are different than the original... I put an @todo HELP HERE in where I was having trouble

andypost’s picture

I think better to make it different:

class MenuLinkListController extends EntityListController implements FormInterface
+++ b/core/modules/menu/menu.admin.incundefined
@@ -108,8 +106,70 @@ function menu_overview_form($form, &$form_state) {
+  $form['links'] = array(
+    '#type' => 'table',
...
+  $links = _menu_overview_tree_form($tree, $delta);
+  foreach (element_children($links) as $mlid) {
+    $element = $links[$mlid];

List of menu item entities (the menu_link needs list controller) should be embedded into MenuFormController the thing lost sanity after #663946-80: Merge "List links" page into "Edit menu" page

joelpittet’s picture

Assigned: joelpittet » Unassigned

Thanks @andypost for the feedback, though I am not really sure where that rabit hole will take me (read above my head) so I am going to to unassign it from me for now. Anybody that knows more about this could pick it up.

star-szr’s picture

Assigned: Unassigned » star-szr

@joelpittet - I've been away but will take a look at this later in the week.

andypost’s picture

andypost’s picture

Issue summary: View changes

added the twig conversion that is related to this one.

star-szr’s picture

@andypost - I'm not seeing how #3 directly relates to this issue, can you please clarify? This is just a conversion issue and what you're proposing there sounds to me like higher level re-architecting that would be better in a separate issue.

Edit: clarified wording.

star-szr’s picture

Status: Needs work » Needs review

Talked to @andypost on IRC - he clarified that he is talking about separating the "edit form properties (title, description)" form from the "reorder/disable/enable menu items" form. Which I still think needs to happen in a separate issue.

Addlink 2

I've started working on #2 locally and am slowly understanding a few things, but haven't gotten very far yet. I would wager that this is among the more complex of the conversions. If I can't fix this up in the next couple days I'll post back here with specific questions.

(Sending #2 for review so we can see what fails).

Edited to add screenshot.

Status: Needs review » Needs work

The last submitted patch, type-table-1938918-2.patch, failed testing.

star-szr’s picture

Assigned: star-szr » Unassigned

Unassigning, need to focus on Twig conversion tasks. I worked on this patch last weekend but unfortunately wasn't able to make any progress on it.

jibran’s picture

Issue tags: +Novice

Tagging.

nielsonm’s picture

FileSize
4.03 KB

Rebased #2 to head. Then fleshed out TODOs.

nielsonm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Novice

The last submitted patch, type-table-1938918-12.patch, failed testing.

star-szr’s picture

Status: Needs work » Needs review

#12: type-table-1938918-12.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice

The last submitted patch, type-table-1938918-12.patch, failed testing.

tim.plunkett’s picture

Once #1984702: Convert menu.module's page callbacks to Controllers goes in, theme_menu_overview_form() will be the only function left in menu.admin.inc.

seanr’s picture

So this is odd. When you uncheck the enabled checkbox with the patch applied and hit save, the checkbox is checked again on the next screen. The value isn't being set even though Drupal claims success (hence the failed test). Investigating.

seanr’s picture

I just blew two hours trying to figure out what in God's name is going on in that submit function, but I can't crack it for the life of me. Something about changing the form to use the new table type broke what the submit function is looking for. Instead of mlids in the foreach in menu_overview_form_submit we end up with the keys of the two sections of the form. Even trying to correct for that, the if right below doesn't work because #item never appears anywhere. Someone either smarter or crazier than me is going to have to take this over. :-(

seanr’s picture

Issue summary: View changes

Updated issue summary.

joelpittet’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Novice
FileSize
8.28 KB

This will likely not work but I removed the #parents, that makes input fields not be exact matches for their names but with that it there was no difference at all in the saving. Because of the weird voodoo that was happening with the theme function this form submit handler is not correctly going through the values. I got this far but I'm stuck, maybe someone else can tackle the submitOverviewForm debugging?

Drag and drop and table works fine, matching the input names doesn't seem to help the submitOverviewForm handler.

Status: Needs review » Needs work

The last submitted patch, 20: 1938918-menu-type-table-20.patch, failed testing.

RainbowArray’s picture

Issue tags: +Needs reroll
joelpittet’s picture

May need a manual re-roll or complete rewrite. There has been many changes since this was posted.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
9.11 KB

Here's an attempt at a manual reroll. So much has changed with the menu system that the chance that this will break is very high. But hey, you only live once.

Status: Needs review » Needs work

The last submitted patch, 24: menu-table-type-1938918-23.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll +Needs manual testing, +Needs markup review
FileSize
8.75 KB
5.46 KB

Thanks @mdrummond, I've attempted to fix the other issues. Though that UI may be completely broken as the drag and drop seems to be not working at all before or after the patch.

We'll see how testbot takes these changes. I also fixed the path to the menu_ui.css which was broken in HEAD. There must be an issue already addressing this?

Status: Needs review » Needs work

The last submitted patch, 26: convert_menu_theme-1938918-26.patch, failed testing.

mgifford’s picture

joelpittet’s picture

Status: Needs work » Needs review
FileSize
8.78 KB

Re-roll

Status: Needs review » Needs work

The last submitted patch, 29: 1938918-menu-type-table-29.patch, failed testing.

seanr’s picture

What's going on with the failing tests? Do they need to be updated to account for the changes in the patch?

joelpittet’s picture

LIkely #parents/#parents_array have shifted a bit I'm guessing. Want to give it a check @seanr?

lauriii’s picture

Status: Needs work » Needs review
FileSize
8.85 KB
536 bytes

Status: Needs review » Needs work

The last submitted patch, 33: 1938918-menu-type-table-33.patch, failed testing.

lauriii’s picture

Status: Needs work » Needs review
FileSize
4.5 KB
11.98 KB

Listing is visually broken with this patch. Testing if the functionality works now.

Status: Needs review » Needs work

The last submitted patch, 35: 1938918-menu-type-table-35.patch, failed testing.

joelpittet’s picture

+++ b/core/includes/file.inc
@@ -323,7 +323,19 @@ function file_create_url($uri) {
-      return $GLOBALS['base_url'] . '/' . UrlHelper::encodePath($uri);
+      $options = UrlHelper::parse($uri);
+      $path = $GLOBALS['base_url'] . '/' . UrlHelper::encodePath($options['path']);
+      // Append the query.
+      if ($options['query']) {
+        $path .= '?' . UrlHelper::buildQuery($options['query']);
+      }
+
+      // Append fragment.
+      if ($options['fragment']) {
+        $path .= '#' . $options['fragment'];
+      }
+
+      return $path;

This look like's it's unrelated and snuck in eh?

lauriii’s picture

Status: Needs work » Needs review
FileSize
9.36 KB

yeah fixed that mess

joelpittet’s picture

Status: Needs review » Needs work

Yay it's green! Thanks @lauriii now we just have to do some manual testing and curious about these two items:

  1. +++ b/core/modules/menu_ui/src/MenuForm.php
    @@ -242,10 +239,81 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
    +        $form['links'][$id]['#item'] = $element['#item'];
    ...
    +        $form['links'][$id]['#item'] = $element['#item'];
    

    Duplication.

  2. +++ b/core/modules/menu_ui/src/MenuForm.php
    @@ -242,10 +239,81 @@ protected function buildOverviewForm(array &$form, FormStateInterface $form_stat
    +        $form['links'][$id]['weight'] = $element['weight'];
    +        $form['links'][$id]['id'] = $element['id'];
    +        $form['links'][$id]['parent'] = $element['parent'];
    

    The reason these were in a group because it looked like they needed to be in a column together. Maybe that's not the case?

lauriii’s picture

#39.2 Thats what i mean on my first comment that its visually broken now. The problem is that the submit handler doesnt work if the array structure changes. We might have to fix the submit handler to make it visually look how it should.

joelpittet’s picture

I've hacked things to make things work with #parents or #array_parents (never remember which one to use). Maybe that will help? I think it's #parents from reading/deciphering this https://www.drupal.org/node/279246#comment-2292322

botanic_spark’s picture

Assigned: Unassigned » botanic_spark

It seams that the dragging functionality is not working.
Also, there is no "show weight" link that toggles dragging functionality.

I will start working on it.

botanic_spark’s picture

Here is the patch that fixes broken javascript and visually fixes table.

$this->t('Operations'),
is replaced with

array(
  'data' => $this->t('Operations'),
  'colspan' => 3,
),

This solves the javascript error and displays the header all the way to the end of the table.

Also part with

$form['links'][$id]['id'] = $element['id'];
$form['links'][$id]['parent'] = $element['parent'];

is moved under

// Operations (dropbutton) column.
$form['links'][$id]['operations'] = $element['operations'];

and this solves alignment issue of the 'Operations' header column.

botanic_spark’s picture

Status: Needs work » Needs review
euphoric_mv’s picture

Status: Needs review » Reviewed & tested by the community

Tested last patch #43.
It works fine.

joelpittet’s picture

Cool thanks @botanic_spark, that's a slightly less hacky way than I was proposing. It would be nice to not use colspan to do this, but this avoids dealing with the #parents

botanic_spark’s picture

@joelpittet I will investigate a little bit more if there is some other solution. Any suggestion is welcome :)

botanic_spark’s picture

I had a second look at it, and I really don't see the other way.
If we don't use colspan, we would definitely have to modify the submit handler.

catch’s picture

Status: Reviewed & tested by the community » Fixed

I looked at this in the context of #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? and it doesn't fall into any of the obvious categories there, so I've suggested we add changes to individual theme functions to the 'unfrozen' category. Even if we don't do that, it's a limited change shouldn't cause any disruption and has real benefits (less theme functions the better!) so committed/pushed to 8.0.x.

  • catch committed ecaa527 on 8.0.x
    Issue #1938918 by joelpittet, lauriii, botanic_spark, mdrummond,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.