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 #56
Problem/Motivation
Get as many local tasks as possibly converted to the new YAML definitions without any major changes.
Proposed resolution
Convert local tasks to YAML definitions.
Remaining tasks
Some outstanding tasks need to be converted. Possibly as a follow up.
Follow up: #2111823: Convert field_ui / Entity local tasks to YAML definitions
Also: #2135777: Follow-up [meta] local task conversion, move out of hook_menu() and to local_tasks.yml
User interface changes
None.
API changes
None.
Related Issues
- #2109287: Replace list_themes() with a service.
- #2091691: Convert test non-form page callbacks to routes and controllers
- #2103145: ParameterConverter mangles raw values
- no need for _tab. see #2107531: Improve DX of local task YAML definitions
- #2100073: Convert local_actions to the new local action plugins
- #2100213: take local tasks out of hook_menu so it uses comment.local_tasks.yml with the recommended route naming conventions
- #2070651: Introduce a path-based breadcrumb builder, remove the one that's based on {menu_links}. related to #58
- #2099363: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7) will address the @todo about two config imports
Comment | File | Size | Author |
---|---|---|---|
#114 | local-task-2102125-108-plus-104.patch | 16.18 KB | Gábor Hojtsy |
#108 | local-task-2102125-108.patch | 15.1 KB | tim.plunkett |
#107 | local-task-2102125-107.patch | 3.75 KB | tim.plunkett |
#106 | local-task-2102125-106.patch | 1.13 KB | tim.plunkett |
#104 | parent-task.patch | 1.08 KB | Gábor Hojtsy |
Comments
Comment #2
neclimdulnoticed user/login had the wrong local tasks. fix and tests.
Tests aren't going to be able to login because user/1 throws an exception that was part of the reason I wanted to post this. Disappointing I can't see the other tests but that's life.
Comment #3
Crell CreditAttribution: Crell commentedTagging
Comment #4
dawehnerShameless tagging and cross-linking: #2100073: Convert local_actions to the new local action plugins
Comment #5
dawehnerSome work on that.
Comment #7
tim.plunkettFYI, #2091691: Convert test non-form page callbacks to routes and controllers had to include the local task for content_translation
Comment #8
neclimdul#2103145: ParameterConverter mangles raw values is the fix for #5. applied and merged conflicts with HEAD. Committed local tasks where are related to entities so I hadn't converted them yet anyways(including what tim mentioned)
Comment #10
neclimdulreroll with enhancer fix that was commited.
Comment #11
pwolanin CreditAttribution: pwolanin commentedComment #13
dawehnerFixed a couple of bugs.
Comment #15
tim.plunkettMore fixes.
Comment #17
dawehnerSo in local actions, we dynamically generate local_tasks? Additional you can just call invokeAll
Comment #18
tim.plunkettI just c/p'd from menu_local_tasks().
This is actually a nasty hack, and should be removed. I'll see what I can do.
Comment #19
YesCT CreditAttribution: YesCT commentedmarking #2100213: take local tasks out of hook_menu so it uses comment.local_tasks.yml with the recommended route naming conventions dup.
Comment #20
dawehnerComment #21
tim.plunkettThis removes the menu.inc hack and the weirdness in custom_block.
Comment #23
tim.plunkettI must have hit a key while switching through open files in my IDE...
Comment #25
dawehnerSad enough #2100073: Convert local_actions to the new local action plugins has the same remaining test failure.
Comment #26
tim.plunkettThat will need a fix
Comment #28
tim.plunkettMore fixes
Comment #30
damiankloip CreditAttribution: damiankloip commentedSome more fixes.
Comment #32
dawehnerBack to one failure.
Comment #34
tim.plunkettThis is actually a bogus test. We *want* the title to be "Manage fields", it was just being ignored previously.
Now that this actually passes, we need to clean up the patch. There is stuff commented out and inline comments abound.
Comment #35
webchickIt may be a long shot, but it would be SO NICE to have this done by alpha4. (next Friday)
Comment #36
damiankloip CreditAttribution: damiankloip commentedEasy :-)
Comment #38
tim.plunkettThis needs to be addressed.
Comment #40
neclimdul#38: local-tasks-2102125-38.patch queued for re-testing.
Comment #41
dawehnerComment #42
neclimdulWhen I ran this things blew up all over but seems to work now. We should probably @todo this and I don't know if we can still but it'd be nice if we could open a follow up to put this in a service for testing if nothing else. As soon as something else wants to phpunit test something with listing themes we're dead.
Comment #43
dawehnerOpened a follow up: #2109287: Replace list_themes() with a service.
Comment #44
pwolanin CreditAttribution: pwolanin commentedI don't see why the variable name change $theme_name to $theme is needed for this patch.
This is some discrepancy in how the cache tags are written. Should it be
array('local_task' => 1)
orarray('local_task' => TRUE)
?Looks like a couple places we could remove MENU_VISIBLE_IN_BREADCRUMB entries from hook_menu?
There's a change in the ViewSubscriber to look at _title from the request - should it also handle the _title_callback?
Comment #45
tim.plunkettFor the 'theme_name' switch, code like ThemeAccessCheck is expecting the theme name to be {theme}, and that's a reasonable thing to standardize on.
Comment #46
pwolanin CreditAttribution: pwolanin commentedThis converts some more tab, fixes the missing Views UI listing of fields, reverts some minor changes to local tasks to 8.x (no need to change them), and removes cases where the description field was incorrectly copied to the YAML.
Comment #47
pwolanin CreditAttribution: pwolanin commentedSome more tweaks to move things out of hook_menu and define needed tabs.
Also,
_access_mode: 'ALL'
is now the default, right?Comment #49
pwolanin CreditAttribution: pwolanin commentedThis fixes the 2 fails in #47.
Comment #50
tim.plunkettComment #52
dawehnerMost other conversions in this patch don't use the _tab suffix. This is kind of a horrible idea from my point of view as this makes it appear to be more magic.
Comment #53
YesCT CreditAttribution: YesCT commentedI've seen _tab in local tasks before... for example system.local_tasks.yml
and user.local_tasks.yml
and views_ui.local_tasks.yml
etc.
Comment #54
neclimdulI disagree with they're use. If we add them here or don't touch them here or what ever, they'll currently get removed in #2107531: Improve DX of local task YAML definitions.
Comment #55
YesCT CreditAttribution: YesCT commentedok, I'll make a new patch taking out the _tab from the local tasks, and also do an update to the issue summary
Comment #56
YesCT CreditAttribution: YesCT commentedthese are not moved to local tasks, so where did these move to? maybe they are just left over from another conversion?
add is in routing, but what was the breadcrumb thing for? are we just taking those out?
configure/% and configure/%/delete are already in routing.yml
core/modules/action/action.routing.yml
16: path: '/admin/config/system/actions/configure/{action}'
23: path: '/admin/config/system/actions/configure/{action}/delete'
nit: some need a period to make it a sentence. https://drupal.org/node/1354#file
some function one liners needs third person verbs.
"For most functions (see exceptions below), summary lines must start with a third person singular present tense verb, and they must explain what the function does. Example: "Calculates the maximum weight for a list.""
I repeat to myself, order does not matter. But I do notice that usually title comes before tab_root_id
also nit: fixing some classes to have newline before closing brace.
Where did these move to?
https://drupal.org/node/325974
"There is no PHPDoc on this function since it is an inherited method."
but movement on #338403: Use {@inheritdoc} on all class methods (including tests) might help.
why not take out the whole array item?
noting.
I think the newlines are usually used to separate groups, grouped by root_id.
these are out of scope changes. I dont think we have a standard for one word titles and the use of quotes. also, elsewhere in this patch, some are added without the quotes, so no need to add these here.
are these todo's for this patch, or should they be separate issues?
in one spot no cache_tags are set.
in another, they are added.
why?
Comment #56.0
YesCT CreditAttribution: YesCT commentedused template and listed related issues. listed remaining tasks.
Comment #58
dawehnerIn Drupal 7 the default breadcrumb was built using the menu system. MENU_IN_BREADCRUMB is a entry in the menu system which appears in the BREADCRUMB
but not as menu item nor local task etc. As the BREADCRUMB is now built based upon the path and not the menu system, these entries are not needed anymore.
The other example is %/delete which was not marked as something so it is MENU_NORMAL_ITEM aka a menu link entity. It does though not make sense to
have a default menu link with a placeholder as you can't show that anywhere. A menu link entity needs static variables.
Just linking to a discussion about documentation of tests methods. https://drupal.org/node/2108785
Some of these indeed had a wrong change. I went through the full patch and removed all of this accidentally changes.
There is indeed no real reason to not to, though all these tests don't deal with actual caching anyway. Fixed it.
Comment #60
YesCT CreditAttribution: YesCT commented=> TRUE or => 1
@pwolanin brought up this same thing in #44
Comment #60.0
YesCT CreditAttribution: YesCT commentednoting updated as of comment
Comment #61
dawehnerGood catch. I also changed the place where I coped it from.
Comment #62
neclimdulI don't understand the point of supplying cache tags. I consciously didn't supply them because we aren't testing that and the only reason we're adding a cache backend is so it will return empty and trigger the parsing code we're trying to test. If cache tags where required for that it would actually be a bug but more importantly, there is literally no implementation to use them because we are mocking a empty implementation.
Comment #63
pwolanin CreditAttribution: pwolanin commented@dawehner - I think using something like "_tab" or something to distinguish the plugin ID from the route name is helpful.
I don't see why that would make it look like it has any magic? To me it's the opposite - naming it the same as the route name is way more confusing if you are just looking at the file and trying to understand what each string actually means.
Comment #64
dawehnerThere are other places in the ViewSubscriber which should use the title resolver, though this will be fixed by #2068471: Normalize Controller/View-listener behavior with a Page object
Comment #66
dawehnerShame on me.
Comment #68
neclimdul#66: local_tasks-2102125-66.patch queued for re-testing.
Comment #68.0
neclimduladded issue related to the breadcrumbs
Comment #69
pwolanin CreditAttribution: pwolanin commentedI realize there's more to be converted to use the title resolver, but glad to see us at least covering the new use.
With that, plus thorough review by YesCT and removal of a lot of hook_menu cruft I think this is looking ready.
Comment #70
hass CreditAttribution: hass commentedWhere are all the menu descriptions gone?
Comment #71
dawehnerMenu descriptions are just shown on menu links, which are not touched by this patch.
Comment #72
YesCT CreditAttribution: YesCT commentedI was actually wondering the same thing about descriptions. I asked in irc the other day, and didn't get an answer... today @dawehner and I discussed it in irc, and now I have checked.
I checked, for example here. And before the patch there is no description used there anyway. (Not on the page, and not on hover.)
So I think that these descriptions where never used anywhere anyway. So it is ok to remove them.
Comment #73
YesCT CreditAttribution: YesCT commentedre #56 9,
#2099363: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7) will probably handle that @todo question.
Comment #73.0
YesCT CreditAttribution: YesCT commentedupdate long outdated summary.
Comment #74
neclimdulThere are a couple todos in the patch but tim is fixing one in #2099363: Allow single config files to be imported and exported (Resolve regression from Views in Drupal 7) and the others are about expanding testing which is currently sufficient for this patch but should be expanded to replace some web tests.
Comment #75
YesCT CreditAttribution: YesCT commentedtook out the @todo question about importing, that will be covered by the config issue.
and linked in the issue for the other todo.
I checked several more pages like forums, config.
There are some small differences, like there is no-longer a admin/config/development/configuration/sync since that tab is now the root it is just admin/config/development/configuration
and
on the languages add page, there are no tabs for list, detection and selection. But that is probably ok, as it does still get the language in the breadcrumb which takes people back to where the list tab would.
One of the reasons I'm really interested in getting this in soon, is it fixes the problem that tabs are missing from the node pages. For example:
oh crud. I was hoping to show how this is needed to solve this... but it shows a problem also.
Before:
one edit tab that leads to: node/1?entity=1
after:
first edit tab that leads to: node/1/edit
second edit tab leads to: node/1?entity=1 (which is not actually an edit page)
Also, the translate for node 1.. does not have all the tabs it should (like view, outline, the correct edit tab, etc.)
Comment #76
YesCT CreditAttribution: YesCT commentedComment #77
andypostCollision with #2102437: Remove drupal_set_title in comment module controllers and different implementation
Comment #78
neclimdulDon't know what's going on with the entity argument(?entity=1) but it seems to be an existing problem with router weirdness. The extra tab was a hack tim added to get tests passing until this issue happened and we missed it. Updated with test to fix it.
New follow up task for changes made to support testing #2112575: Add a DerivativeBase in the Core namespace with t() as a helper method.
Comment #80
neclimdulDon't have time to finish this up this evening I don't think but that edit task is apparently for when editing under a language so in some form it'll need to get added back probably with some sort of requirement for its visibility on the route that doesn't exist currently.
Comment #81
neclimdulStill haven't sorted out the Edit tab. This fixes the language failures. I'm ashamed.
Leaving as NW rather then adding another failed return because there's nothing interesting to see. ContentTranslationTestBase fails around that edit tab still.
Comment #82
markhalliwellx-ref #2040037: The requested page "/admin/appearance/settings/bartik" could not be found after installation was marked as a dup of this issue
Comment #83
tim.plunkettDidn't make it :(
Comment #84
dawehnerThe problem is the following: the content translation local tasks assume that the route name matches with the local task plugin ID
in order to set the tab_root_id. As @neclimdul suggested before, we should use the route name here, which is more save, though here is a fix without that change.
Therefore we needed also proper $entity_type parameters in the entity_test routes as well as actual implemented local tasks for entity_test.
Comment #86
dawehnerThis also fixes the content translation integration test.
Comment #88
andypostThere's issue #1834002: Configuration delete operations are all over the place that could help to minimize the patch size here by unifying local tasks for content entities
Comment #89
YesCT CreditAttribution: YesCT commentedWe dont even know where to start on that delete operations issue for content yet. So I dont think we should wait on it.
Looks like the next thing here is to investigate the fails.
Comment #90
pwolanin CreditAttribution: pwolanin commentedlooking at the 1st set of exceptions, it seems as though there is an empty string or NULL for $root_id which means someplace there is such a value for
$task_info['tab_root_id']
trying to find the problem locally, I'm seeing it first on path /block/1/translations
The code in \Drupal\content_translation\Plugin\Derivative\ContentTranslationLocalTasks looks pretty problematic - I think changing to groupin gto be route is critical to make this more reliable.
This also seems to be fallout from re-deifining the path in the entity info rather than referencing a route:
Comment #91
dawehnerChecking regular expressions on the route provider caused some issues, so I removed the regex on custom block, see #2116111: When looking up a route based on its path (pattern with slugs) we don't need to apply regex validation or other processing as a followup.
This also adds a couple of tests.
Comment #92
pwolanin CreditAttribution: pwolanin commentedI'm not sure that's the correct interdiff.
From IRC, the relevant change to stop the test fail is:
Comment #93
dawehnerThis time with the proper interdiff.
Comment #94
tim.plunkettThis blocks a half a dozen things, let's get it in!
Comment #95
Tim Bozeman CreditAttribution: Tim Bozeman commentedRerolling!
Comment #96
Tim Bozeman CreditAttribution: Tim Bozeman commentedI removed the conflict:
config.sync:
path: '/admin/config/development/configuration/sync'
defaults:
_form: '\Drupal\config\Form\ConfigSync'
_title: 'Synchronize'
requirements:
_permission: 'synchronize configuration'
from core/modules/config/config.routing.yml
Comment #97
tim.plunkettMissed the _title that was recently added.
Comment #98
Gábor HojtsyGreat stuff! @dawehner pointed out elsewhere that the $route_name + '_tab' naming is not to be assumed anymore for default local tasks. The arguments around the _tab suffix died in/after #63, but the existing tabs that used that naming scheme were left as-is. It would be great to have a followup then to clean up the existing ones so there is consistency? :) NOT in the scope of this patch IMHO, looking forward to the commit :)
Comment #99
catchCommitted/pushed to 8.x, thanks!
Comment #100
Gábor HojtsyFYI working on patch for config_translation deducted from the content_translation procedural alter + static create + alter method approach from the patch at #2119497: Default local tasks are not to be assumed $route_name . '_tab'. This seems to be the kind of code people need if they want to alter a tab in where they don't know the tab name of the tab root.
Comment #101
dawehnerIndeed we need the same procedure in content_translation, field_ui as well as views.
Maybe we could provide some common base class, but I am not sure whether these altering in by path is a common usecase for other modules, what do you think?
Comment #102
Gábor HojtsySounds like 4 core modules is common enough? :)
Comment #103
pwolanin CreditAttribution: pwolanin commentedWe need to get rid of root and use the route name where they appear to group them
Comment #104
Gábor HojtsyI blindly copy-pasted this function for use in #2119497: Default local tasks are not to be assumed $route_name . '_tab' (in config translation) and assumed it would work. Then got very surprised I was getting arrays as return values. Then looked closer, and see the $local task variable is FALSE, then gets assigned an array from foreach, then if the route matches, it will be a string. If no tab matches, you get the array back though :)
May be a followup issue or a followup patch here.
Comment #105
alexpottThis issue has caused PHPUnit to start printing out info.yml files...
Comment #106
tim.plunkettHere's where --strict helps in phpunit.
So I tracked it down to ModuleHandler::load(), the include_once is printing out the file contents somehow... Not sure why.
In LocalTaskIntegrationTest::getLocalTaskManager(), why aren't we mocking the ModuleHandler? We can cheat on the module dirs, and this just sidesteps the whole thing.
Comment #107
tim.plunkettThat's not going to work, thanks content_translation :)
Comment #108
tim.plunkettneclimdul found the actual bug here.
The constant needs to move since block.module loads after BlockFormControllerTest.
Comment #109
dawehnerThis is a nice way to get in this kind of small bits done :)
Comment #110
Mile23+1
Also, once again, I remind folks that we could mock entire filesystems from a dataprovider with vfsStream. :-) #2095037: Add vfsStream as a dependency in composer.json.
Comment #111
dawehnerWell, the point here is to have integration tests not unit tests ... so mocking everything don't really help here :)
Comment #112
Gábor HojtsyWell, my fix from #104 did not make it into the RTBC patch. Do people agree its an issue? The existing tests obviously don't prove it, but IMHO it should be fixed :)
Comment #113
dawehnerOh right.
Comment #114
Gábor HojtsyHere is a roll with #108 and #104 combined, so I put my keyboard where my mouth is. IMHO still RTBC :)
Comment #115
dawehner+1
Comment #116
Mile23+1 for #114 because it applies and makes the yaml output go away. Also works with
--strict
.Comment #117
alexpottCommitted 270708d and pushed to 8.x. Thanks!
Comment #118
XanoThe patch caused #2121461: \Drupal\Core\EventSubscriber\ViewSubscriber::onHtml() does not allow controllers to return string content.
Comment #118.0
Xanoadded related issue for one of the @todos
Comment #119
eliza411 CreditAttribution: eliza411 commentedComment #120
eliza411 CreditAttribution: eliza411 commentedComment #121
dawehner@eliza411
Wait, you just reuploaded the patch from https://drupal.org/comment/7998221#comment-7998221 ... is that maybe just a bug on d.o?
Comment #122
xjmComment #123
YesCT CreditAttribution: YesCT commentedadded #2135777: Follow-up [meta] local task conversion, move out of hook_menu() and to local_tasks.yml
Comment #124
YesCT CreditAttribution: YesCT commentedah, re #121, it wasn't reuploaded, just hidden and then shown.
d.o d7 upgrade: #2125741: The comment marking a file as hidden shows the file in the comment as though you uploaded it
Comment #126
donquixote CreditAttribution: donquixote commentedCustomBlockLocalTasksTest ended up in the wrong folder :)
PHPUnit does not care about PSR-0, it just scans the file contents. This is why this did not break.
I'm going to fix this as part of #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 with format-patch.
Comment #127
sunCreated #2271801: CustomBlockLocalTasksTest is located in wrong directory