Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Task
Convert the following theme functions to use the new table #type:
Module | Theme function name | Where in Code | What is it really? |
---|---|---|---|
menu | theme_menu_overview_form | function | form table draggable |
Related issues
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff.txt | 1.21 KB | botanic_spark |
#43 | 1938918-menu-type-table-43.patch | 9.43 KB | botanic_spark |
#38 | 1938918-menu-type-table-38.patch | 9.36 KB | lauriii |
Comments
Comment #1
joelpittetComment #2
joelpittetI 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
Comment #3
andypostI think better to make it different:
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
Comment #4
joelpittetThanks @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.
Comment #5
star-szr@joelpittet - I've been away but will take a look at this later in the week.
Comment #6
andypostThere's related issue #1882218: Remove static menu link creation for menus in menu_enable() and elsewhere
Comment #6.0
andypostadded the twig conversion that is related to this one.
Comment #7
star-szr@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.
Comment #8
star-szrTalked 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.
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.
Comment #10
star-szrUnassigning, 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.
Comment #11
jibranTagging.
Comment #12
nielsonm CreditAttribution: nielsonm commentedRebased #2 to head. Then fleshed out TODOs.
Comment #13
nielsonm CreditAttribution: nielsonm commentedComment #15
star-szr#12: type-table-1938918-12.patch queued for re-testing.
Comment #17
tim.plunkettOnce #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.
Comment #18
seanrSo 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.
Comment #19
seanrI 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. :-(
Comment #19.0
seanrUpdated issue summary.
Comment #20
joelpittetThis 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.
Comment #22
RainbowArrayComment #23
joelpittetMay need a manual re-roll or complete rewrite. There has been many changes since this was posted.
Comment #24
RainbowArrayHere'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.
Comment #26
joelpittetThanks @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?
Comment #28
mgiffordComment #29
joelpittetRe-roll
Comment #31
seanrWhat's going on with the failing tests? Do they need to be updated to account for the changes in the patch?
Comment #32
joelpittetLIkely #parents/#parents_array have shifted a bit I'm guessing. Want to give it a check @seanr?
Comment #33
lauriiiComment #35
lauriiiListing is visually broken with this patch. Testing if the functionality works now.
Comment #37
joelpittetThis look like's it's unrelated and snuck in eh?
Comment #38
lauriiiyeah fixed that mess
Comment #39
joelpittetYay it's green! Thanks @lauriii now we just have to do some manual testing and curious about these two items:
Duplication.
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?
Comment #40
lauriii#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.
Comment #41
joelpittetI'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
Comment #42
botanic_spark CreditAttribution: botanic_spark commentedIt 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.
Comment #43
botanic_spark CreditAttribution: botanic_spark commentedHere is the patch that fixes broken javascript and visually fixes table.
$this->t('Operations'),
is replaced with
This solves the javascript error and displays the header all the way to the end of the table.
Also part with
is moved under
and this solves alignment issue of the 'Operations' header column.
Comment #44
botanic_spark CreditAttribution: botanic_spark commentedComment #45
euphoric_mv CreditAttribution: euphoric_mv commentedTested last patch #43.
It works fine.
Comment #46
joelpittetCool 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
Comment #47
botanic_spark CreditAttribution: botanic_spark commented@joelpittet I will investigate a little bit more if there is some other solution. Any suggestion is welcome :)
Comment #48
botanic_spark CreditAttribution: botanic_spark commentedI 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.
Comment #49
catchI 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.