Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Updated: Comment 0
Problem/Motivation
If you have a local task which has upcasted parameters like a translate tab on some config entites
there is an "Error" shown as title.
Proposed resolution
Remaining tasks
User interface changes
API changes
Related Issues
- this issue is blocking the config_translation issue: #2044737: Provide local tasks as plugins
- #2095271: Add default tabs for routes expected by config_translation
Comment | File | Size | Author |
---|---|---|---|
#28 | local_task-2095139-26-28-interdiff.txt | 633 bytes | kfritsche |
#28 | local_task-2095139-28.patch | 7.97 KB | kfritsche |
#26 | local_task-2095139-26.patch | 8 KB | dawehner |
#26 | interdiff.txt | 2.99 KB | dawehner |
#25 | local_task-20951390-25.patch | 7.24 KB | pwolanin |
Comments
Comment #1
dawehner.
Comment #2
pwolanin CreditAttribution: pwolanin commentedWe need to insure that attribute exists, at least, before calling a method on it.
Comment #3
dawehner... this blocks many conversions.
Comment #4
tim.plunkettOuch, just hit this in #2095271: Add default tabs for routes expected by config_translation.
Comment #5
dawehnerHere is a test.
Comment #6
pwolanin CreditAttribution: pwolanin commentedlooks good.
Comment #7
tim.plunkettExtra debug.
Also, can you upload the test-only in the next one
Comment #8
Gábor HojtsyComment #9
Gábor HojtsyComment #10
dawehnerThere we go.
Comment #11
dawehnerIssuetag monster
Comment #12
YesCT CreditAttribution: YesCT commentedcomment here about what testing for.
I think this one is checking to see if there is only one active tab.
I think this is seeing if sub1 is active (not if settings tab is active).
a comment before this second section would be nice.
I think it is:
Switching to the second tab, sub2, and checking that sub1 does not still have the active class, by checking there is only one active tab and it is sub2.
why _ in the first placeholder sub1,
but . in the second placeholder sub2.
and then again for the upcasting.
also, I thought the pattern was module_name.something_unique
this should be menu_test.placeholder_sub1:
wait. I think it was meant to have the dot over so the unique part is local_task_test_upcasting_sub1
@attrib is going to work on this.
[edit adding links to docs]
https://drupal.org/node/2044515
when the dot moved to just after the module name, the local task name was the same as the route, the local task tests failed. Let's try putting _tab on the end of the plugin in for the local tasks.
Comment #13
kfritscheworking on it now.
Comment #14
kfritscheChanged the assert messages and added comments, see interdiff.
Like pointed out by YesCT we should use the correct pattern here. Also we couldn't named the tabs like the route name so I added "_tab":
This introduced a lot of changes in menu_test.local_tasks.yml and I replaced the usages in LocalTasksTest. Also searched for other uses, but didn't found any.
Comment #15
tim.plunkettAbsolutely NONE of those route patterns were in scope to be changed here. There is a dedicated issue for that. This was just about adding
->get('_raw_variables')
on one line...Comment #16
YesCT CreditAttribution: YesCT commentedyeah, I was thinking the changes would only be in those lines already changed/added in this patch.
but ... it has reached too far.
Sorry, I'll take it out.
Comment #17
YesCT CreditAttribution: YesCT commentedI guess maybe the second half of the local tasks is new (not moved) and it could have gotten the current pattern, but I left it.
So this is just the comment changes, and the corrections to the assertion messages.
Comment #18
vijaycs85Reviewed and all looks good except a minor issue.
Minor: Can we split this into two lines to make it more readable?
This is very minor. So RTBC from my side.
Comment #19
tstoecklerI wanted to re-roll this in order to bring this forward. Because of the nested AND/OR the line gets very verbose, though, so I am not 100% sure this is an improvement. I'll let the committers decide.
Comment #20
tstoecklerSorry, the interdiff shows a deleted file, which was already deleted before, I messed that up. The patch is fine though, as far as I can see.
Comment #21
pwolanin CreditAttribution: pwolanin commented@tstoeckler - that formatting seems odd. I don't think that conforms to code style for multi-line.
https://drupal.org/coding-standards#linelength
Comment #22
tim.plunkettCan we *please* just have this issue fix the bug? The multiline boolean is already in HEAD, this shouldn't change that.
Comment #23
pwolanin CreditAttribution: pwolanin commentedI'm re-rolling based on #10.
i don't think that was RTBC since it needs to verify it got something for _raw_variables.
Comment #24
pwolanin CreditAttribution: pwolanin commentedHere's a patch that validates the presence of the attribute and also fixes the test text.
Comment #25
pwolanin CreditAttribution: pwolanin commentedFeedback from Tim in IRC, we should fallback to the request attributes, as we do in other code.
Comment #26
dawehnerLet's move that to a easy to understand method to unclutter the code here.
Comment #28
kfritscheLast change seems fine. Tested it locally again and it works also the test are green, no clue what was wrong with the testbot.
Just a very very minor issue, there are two returns which is unnecessary. Removed it.
If testbots agree, RTBC again.
Comment #29
dawehnerGood catch!
Comment #30
alexpottCommitted 375fadf and pushed to 8.x. Thanks!
Comment #31.0
(not verified) CreditAttribution: commentedadding the related