Comments

dawehner’s picture

Status: Active » Needs review
FileSize
18.24 KB
FAILED: [[SimpleTest]]: [MySQL] 58,518 pass(es), 282 fail(s), and 254 exception(s). View

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.

Status: Needs review » Needs work

The last submitted patch, local_tasks-2111823-1.patch, failed testing.

dawehner’s picture

Title: Convert Entity local tasks to YAML definitions » Convert field_ui / Entity local tasks to YAML definitions

.

dawehner’s picture

dawehner’s picture

FileSize
17.87 KB
FAILED: [[SimpleTest]]: [MySQL] 58,922 pass(es), 275 fail(s), and 250 exception(s). View

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

webchick’s picture

Status: Needs work » Needs review

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

swentel’s picture

  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 }

Status: Needs review » Needs work

The last submitted patch, local_tasks-2111823-5.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
17.87 KB
FAILED: [[SimpleTest]]: [MySQL] 59,064 pass(es), 275 fail(s), and 250 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, local_tasks-2111823-8.patch, failed testing.

webchick’s picture

Priority: Normal » Critical

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

xjm’s picture

Issue summary: View changes
Parent issue: » #2107533: Remove {menu_router}
Gábor Hojtsy’s picture

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.

YesCT’s picture

Issue tags: +MenuSystemRevamp, +WSCCI

tagging.

tim.plunkett’s picture

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

amateescu’s picture

amateescu’s picture

Component: menu.module » field_ui.module
Assigned: Unassigned » amateescu
Status: Needs work » Needs review
FileSize
36.15 KB
FAILED: [[SimpleTest]]: [MySQL] 57,835 pass(es), 147 fail(s), and 29 exception(s). View

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.

dawehner’s picture

  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.

tim.plunkett’s picture

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

dawehner’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 17: local_tasks-2111823-17.patch, failed testing.

Gábor Hojtsy’s picture

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.

amateescu’s picture

Status: Needs work » Needs review
FileSize
37.09 KB
FAILED: [[SimpleTest]]: [MySQL] 57,847 pass(es), 127 fail(s), and 28 exception(s). View
17.68 KB

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

Status: Needs review » Needs work

The last submitted patch, 23: local_tasks-2111823-23.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
41.12 KB
FAILED: [[SimpleTest]]: [MySQL] 57,810 pass(es), 136 fail(s), and 32 exception(s). View
8.99 KB

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.

Status: Needs review » Needs work

The last submitted patch, 25: local_tasks-2111823-25.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
41.78 KB
FAILED: [[SimpleTest]]: [MySQL] 57,897 pass(es), 86 fail(s), and 27 exception(s). View
7.21 KB

Some more progress..

Status: Needs review » Needs work

The last submitted patch, 27: local_tasks-2111823-27.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
44.9 KB
FAILED: [[SimpleTest]]: [MySQL] 58,691 pass(es), 84 fail(s), and 26 exception(s). View
9.69 KB

Even more progress.

tim.plunkett’s picture

  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?

amateescu’s picture

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

Status: Needs review » Needs work

The last submitted patch, 29: local_tasks-2111823-29.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
49.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,818 pass(es). View
8.88 KB

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

Gábor Hojtsy’s picture

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?

amateescu’s picture

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.

Gábor Hojtsy’s picture

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

Gábor Hojtsy’s picture

[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 :)

amateescu’s picture

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

Gábor Hojtsy’s picture

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

tim.plunkett’s picture

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

amateescu’s picture

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.

tim.plunkett’s picture

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

amateescu’s picture

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

amateescu’s picture

FileSize
0 bytes
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
668 bytes

Marking getAdminPath() as deprecated.

amateescu’s picture

FileSize
50.44 KB
PASSED: [[SimpleTest]]: [MySQL] 59,005 pass(es). View

Ahem, that's embarassing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for adding that!

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community

Hey d.o, stop it.

dawehner’s picture

  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.

amateescu’s picture

FileSize
52.44 KB
FAILED: [[SimpleTest]]: [MySQL] 58,889 pass(es), 160 fail(s), and 42 exception(s). View
3.91 KB

Excellent points, thanks!

dawehner’s picture

Awesome!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 49: local_tasks-2111823-49.patch, failed testing.

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work

The last submitted patch, 49: local_tasks-2111823-49.patch, failed testing.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
52.46 KB
PASSED: [[SimpleTest]]: [MySQL] 59,096 pass(es). View
1.27 KB

Need to go look for that brown paper bag :/

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 54: local_tasks-2111823-54.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

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

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

And we're green again.

catch’s picture

Status: Reviewed & tested by the community » Fixed
+++ 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!

tstoeckler’s picture

Title: Convert field_ui / Entity local tasks to YAML definitions » HEAD BROKEN: Convert field_ui / Entity local tasks to YAML definitions
Status: Fixed » Active

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

It was already critical, so...

xjm’s picture

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

tstoeckler’s picture

Title: HEAD BROKEN: Convert field_ui / Entity local tasks to YAML definitions » Convert field_ui / Entity local tasks to YAML definitions
Status: Active » Fixed

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.

Xano’s picture

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?

swentel’s picture

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

Berdir’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.