Updated: Comment #11

Problem/Motivation

getAdminPath() is deprecated, it should be removed.
getAdminRouteInfo() points to the wrong route, it should be fixed

Proposed resolution

Add a FieldUI helper for the field overview page

Remaining tasks

N/A

User interface changes

N/A

API changes

getAdminPath() is removed, getAdminRouteInfo() now points to the bundle edit page (admin/structure/types/manage/article) instead of the field overview (admin/structure/types/manage/article/fields)

Original report by @Xano

Replace entity manager's getAdminPath() with getAdminRouteInfo() in Field UI as part of our effort to replace all path handling with route handling.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +MenuSystemRevamp, +WSCCI
FileSize
26.9 KB

getAdminRouteInfo() was actually pointing to the field UI overview page, that was my fault way back in #1963394: ConfirmFormBase::getCancelPath() should allow for a route

Crell’s picture

Issue tags: -WSCCI

This doesn't feel WSCCI related to me...

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

It's just menu stuff. Needs some reworking though.

tim.plunkett’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
26.78 KB

Comments 2-8 were just retests and fails, I removed them.

Status: Needs review » Needs work

The last submitted patch, 11: route-2141329-11.patch, failed testing.

The last submitted patch, 11: route-2141329-11.patch, failed testing.

The last submitted patch, 11: route-2141329-11.patch, failed testing.

The last submitted patch, 11: route-2141329-11.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.39 KB
28.96 KB

It was a subdir install issue...

Status: Needs review » Needs work

The last submitted patch, 16: field_ui-2141329-16.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

16: field_ui-2141329-16.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 16: field_ui-2141329-16.patch, failed testing.

tim.plunkett’s picture

FileSize
28.99 KB

Rerolled.

Xano’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

FileSize
28.12 KB

Thanks Xano :)

Reroll.

dawehner’s picture

+++ b/core/modules/field/lib/Drupal/field/Entity/FieldInstance.php
@@ -511,21 +511,25 @@ public function isTranslatable() {
+  protected function uriPlaceholderReplacements() {
+    if (empty($this->uriPlaceholderReplacements)) {

I am not sure whether I can support that crappyness. Why do we have the url generator if we don't use it at all and rebuild it based on pure string replacement. I know it is kind of out of scope of this issue, though is changes part of it and moves it to the base class.

tim.plunkett’s picture

Status: Needs review » Needs work

The last submitted patch, 22: admin-path-2141329-22.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

22: admin-path-2141329-22.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community
  1. +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    @@ -296,29 +296,16 @@ public function getForm(EntityInterface $entity, $operation = 'default', array $
    -  public function getAdminRouteInfo($entity_type, $bundle) {
    
    +++ b/core/lib/Drupal/Core/Entity/Entity.php
    index bffc91b..b26ed40 100644
    --- a/core/lib/Drupal/Core/Entity/EntityManager.php
    
    --- a/core/lib/Drupal/Core/Entity/EntityManager.php
    +++ b/core/lib/Drupal/Core/Entity/EntityManager.php
    

    Wow, I would have not expected that!

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldListController.php
    @@ -111,8 +111,12 @@ public function buildRow(EntityInterface $field) {
    +        $usage[] = \Drupal::l($this->bundles[$field->entity_type][$bundle]['label'], $route_info['route_name'], $route_info['route_parameters']);
    

    I just wonder why we don't inject stuff properly here.

tim.plunkett’s picture

1) Yeah, me neither. Glad to have it fixed before it breaks.

2) I didn't inject it because I thought there was an issue about list controllers getting the link/url generator passed to it, but I can't find that. I'll look more and open an issue if there isn't one.

tim.plunkett’s picture

22: admin-path-2141329-22.patch queued for re-testing.

webchick’s picture

Unfortunately, needs a re-roll at this point. I'll try and get it in tonight/tomorrow.

Went through a bunch of dumb questions with Tim on IRC, the only one that means any work for this patch is:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php
@@ -105,8 +105,8 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
+      drupal_set_message($this->t('There are no fields yet added. You can add new fields on the <a href="@link">Manage fields</a> page.', array('@link' => $this->url($route_info['route_name'], $route_info['route_parameters']))), 'warning');
...
+          drupal_set_message($this->t('The %display_mode mode now uses custom display settings. You might want to <a href="@url">configure them</a>.', array('%display_mode' => $display_mode_label, '@url' => $this->url($route['route_name'], $route['route_parameters'], $route['options']))));

The latter is passing $route_info['options'], the former is not. (Actually nowhere else seems to pass 'options' either in a quick scan.)

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Meant to do this.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
28.18 KB
2.03 KB

Happily rerolled after the excellent #2144919: Allow widgets and formatters for base fields to be configured in Field UI.

Only change was ensuring that we pass the options along to everything, as pointed out by @webchick.

chx’s picture

Is there a followup to get rid of $this->url($route_info['route_name'], $route_info['route_parameters'], $route_info['options'])

tim.plunkett’s picture

Yep

The last submitted patch, 32: admin-path-2141329-30.patch, failed testing.

tim.plunkett’s picture

FileSize
28.21 KB
522 bytes

Ahem.

webchick’s picture

Title: Replace getAdminPath() with getAdminRouteInfo() in Field UI » Change notice: Replace getAdminPath() with getAdminRouteInfo() in Field UI
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Ok great, getting this in while it's hot. :)

Committed and pushed to 8.x. Thanks!

Marking for change notice.

dawehner’s picture

tim.plunkett’s picture

Title: Change notice: Replace getAdminPath() with getAdminRouteInfo() in Field UI » Replace getAdminPath() with getAdminRouteInfo() in Field UI
Assigned: tim.plunkett » Unassigned
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

Status: Fixed » Closed (fixed)

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