CommentFileSizeAuthor
#61 field-ui-1946404-61.patch105.09 KBtim.plunkett
#61 interdiff-from-53.txt2.77 KBtim.plunkett
#59 field-ui-1946404-59.patch105.73 KBtim.plunkett
#59 interdiff.txt594 bytestim.plunkett
#56 interdiff.txt4.68 KBtim.plunkett
#56 field-ui-1946404-55.patch105.71 KBtim.plunkett
#53 field-ui-1946404-53.patch105.18 KBamateescu
#53 interdiff.txt1.4 KBamateescu
#52 field-ui-1946404-52.patch105.14 KBtim.plunkett
#52 interdiff.txt1.78 KBtim.plunkett
#51 field-ui-1946404-51.patch105.13 KBtim.plunkett
#51 interdiff.txt18.88 KBtim.plunkett
#46 field-ui-1946404-46.patch102.94 KBamateescu
#46 interdiff.txt862 bytesamateescu
#45 field-ui-1946404-45.patch102.94 KBtim.plunkett
#45 interdiff.txt898 bytestim.plunkett
#43 field-ui-1946404-43.patch102.06 KBtim.plunkett
#43 interdiff.txt3.5 KBtim.plunkett
#40 field-ui-1946404-40.patch102.79 KBtim.plunkett
#39 field-ui-1946404-39.patch102.91 KBtim.plunkett
#39 interdiff.txt1.51 KBtim.plunkett
#37 interdiff.txt1.73 KBtim.plunkett
#37 field-ui-1946404-37.patch102.94 KBtim.plunkett
#36 field-ui-1946404-36.patch101.91 KBswentel
#36 interdiff.txt2.17 KBswentel
#33 field-ui-1946404-33.patch101.67 KBtim.plunkett
#33 interdiff.txt1.41 KBtim.plunkett
#31 field-ui-1946404-31.patch101.36 KBtim.plunkett
#31 interdiff.txt5.99 KBtim.plunkett
#24 field-ui-1946404-24.patch100.63 KBtim.plunkett
#24 interdiff.txt5.27 KBtim.plunkett
#21 field-ui-1946404-21.patch99.31 KBtim.plunkett
#21 interdiff.txt9.19 KBtim.plunkett
#19 field-ui-1946404-19.patch91.53 KBtim.plunkett
#17 field_ui-1946404-17.patch61.52 KBtim.plunkett
#9 field_ui-1946404-9.patch284.16 KBtim.plunkett
#7 field_ui-1946404-7.patch275.3 KBtim.plunkett
#7 field_ui-1946404-7-do-not-test.patch61.4 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Working on this.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

Yeah, nevermind. The menu item is in a foreach, and I don't have the bandwidth to figure out EventSubscriberInterface to write a RouteSubscriber.

kim.pepper’s picture

Would this be a route enhancer now? Any good examples of this already?

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Active » Postponed

Okay, this actually wasn't hard because of the EventSubscriber, but because #1735118: Convert Field API to CMI isn't in yet. This will be much more straightforward with that in.

tim.plunkett’s picture

Title: Convert confirm_form() in field_ui.admin.inc to the new form interface » Convert forms in field_ui.admin.inc to the new form interface

Working on this, it turns out that it will be very hard to do only the ONE confirm_form(), and leave the other forms, due to the foreach in hook_menu(). So this will now encompass the 6 form callbacks at once.

Crell’s picture

See the section "Dynamic Paths" in the change notice for how to port the foreach loop: http://drupal.org/node/1800686

(That is, unless it's possible to eliminate the foreach and make it a single, more flexible definition. That's always preferable.)

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
61.4 KB
275.3 KB

Just to see what happens.
It's possible that breadcrumbs are completely broken for dynamic routes.

tim.plunkett’s picture

FileSize
284.16 KB

Hm.

jthorson’s picture

From the testbot apache logs:

PHP Fatal error: Call to a member function id() on a non-object in /core/modules/menu/menu.module on line 209

