Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
We still need to figure out #1981644: Figure out how to deal with 'title/title callback' and #2023795: REGRESSION: hook_local_actions doesn't use title callback, but here's a fix for a fun bug we've hacked around.
Comment | File | Size | Author |
---|---|---|---|
#7 | 6-7-interdiff.txt | 3.73 KB | alexpott |
#7 | menu-2027183-7.patch | 7.29 KB | alexpott |
#6 | menu-2027183-6.patch | 6.44 KB | YesCT |
#6 | interdiff-4-6.txt | 561 bytes | YesCT |
#5 | interdiff.txt | 2.02 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettComment #2
dawehnerWe should certainly explain this line or even lines.
Comment #3
tim.plunkettPosting two patches, I can't remember why I made the one change. If both pass, it was leftover. If the one fails, I'll know why :)
Also, this is being coded around in patches like #1984702: Convert menu.module's page callbacks to Controllers and #111715: Convert node/content types into configuration, and was hacked around in the Field UI conversion.
But I'm calling this a pretty big regression, even if we DO want to kill title callback...
EDIT:
The difference for those curious is
As seen in the first hunk of 3b
Comment #4
tim.plunkettOh, now I remember :) This is used by dynamic titles on local actions, but could be elsewhere. See interdiff (against the patch in 1 since I forgot one last time).
The hunk in field_ui.module explains the bug.
Comment #5
tim.plunkettHere's the missing interdiff.
Comment #6
YesCT CreditAttribution: YesCT commentedI read it closely and it seems reasonable to me.
Does this need to get looked at by someone else, with more knowledge in the area?
Only thing I found was one comment not a sentence with a weird ex: and lots of parentesis.
I fixed it.
But it turns out, that's copy and paste.
Still, hope it's not too disruptive, and made #2027351: tidy up grammar in doc copied around: (ex: array('node', '5')) to take care of the rest.
Since this thing I found is not a core gate blocker, if preferred, could just use the patch from #4.
Comment #7
alexpottIt looks like menu_item_route_access is doing unrelated stuff on the map. I think this should be refactored into a separate function.
Comment #8
tim.plunkettFor menu_item_populate_map() to work it HAS to be called after menu_item_route_access(), because it needs the matchRequest() part called. That seems very brittle to me, since it makes the new function useless on its own.
Comment #9
tim.plunkettFurthermore, menu_item_route_access() has been refactored and rescoped several times, and isn't really an API function (I made it up).
It should really be called
_menu_item_route_manipulation_du_jour()
, but I doubt that would fly.Instead of introducing a new function that is dependent on another function being called first, I'd rather see it combined as in #4, and the function renamed.
If we ever manage to combine _menu_translate() and _menu_link_translate(), we'll be able to move this function inline.
Comment #10
dawehnerSo this patch improves and fixes stuff and on the longrun it will have to look different anyway.
Let's move forward and RTBC #6
Comment #11
alexpottCommitted 0cb8f16 and pushed to 8.x. Thanks!