Convert field_ui / Entity local tasks to YAML definitions

This 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.

.

#2094097: Convert field_ui to use the new local task plugins was an old issue for that.

Just a damn simple rerole to be able to reset the local git again.

Marking back to needs review since there's a patch. Also! This fixes the "no manage fields tabs on users" problem. Hooray! :D

1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Plugin/Derivative/FieldUiFieldLocalTasks.php
   @@ -0,0 +1,219 @@
   +    // Built a map of entity types and they bundle entity types.
   

   s/they/their|they ?

2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Plugin/Derivative/FieldUiFieldLocalTasks.php
   @@ -0,0 +1,219 @@
   +} ¶
   

   space after }

3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
   @@ -38,10 +38,22 @@ public function __construct(EntityManager $manager) {
   +    // Built a map of entity types and they bundle entity types.
   

   s/they/their|they ?

4. +++ b/core/modules/node/lib/Drupal/node/Plugin/Menu/LocalTask/NodeTypeEdit.php
   @@ -0,0 +1,26 @@
   +} ¶
   

   space after }

There we go ... but yeah don't expect much.

You can't currently get to some major pages in Drupal 8, including Roles and user fields, so this is actually critical IMO.

It 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.

tagging.

I wonder if we could move this into $entity_info['links']. It'd be nice to keep those in that one place...

We could, but that means doing #2120005: Provide an easy way to figure out the bundle entity type of another entity type. first.

It 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.

