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.
Follow up to #2102125: Big Local Task Conversion to convert fieldable entity tasks to the plugin system.
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff.txt | 1.27 KB | amateescu |
#54 | local_tasks-2111823-54.patch | 52.46 KB | amateescu |
Comments
Comment #1
dawehnerThis converts the actual tabs, though the price to pay is that we cannot longer use 'bundle' in the field UI pattern but should better fallback to the actual bundle entity types.
Sadly you have to iterate over all entity types, as the relation is from node_type to node.
Comment #3
dawehner.
Comment #4
dawehner#2094097: Convert field_ui to use the new local task plugins was an old issue for that.
Comment #5
dawehnerJust a damn simple rerole to be able to reset the local git again.
Comment #6
webchickMarking back to needs review since there's a patch. Also! This fixes the "no manage fields tabs on users" problem. Hooray! :D
Comment #7
swentel CreditAttribution: swentel commenteds/they/their|they ?
space after }
s/they/their|they ?
space after }
Comment #9
dawehnerThere we go ... but yeah don't expect much.
Comment #11
webchickYou can't currently get to some major pages in Drupal 8, including Roles and user fields, so this is actually critical IMO.
Comment #12
xjmComment #13
Gábor HojtsyIt is not possible to add fields to accounts for months now due to this lack of conversion and also config translation cannot add a tab to field instance translation (less of a problem since you have other entry points to do that, unlike fields). I wanted to elevate this issue, but its already crit, so just saying.
Closed #2109937: Manage fields local tasks not displayed at admin/config/people/accounts down as duplicate.
Not sure how the page callback issue is a parent (local tasks don't need page callbacks, they are already route based), but added a local task creation meta as related at least.
Comment #14
YesCT CreditAttribution: YesCT commentedtagging.
Comment #15
tim.plunkettI wonder if we could move this into $entity_info['links']. It'd be nice to keep those in that one place...
Comment #16
amateescu CreditAttribution: amateescu commentedWe could, but that means doing #2120005: Provide an easy way to figure out the bundle entity type of another entity type. first.
Comment #17
amateescu CreditAttribution: amateescu commentedIt would've been nice of me to remember about this issue before starting to work on it locally, but alas I didn't so here's my progress on a patch written from scratch. This also moves
$route_base_path
to the $links property and incorporates #2120005: Provide an easy way to figure out the bundle entity type of another entity type., which I'm going to close as a dup now.Comment #18
dawehnerYou know, we could also inject the _raw_variables into the buildForm() method, without any additional effort.
Should we not try to throw a more helpful exception here? Hiding things is kind of evil.
Comment #19
tim.plunkettAlso, OverviewBase has $this->getRequest from FormBase, no need for injection. Or what @dawehner said, $_raw_variables = NULL in the method should do it.
Comment #20
dawehnerHere is the problem, but I would like to talk with someone about this problem, as this affects potentially a lot of different regions
of the code.
The problem is the following: The view_mode/form_mode does not appear as parameter in the URL, but just in the defaults of the route,
so LocalTaskDefault::getRouteParameters() does not return them and so checking access to the tab fails, as view_mode is NULL.
Some possible fixes with potential problems:
have more variables than needed. I tried that with a nice unit test.(Note, other ones don't break), though this breaks the active handling in the actual UI.
Comment #22
Gábor HojtsyNote that #2135787: Move static config entity local tasks to local_tasks.yml converts the base local tasks of the config entities where the this patch converts the field UI tasks. I think these two issues should be committed at once, since committing that before this one would hide all the field UI tabs and likely committing this one would make all the tabs not show up properly either.
I do think it makes sense to maintain them as two separate issues for the sake of the other one being easier to review for example and also the extent of changes / new code needed.
Comment #23
amateescu CreditAttribution: amateescu commentedI've tried this approach:
And it kinda works but I think there's a problem in
LocalTaskManager::getLocalTasksForRoute()
because now we're using the same route for two different level of tabs. For manual testing, just go toadmin/structure/types/manage/article/display/teaser
to observe the problem :/Comment #25
amateescu CreditAttribution: amateescu commentedWhat works though is creating a different route for the individial form_mode/view_mode tabs, and it's still better than one route per form_mode/view_mode. Let's see if converting a couple more tasks brings down the number of failures.
Comment #27
amateescu CreditAttribution: amateescu commentedSome more progress..
Comment #29
amateescu CreditAttribution: amateescu commentedEven more progress.
Comment #30
tim.plunkettShould this go into the EntityType annotation as a default?
So, content_translation and field_ui have the same priority now. Is that going to be a problem?
Comment #31
amateescu CreditAttribution: amateescu commented1. Why not :) In the next reroll.
2. Nope, because content_translation doesn't provide any (content) entity type of its own.
Comment #33
amateescu CreditAttribution: amateescu commentedAlmost there. The only failure left should be ConfigTranslationListUiTest, which I have no idea how to fix because in regular site operation the translate tab is there and the route works..
Comment #34
Gábor HojtsyThe config translation module will only add the tab if there is a tab root, and many of the config entities lack a tab root (in the new system) without #2135787: Move static config entity local tasks to local_tasks.yml. Maybe that would need to be added as well in this patch?
Comment #35
amateescu CreditAttribution: amateescu commentedOh! I was wrong, it was just a local failure :)
@Gábor Hojtsy, the needed tabs (for config entities which provide bundles for content entities) are already added by this patch, otherwise it wouldn't work. This means they have to be removed from the other issue.
Comment #36
Gábor HojtsyWhy is this change is needed?
Comment #37
Gábor HojtsyLooks good looking with this info now :)
Comment #38
amateescu CreditAttribution: amateescu commentedBecause the patch gets rid of the assumtion of a {bundle} parameter in field_ui routes and uses the actual bundle type in it's paths ({node_type}, {taxonomy_vocabulary}, etc.)
Comment #39
Gábor HojtsyThat is amazing BTW, the upcasting will validate the paths work, and you cannot enter 'd8rulz' in place of the bundle part anymore then :P
Comment #40
tim.plunkettIdeally we'd inject this, but I think we should actually mark getAdminPath() deprecated and encourage using getAdminRouteInfo instead. I spot checked the usage of getAdminPath, and almost all of them are passed to url() or l()
Comment #41
amateescu CreditAttribution: amateescu commentedI tried to inject the url generator, see the patch from #25 and the fails for Drupal\rest\Tests\AuthTest (https://qa.drupal.org/pifr/test/672528). There's some circular dependency going on there.
Comment #42
tim.plunkettOh yeah, that would happen.
But my point was to NOT inject it, just mark getAdminPath as @deprecated in the next pass.
Comment #43
amateescu CreditAttribution: amateescu commentedI'm hoping the next pass will be a commit. Care to help with that? :)
Comment #44
amateescu CreditAttribution: amateescu commentedMarking getAdminPath() as deprecated.
Comment #45
amateescu CreditAttribution: amateescu commentedAhem, that's embarassing.
Comment #46
tim.plunkettThanks for adding that!
Comment #47
tim.plunkettHey d.o, stop it.
Comment #48
dawehnerWe seem to have two overview classes, one for form modes, one for display modes, so why not have specialized code in both of them? This can be a follow up.
<3
This could have some documentation.
Comment #49
amateescu CreditAttribution: amateescu commentedExcellent points, thanks!
Comment #50
dawehnerAwesome!
Comment #52
Gábor Hojtsy49: local_tasks-2111823-49.patch queued for re-testing.
Comment #54
amateescu CreditAttribution: amateescu commentedNeed to go look for that brown paper bag :/
Comment #56
amateescu CreditAttribution: amateescu commented54: local_tasks-2111823-54.patch queued for re-testing.
Comment #57
amateescu CreditAttribution: amateescu commentedAnd we're green again.
Comment #58
catchentity_get_form_modes() should likely move into the entity manager somewhere, but it's not there now.
Otherwise I didn't really have any comments. We've got a similar problem to routes where declaring dynamic tasks is a completely different workflow/format altogether, but that's nothing to do with this patch.
Committed/pushed to 8.x, thanks!
Comment #59
tstoecklerSeems head was broken on this: https://qa.drupal.org/8.x-status
It was already critical, so...
Comment #60
xjm@tstoeckler, I am getting different HEAD failures with each retest. Have you confirmed particular failures locally? If so please put that info here. :)
Comment #61
tstoecklerSo my assumption was: HEAD broken -> The last commit broke HEAD.
I was informed that this is infact not a safe assumption. Marking as fixed again. Sorry for the noise.
Comment #62
XanoThis patch introduced the
admin-form
link relation for entities, but http://www.iana.org/assignments/link-relations/link-relations.xml doesn't mention that relation at all, and according the change record this is still the only list of allowable relations. Can someone shed some light on this?Comment #63
swentel CreditAttribution: swentel commentedLooks like we still have some troubles here, see #2142553: Field UI Local tasks are incorrect on any entity
Comment #64
Berdir@Xano: #2113345: Define a mechanism for custom link relationships