swentel’s picture

Status: Needs review » Needs work
Issue tags: +Field API

tagging

effulgentsia’s picture

Priority: Normal » Major
mgifford’s picture

@jthorson I'm guessing that this is because of:

+  /**
+   * Overrides \Drupal\Core\Entity\Entity::id().
+   */
+  public function id() {
+    return $this->entity_type . '.' . $this->bundle . '.' . $this->field_name;
+  }

I had a problem apply the code to my git repo. I think a lot has changed recently.

tim.plunkett’s picture

@mgifford that code isn't anywhere near the menu module...

I'm working on rerolling this.

mgifford’s picture

Thanks @tim.plunkett it's a big patch, thanks for doing the re-roll. But ya, you hadn't touched menu.module so was trying to look at what seemed like the next logical place.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
61.52 KB

So this is just a reroll because the last patch included the entire Field API as CMI conversion.

This needs a complete overhaul, because all of the routes are wrong and broken.

It will likely incorporate something like #1783964: Allow entity types to provide menu items or #1970360: Entities should define URI templates and standard links.

tim.plunkett’s picture

FileSize
91.53 KB

Ooookay. This was interesting.
It still needs a lot of cleanup, but I want to get it green first.

tim.plunkett’s picture

I think EnableDisableTest will still fail, but this is a start.

tim.plunkett’s picture

Um, bot, I wanted those tags

tim.plunkett’s picture

FileSize
5.27 KB
100.63 KB

Missed a spot, and figured out the EnableDisableTest

yched’s picture

Yay & double yay !

Just began skimming through the patch, and I'm fairly new to the new routing system :
Can you expand a bit on the changes made on the menu paths building logic ?
Like :
- in hook_entity_bundle_info(), the 'path', 'bundle argument', 'access callback' entries are not needed anymore ?
- admin/structure/types/manage/article/fields/field_foo --> admin/structure/types/manage/article/fields/node.article.field_foo
Ouch :-/. Is there any way we could avoid putting config ids in URLs ?

As for the form callbacks themselves: is this just moved around, or did this require more significant changes ?

tim.plunkett’s picture

I didn't make any changes to the form callbacks, other than storing $instance as $this->instance, instead of putting it in $form_state['instance'], $form['#instance'], or just calling field_info_instance() a lot (all of which were done).

----

For Field UI paths, we currently have something like:

admin/structure/types/manage/%node_type/fields/%field_ui_instance/edit
It also has 'load arguments' => array($entity_type, $bundle_arg, $bundle_pos, '%map') in the hook_menu.

When visiting admin/structure/types/manage/article/field_body/edit
node_type_load() turns 'article' into an object, and field_ui_instance_load() takes the object and 'field_body' and the load arguments and loads a field instance.

In order to avoid adding more ParamConverters, I've switched the path to
admin/structure/types/manage/article/node.article.field_body/edit
since node.article.field_body is the ID of the FieldInstance entity

But then we have the bundle (article) repeated twice, and an uglier URL.

I'm not sure that routes can be bent to suit our original implementation, or if its worth it since ParamConverters are vaguely expensive

Crell’s picture

I'm A-OK with shifting the URI around to be easier. My main concern would be keeping breadcrumbs sane within the Field UI pages, which Tim informs me he was able to do. (I've not verified.) I didn't go over the patch in detail, but on a skim through it all looks fairly sane to me.