1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.php
   @@ -66,7 +78,8 @@ public function __construct(EntityManagerInterface $entity_manager) {
   @@ -77,6 +90,9 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
   
   @@ -77,6 +90,9 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
   +    if (!$bundle) {
   +      $bundle = $this->request->attributes->get('_raw_variables')->get($entity_info['bundle_entity_type']);
   +    }
   

   You know, we could also inject the _raw_variables into the buildForm() method, without any additional effort.

2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
   @@ -40,8 +51,14 @@ public function __construct(EntityManagerInterface $manager) {
   +          // Skip this entity type if the route was not found.
   +          continue;
   

   Should we not try to throw a more helpful exception here? Hiding things is kind of evil.

Also, OverviewBase has $this->getRequest from FormBase, no need for injection. Or what @dawehner said, $_raw_variables = NULL in the method should do it.

Here 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:

• Allow the local task default, to also pull the values, from the defaults var. This changes the route_parameters to potentially
  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.
• A different approach could be to pull the defaults from the routes in the ParamConverterManager as well and use it in the LocalTaskDefault
• Another maybe just orthogonal approach: Don't add a new route for local task here (each view_mode/form_mode) but just use one for all of them and put {mode} into the path.

Note 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.

I've tried this approach:

Another maybe just orthogonal approach: Don't add a new route for local task here (each view_mode/form_mode) but just use one for all of them and put {mode} into the path.

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 to admin/structure/types/manage/article/display/teaser to observe the problem :/

What 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.

Some more progress..

Even more progress.

1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
   @@ -317,10 +317,11 @@ public function getAdminPath($entity_type, $bundle) {
   +    $bundle_arg = isset($entity_info['bundle_entity_type']) ? $entity_info['bundle_entity_type'] : 'bundle';
   

   Should this go into the EntityType annotation as a default?

2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
   @@ -133,4 +147,13 @@ protected function routes(RouteCollection $collection) {
   +    $events[RoutingEvents::DYNAMIC] = array('onDynamicRoutes', -100);
   

   So, content_translation and field_ui have the same priority now. Is that going to be a problem?

1. Why not :) In the next reroll.
2. Nope, because content_translation doesn't provide any (content) entity type of its own.

Almost 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..

The 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?

Oh! 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.

+++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigFieldInstanceMapper.php
@@ -23,13 +23,10 @@ class ConfigFieldInstanceMapper extends ConfigEntityMapper {
    */
   public function getBaseRouteParameters() {
     $parameters = parent::getBaseRouteParameters();
-    // @todo All core content entity path placeholders can be fully filled in
-    //   with an additional {bundle} value in their paths, but a more
-    //   predictable solution would be ideal. See
-    //   https://drupal.org/node/2134871
+    $base_entity_info = $this->entityManager->getDefinition($this->pluginDefinition['base_entity_type']);
     // @todo Field instances have no method to return the bundle the instance is
     //   attached to. See https://drupal.org/node/2134861
-    $parameters['bundle'] = $this->entity->bundle;
+    $parameters[$base_entity_info['bundle_entity_type']] = $this->entity->bundle;
     return $parameters;
   }

Why is this change is needed?

[5:08pm] GaborHojtsy: amateescu: I don't understand the change in the config translation code 
[5:08pm] GaborHojtsy: amateescu: ie. how it is related
[5:09pm] amateescu: GaborHojtsy: in the field_ui issue?
[5:09pm] GaborHojtsy: amateescu: yup
[5:09pm] GaborHojtsy: amateescu: posted a comment asking that question too
[5:09pm] amateescu: GaborHojtsy: it's related because that patch introduces a "unified" way of getting the 'bundle' parameter for content entity links
[5:10pm] amateescu: GaborHojtsy: if you look at the whole patch, you'll see it 
[5:10pm] amateescu: it is confusing only by looking at the interdiff
[5:10pm] GaborHojtsy: amateescu: ha, ok 

Looks good looking with this info now :)

Because 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.)

That is amazing BTW, the upcasting will validate the paths work, and you cannot enter 'd8rulz' in place of the bundle part anymore then :P

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php
@@ -301,9 +301,9 @@ public function getAdminPath($entity_type, $bundle) {
+      $admin_path = \Drupal::urlGenerator()->getPathFromRoute($entity_info['links']['admin-form'], $route_parameters);

Ideally 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()

I 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.

Oh yeah, that would happen.
But my point was to NOT inject it, just mark getAdminPath as @deprecated in the next pass.

I'm hoping the next pass will be a commit. Care to help with that? :)

Marking getAdminPath() as deprecated.

Ahem, that's embarassing.

Thanks for adding that!

Hey d.o, stop it.

1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php
   @@ -85,7 +85,16 @@ public function getRegions() {
   +    $raw_variables = $this->getRequest()->attributes->get('_raw_variables')->all();
   +    if (isset($raw_variables['form_mode_name'])) {
   +      $this->mode = $raw_variables['form_mode_name'];
   +    }
   +    elseif (isset($raw_variables['view_mode_name'])) {
   +      $this->mode = $raw_variables['view_mode_name'];
   +    }
   +    else {
   +      $this->mode = 'default';
   +    }
   

   We 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.

2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.php
   @@ -193,12 +193,7 @@ public function submitForm(array &$form, array &$form_state) {
   -      $form_state['redirect_route'] = array(
   -        'route_name' => 'field_ui.overview_' . $this->instance->entity_type,
   -        'route_parameters' => array(
   -          'bundle' => $this->instance->bundle,
   -        )
   -      );
   +      $form_state['redirect_route'] = $this->entityManager->getAdminRouteInfo($this->instance->entity_type, $this->instance->bundle);
   

   <3

3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Plugin/Derivative/FieldUiLocalTask.php
   @@ -0,0 +1,231 @@
   +   * Alters the tab_root_id definition for field_ui local tasks.
   +   */
   +  public function alterLocalTasks(&$local_tasks) {
   

   This could have some documentation.

Excellent points, thanks!

Awesome!

49: local_tasks-2111823-49.patch queued for re-testing.

Need to go look for that brown paper bag :/

54: local_tasks-2111823-54.patch queued for re-testing.

And we're green again.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Plugin/Derivative/FieldUiLocalTask.php
@@ -0,0 +1,234 @@
+        foreach (entity_get_form_modes($entity_type) as $form_mode => $form_mode_info) {

entity_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!

Seems head was broken on this: https://qa.drupal.org/8.x-status

It was already critical, so...

@tstoeckler, I am getting different HEAD failures with each retest. Have you confirmed particular failures locally? If so please put that info here. :)

So 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.

This 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?

Looks like we still have some troubles here, see #2142553: Field UI Local tasks are incorrect on any entity

@Xano: #2113345: Define a mechanism for custom link relationships