Updated: Comment #0

Problem/Motivation

Before:
Before
After:
after

Proposed resolution

I think we should fix it in breadcrumb builder as can be seen in the attached patch #0
@dawehner suggested we should fix it in \Drupal\field_ui\Routing\RouteSubscriber::routes see #1 and http://privatepaste.com/2b58ea47c8.

Remaining tasks

Decide is it ok to fix it in breadcrumb builder? Or do we have to fix fields routing title.

User interface changes

See after image

API changes

None

CommentFileSizeAuthor
#20 interdiff.txt1.52 KBjibran
#20 2147685-20.patch3.62 KBjibran
#14 2147685-14.patch3.58 KBjibran
#14 interdiff.txt1.61 KBjibran
#14 2147685-test-only.patch1.61 KBjibran
#9 interdiff.txt832 bytesjibran
#9 2147685-9.patch1.97 KBjibran
#7 Body-after (2).png3.17 KBjibran
#7 Body settings for Article-before.png3.96 KBjibran
#7 interdiff.txt2.84 KBjibran
#7 2147685-7.patch1.96 KBjibran
#1 Body-after-1.png3.42 KBjibran
#1 2147685-1.patch1.83 KBjibran
fields-breadcrumb-fix.patch913 bytesjibran
Body-after.png3.71 KBjibran
Body-before.png3.93 KBjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue summary: View changes
FileSize
2147685-1.patch1.83 KB
Body-after-1.png3.42 KB

Here is the other approach approved by @dawehner.

New After

After

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Is there a reason for using the FieldUi class instead of the class that displays the form (FieldInstanceEditForm)?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

It is just a static code we can move it around if the approach is fine we can move it to anyplace.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue summary: View changes

The last submitted patch, fields-breadcrumb-fix.patch, failed testing.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

The approach seems fine :) Let's move it to a getTitle() method on FieldInstanceEditForm and also remove $form['#title'] from that form.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
2147685-7.patch1.96 KB
interdiff.txt2.84 KB
Body settings for Article-before.png3.96 KB
Body-after (2).png3.17 KB

Moved \Drupal\field_ui\Form\FieldInstanceEditForm::getTitle. I don't think we should remove $form['#title']

Before Removing

before

After Removing

After
Title is all messed up so I think we should keep it.

amateescu’s picture

amateescu CreditAttribution: amateescu commented

Sure, let's keep it then.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
@@ -70,7 +70,9 @@ protected function routes(RouteCollection $collection) {
+          array('_form' => '\Drupal\field_ui\Form\FieldInstanceEditForm',

'_form' needs to be moved below "array(", on it's own row :)

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
2147685-9.patch1.97 KB
interdiff.txt832 bytes
amateescu’s picture

amateescu CreditAttribution: amateescu commented
Title: Fields settings page breadcrumb is not correct. » Fields settings page breadcrumb is not correct
Component: base system » field_ui.module
Status: Needs review » Reviewed & tested by the community

Thanks!

The last submitted patch, 9: 2147685-9.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +Quick fix

It is a quick fix IMHO.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Can we get a quick test for this?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2147685-test-only.patch1.61 KB
interdiff.txt1.61 KB
2147685-14.patch3.58 KB

Here are some tests.

The last submitted patch, 14: 2147685-test-only.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2147685-14.patch, failed testing.

The last submitted patch, 14: 2147685-14.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Status: Needs work » Needs review

14: 2147685-14.patch queued for re-testing.

amateescu’s picture

amateescu CreditAttribution: amateescu commented 
  1. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUiTestBase.php
@@ -73,6 +73,8 @@ function fieldUIAddNewField($bundle_path, $initial_edit, $field_edit = array(),
+    $this->assertLink($label, 0, 'Breadcrumbs were correct on field settings page.');

    We normally use present tense in test assertions, so how about 'Field label is correct in the breadcrumb of the field settings page.' 

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUiTestBase.php
@@ -129,6 +131,9 @@ function fieldUIDeleteField($bundle_path, $field_name, $label, $bundle_label) {
+    $this->assertLink($label, 0, 'Breadcrumbs were correct on field delete page.');

    And 'Field label is correct in the breadcrumb of the field delete page.'

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
FileSize
2147685-20.patch3.62 KB
interdiff.txt1.52 KB

Fixed. But

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUiTestBase.php
@@ -73,6 +73,8 @@ function fieldUIAddNewField($bundle_path, $initial_edit, $field_edit = array(),
     $this->assertRaw(t('These settings apply to the %label field everywhere it is used.', array('%label' => $label)), 'Field settings page was displayed.');

@@ -129,6 +131,9 @@ function fieldUIDeleteField($bundle_path, $field_name, $label, $bundle_label) {
     $this->assertRaw(t('Are you sure you want to delete the field %label', array('%label' => $label)), 'Delete confirmation was found.');

In my defiance.

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Needs review » Reviewed & tested by the community

I think those examples are fine as they are, but "was correct" sounds to me more like "it was correct at some point but it's not necessarily the case anymore". It's also probably just a matter of preference, so thanks for the quick update :)

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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