A few quick notes while I'm at it:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
@@ -0,0 +1,83 @@
+    foreach (entity_get_info() as $entity_type => $entity_info) {

entity_get_info() should be replaced with whatever its injectable modern version is.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.php
@@ -0,0 +1,83 @@
+        $route = new Route("$path/display", array(
+          '_form' => '\Drupal\field_ui\DisplayOverview',
+          'view_mode' => 'default') + $defaults, array(
+          '_permission' => 'administer ' . $entity_type . ' display'));

The array merging syntax here took me a while to grok. Without PHP-aware syntax highlighting I can't really follow it. Changing the whitespace structure is probably sufficient to make it grokable.

tim.plunkett’s picture

I should go back through the classes looking for injectable versions of functions.

That Route syntax is CRAZY. I copy/pasted from somewhere else. Glad to know it's not important to someone if we fix it.

yched’s picture

Not sure what to infer from the last exchanges with @Crell. Do we have a hope to get back to non-ugly URLs short-term ?
If not, I'm fine with getting this committed with its current URLs and see if this can be made better in a followup.

Thrilled at how this will make it easier to finally rename field_ui.module to entity_ui.module (since that's what it is, really).

(Still not a real review, though. Will try to do that asap)

core/modules/comment/lib/Drupal/comment/Plugin/Core/Entity/Comment.php:
+ *   entity_menu_base_path = "admin/structure/types/manage/{bundle}/comment",

That info is only correct if {bundle} is the node type, not the actual comment bundle name (comment bundles are 'comment_node_[node_type]'). I don't think that's the case ? So, not sure that works ?

tim.plunkett’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.phpundefined
@@ -43,20 +43,17 @@
+    // Seriously, I hate comment module.
+    if ($entity_type == 'comment') {
+      $bundle = "comment_node_$bundle";
+    }

Yeah I added this to handle that, because WOW this is bizarre.

tim.plunkett’s picture

FileSize
5.99 KB
101.36 KB

Reroll to address Crell's feedback. Still haven't fixed the comment hack.

tim.plunkett’s picture

FileSize
1.41 KB
101.67 KB

Fixed tests and clarified the workaround for comment module.

swentel’s picture

#33: field-ui-1946404-33.patch queued for re-testing.

swentel’s picture

FileSize
2.17 KB
101.91 KB

So, here's a solution for the comment hack, I'm not sure if this is the way to go or not, but it works at least.

tim.plunkett’s picture

FileSize
102.94 KB
1.73 KB

Nice, I like that. Here it is with DI.

tim.plunkett’s picture

tim.plunkett’s picture

FileSize
102.79 KB

Reroll for the various entity/annotation commits.

yched’s picture

Only got minor remarks :

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Routing/RouteSubscriber.phpundefined
@@ -0,0 +1,116 @@
+  /**
+   * @todo.
+   *
+   * @param \Drupal\Core\Entity\EntityManager $manager
+   *   The entity type manager.
+   */
+  public function __construct(EntityManager $manager) {

"Constructs a RouteSubscriber object." ?

Not for this patch either but: core seems to establish a kind of pattern that all subscriber classes to RoutingEvents::DYNAMIC are all called just RouteSubscriber ?

+++ b/core/modules/menu_link/lib/Drupal/menu_link/MenuLinkStorageController.phpundefined
@@ -81,7 +81,9 @@ protected function attachLoad(&$menu_links, $load_revision = FALSE) {
-        $menu_links[$entity_id]->setRouteObject($route_objects[$route]);
+        if (isset($route_objects[$route])) {
+          $menu_links[$entity_id]->setRouteObject($route_objects[$route]);

Is that really needed ? Maybe would warrant a code comment ?

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
@@ -43,6 +43,7 @@
  *   menu_base_path = "taxonomy/term/%taxonomy_term",
+ *   entity_menu_base_path = "admin/structure/taxonomy/{bundle}",

Not for this issue, I guess, but those two entries using different placeholder syntaxes is a bit unfortunate.
Will that be tackled as a side effect of other, more general "mobe routes out of hook_menu()" tasks, or should we open a dedicated followup ?

yched’s picture

Oh, sorry - while we're in there, if we could just reintroduce in class names and route names the sanity that was lost in the previous form_ids when we removed the "field settings" fieldset from the "edit instance" form:

Proposal:
class: FieldEditForm -> FieldInstanceEditForm
route: field_ui.edit.$entity_type -> field_ui.instance_edit.$entity_type

class: FieldSettingsForm -> FieldEditForm
route: field_ui.settings.$entity_type -> field_ui.field_edit.$entity_type

If not, no biggie, can be a followup.

tim.plunkett’s picture

FileSize
3.5 KB
102.06 KB

I think the FieldInstanceEditForm name change makes things clearer, but as long as the URL is /field-settings, I think FieldSettingsForm is a clearer name. Follow-up to rename both the class and the URL seems fine to me.

Crell opened #1970360: Entities should define URI templates and standard links to address the various URI properties we have floating around.

I deleted the change to MenuLinkStorageController, since I think it may have just been an intermediate hack. If this fails, then we'll know what to make the comment :)

Yeah, I think everyone who needs a RouteSubscriber keeps copying the same code :)

Status: Needs review » Needs work

The last submitted patch, field-ui-1946404-43.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
898 bytes
102.94 KB

Welp, that would be why I had that check.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
862 bytes
102.94 KB

Seems that @yched's concerns were adressed, so I just fixed the comment from the last interdiff and call this done :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field-ui-1946404-46.patch, failed testing.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community
+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -194,6 +194,27 @@ class EntityType extends Plugin {
+  public $entity_menu_base_path;
...
+  public $entity_bundle_prefix;

the entity_ is redundant...

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -571,7 +564,7 @@ public function submitForm(array &$form, array &$form_state) {
         field_create_field($field);
-        field_create_instance($instance);
+        $new_instance = field_create_instance($instance);
 
         // Make sure the field is displayed in the 'default' view mode (using
         // default formatter and settings). It stays hidden for other view

@@ -613,7 +606,7 @@ public function submitForm(array &$form, array &$form_state) {
         try {
-          field_create_instance($instance);
+          $new_instance = field_create_instance($instance);
 
           // Make sure the field is displayed in the 'default' view mode (using

Shouldn't this be using the injected plugin.manager.entity

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldDeleteForm.phpundefined
@@ -0,0 +1,92 @@
+    form_load_include($form_state, 'inc', 'field_ui', 'field_ui.admin');
+
+    $field = $this->instance->getField();
+    $bundles = entity_get_bundles();

It's a shame that we're not using an injected services from the container here... but AFAIK at the moment we can't inject things in ConfirmForms

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -0,0 +1,280 @@
+ */
+class FieldInstanceEditForm implements FormInterface {
+
...
+    $bundles = entity_get_bundles();
...
+    $additions = module_invoke($field['module'], 'field_instance_settings_form', $field, $this->instance, $form_state);

Shouldn't this implement the ContainerInterface and have a create method so that the plugin.manager.entity and the module handler can be injected

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldSettingsForm.phpundefined
@@ -0,0 +1,161 @@
+/**
+ * Provides a form for the field settings edit page.
+ */
+class FieldSettingsForm implements FormInterface {
...
+    $additions = module_invoke($field['module'], 'field_settings_form', $field, $this->instance, $has_data);
...
+    $field = field_info_field($field_values['field_name']);

and ContainerInterface here too... to inject modulehandler, field.info

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldWidgetTypeForm.phpundefined
@@ -0,0 +1,109 @@
+ * Provides a form for the widget selection form.
+ */
+class FieldWidgetTypeForm implements FormInterface {
...
+    $field = field_info_field($field_name);

and ContainerInterface here too... to inject field.info

Also chatting to @fubhy in IRC we should have followups in place once #1906810: Require type hints for automatic entity upcasting and #1837388: Provide a ParamConverter than can upcast any entity. so that we can reduce the redundancy of urls like admin/structure/types/manage/article/fields/node.article.body (which used to be admin/structure/types/manage/article/fields/body )

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Crosspost with testbot :)

tim.plunkett’s picture

Thanks @alexpott, working on injecting stuff now.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
18.88 KB
105.13 KB

Okay, injected as much as I could, there is an issue with module_handler and $form_state, so just using \Drupal::moduleHandler for now.

tim.plunkett’s picture

FileSize
1.78 KB
105.14 KB

@amateescu pointed out a typo, thanks

amateescu’s picture

FileSize
1.4 KB
105.18 KB

I just found another problem..

yched’s picture

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldDeleteForm.phpundefined
@@ -70,7 +70,7 @@ public function submitForm(array &$form, array &$form_state) {
-      field_delete_instance($this->instance);
+      $this->instance->delete(TRUE);

Then, just delete() please (param is optional and defaults to TRUE).

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -37,7 +67,7 @@ public function buildForm(array $form, array &$form_state, FieldInstance $field_
-    $field = field_info_field($this->instance['field_name']);
+    $field = Field::fieldInfo()->getField($this->instance['field_name']);

Then should be injected as well ? ($container->get('field.info'))

See next comment.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldSettingsForm.phpundefined
@@ -142,14 +143,14 @@ public function submitForm(array &$form, array &$form_state) {
-    $field = field_info_field($field_values['field_name']);
+    $field = Field::fieldInfo()->getField($field_values['field_name']);

Then should be injected as well ? ($container->get('field.info'))

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldWidgetTypeForm.phpundefined
@@ -41,7 +71,7 @@ public function buildForm(array $form, array &$form_state, FieldInstance $field_
-    $field = field_info_field($field_name);
+    $field = Field::fieldInfo()->getField($field_name);

Same
See next comment.

yched’s picture

Actually, sorry:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -37,7 +67,7 @@ public function buildForm(array $form, array &$form_state, FieldInstance $field_
-    $field = field_info_field($this->instance['field_name']);
+    $field = Field::fieldInfo()->getField($this->instance['field_name']);
+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldWidgetTypeForm.phpundefined
@@ -41,7 +71,7 @@ public function buildForm(array $form, array &$form_state, FieldInstance $field_
-    $field = field_info_field($field_name);
+    $field = Field::fieldInfo()->getField($field_name);

Those two could now more simply be : $field = $field_instance->getField();

tim.plunkett’s picture

FileSize
105.71 KB
4.68 KB

Oh duh, forgot that field.info was a service.

Also, in 2/3 of those cases, we can just do $this->instance->getField().

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks ! Looks good to me.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field-ui-1946404-55.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
594 bytes
105.73 KB

Grrr.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field-ui-1946404-59.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.77 KB
105.09 KB

FieldInfo has ModuleHandler injected, so we cannot serialize that either.

This is #53 + the non-breaking fixes suggested in #54/#55

alexpott’s picture

Title: Convert forms in field_ui.admin.inc to the new form interface » Change notice: Convert forms in field_ui.admin.inc to the new form interface
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 6ed57d3 and pushed to 8.x.

During the commit I fixed the following comment as we no longer use an access callback...

       // used for all bundles of a given entity type; but depending on
       // administrator settings, each bundle has a different set of view
       // modes available for customisation. So we define menu items for all
-      // view modes, and use an access callback to determine which ones are
+      // view modes, and use a route requirement to determine which ones are
       // actually visible for a given bundle.
       $items["$default_path/display/default"] = array(
         'title' => t('Default'),
yched’s picture

Yay ! Thanks a ton @timplunkett !

yched’s picture

tim.plunkett’s picture

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: -Needs change record
swentel’s picture

Title: Change notice: Convert forms in field_ui.admin.inc to the new form interface » Convert forms in field_ui.admin.inc to the new form interface
Priority: Critical » Normal
Status: Needs review » Active

Looks good to me.

yched’s picture

Assigned: tim.plunkett » Unassigned
Status: Active » Fixed

Marking as fixed, then ?

Status: Fixed » Closed (fixed)

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

amateescu’s picture

This FormInterface conversion brought up a very nasty side effect on PHP 5.4: #2025451: Drupal\entity_reference\Tests\EntityReferenceAdminTest fails on PHP 5.4.9 and libxml 2.9.0. Reviews appreciated :)

yched’s picture