Problem/Motivation

In #1875992: Add EntityFormDisplay objects for entity forms we introduced the foundation for form modes but without any actual usage in core. This is the necessary followup :)

Form modes are the equivalent of view modes, applied to entity forms. They were whispered around the corners in various issues for quite a while (e.g. #1668292-17: Move simplified Profile module into core), greated with much enthusiasm at the end of the Field API session from DrupalCon Portland and, imo, mandatory if we want to fix things like #1911080: Replace menu node form additions with entity reference field in a truly generic way. They are also *very* useful for dead-simple creation of multistep entity forms (wizards in ctools speak).

Thanks to the recently added hidden widget and entity form displays, this functionality is achieved by removing code from core .. *gasp* :D In a nutshell, the patch removes half of field_ui.js, 30-40% of field_ui/FieldOverview.php, field_ui/Form/FieldWidgetTypeForm.php entirely, and refactors field_ui/DisplayOverview.php and a couple of other pieces to work generically for both widgets and formatters.

Who will benefit from this

  • Kill the checkbox workaround in Field API to show fields on user register form
  • Menu entity form embedded in nodes
  • Multi step form
  • Inline entity form
  • DS/FieldGroups - whoever, because we now have a unified UI (technically) for moving rows around, changing formatters etc.

User interface changes

  • a new 'Manage form display' tab appears in all Field UI screens, between 'Manage fields' and 'Manage display'
  • the drag-n-drop functionality from 'Manage fields' dissapears
  • Extra fields are not displayed anymore in the 'Manage fields' tab
  • widgets will be handled (like field formatters) in only one screen: 'Manage form display'. Previously they were scattered in three places:
    • they were originally selected in 'Manage fields' when you create a new field
    • their settings were handled in the instance edit form
    • they could be changed in a separate form, presented as a subtab of the instance edit form

Note that yoroy approves the patch, see #7548917, follow ups are also in there and listed below.

API changes

  • a new 'form_mode' config entity is introduced, handled exactly like the 'view_mode' one introduced in #1043198: Convert view modes to ConfigEntity
  • the hacky 'user_register_form' setting dissapears from all field instances

Followups/Related

Remaining tasks

  • implement the new settingsSummary() method for all existing widgets
  • create the equivalents of hook_field_formatter_settings_form_alter() and hook_field_formatter_settings_summary_alter() (right now they are still called in the base class, therefore executed for both formatters and widgets)
  • immediately update summary after switching widget
  • upgrade path

Another debatable/sensitive part is the $operation argument of entity_get_form(), which currently maps to entity form controllers. In the initial patch I've let them coexist in peace, giving us following options. Using 'step_1' as an example form controller/operation AND the same 'step_1' form mode:

  1. if both the form controller and form mode exist, the entity form will use the 'step_1' form controller and display only the fields configured in the 'step_1' form mode
  2. if the form controller exists but the form mode does not, the entity form will use the 'step_1' form controller and display the fields configured in the 'default' form mode
  3. if the form controller does not exist but the form mode does, the entity form will use the 'default' form controller and the fields configured in the 'step_1' form mode

Now, given that #1839516: Introduce entity operation providers has seen some movement lately and it's likely to get into D8, I think it makes sense to just rename the current entity 'operation/form controller api' to a 'form mode api'. This way, we will still have the three cases from above, but the concept will be much simpler: Entity forms use form modes in order to determine what fields to display in a specific form, and form modes can (but not necessarily) have a dedicated form controller for other custom processing needs.

Work happens

In a sandbox, see http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/2014...

CommentFileSizeAuthor
#81 entity-form-2014821-81.patch1.17 KBtim.plunkett
#76 2014821-form_modes-75-do-not-test.patch217 KBamateescu
#76 interdiff.txt1.16 KBamateescu
#74 2014821-form_modes-74.patch217.06 KBamateescu
#74 interdiff.txt9.41 KBamateescu
#68 2014821-form_modes-68.patch211.21 KBamateescu
#66 2014821-form_modes-66.patch211.64 KBamateescu
#64 2014821-form_modes-64.patch211.29 KBamateescu
#63 2014821-form_modes-63.patch210.47 KBamateescu
#59 2014821-form_modes-59.patch210.39 KBamateescu
#57 2014821-form_modes-57.patch209.53 KBamateescu
#53 2014821-form_modes-53.patch375.85 KBamateescu
#53 interdiff.txt2.32 KBamateescu
#54 2014821-form_modes-54.patch210.08 KBamateescu
#46 2014821-form_modes-45.patch208.12 KBamateescu
#46 interdiff.txt1.23 KBamateescu
#43 2014821-form_modes-43.patch207.95 KBamateescu
#41 2014821-form_modes-41.patch207.96 KBamateescu
#41 interdiff.txt584 bytesamateescu
#39 2014821-form_modes-39.patch207.96 KBamateescu
#39 interdiff.txt5.69 KBamateescu
#32 2014821-form_modes-32.patch203.63 KBamateescu
#32 interdiff.txt3.55 KBamateescu
#28 tab-naming-messed-up.png32.09 KByoroy
#27 tab-naming-messed-up.png18.09 KByoroy
#21 2014821-form_modes-21.patch210.38 KBamateescu
#17 2014821-form_modes-17.patch210.39 KBamateescu
#15 2014821-form_modes-15-do-not-test.patch172.08 KBamateescu
#14 2014821-form_modes-14-do-not-test.patch203.85 KBswentel
#10 2014821-form_modes-9-do-not-test.patch147 KBamateescu
#10 2014821-form_modes-9-PLUS-1982138-do-not-test.patch178.57 KBamateescu
#10 interdiff.txt11.42 KBamateescu
#10 user_field_before.png19.55 KBamateescu
#10 user_field_after.png22.98 KBamateescu
#10 wizard_step1.png10.16 KBamateescu
#10 wizard_step2.png15.23 KBamateescu
#3 2014821-form_modes-PLUS-1982138-do-not-test.patch175.19 KBamateescu
#1 2014821-form_modes-do-not-test.patch143.63 KBamateescu
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amateescu’s picture

Status: Active » Needs review
FileSize
143.63 KB

I haven't touched the tests at all because we first need to agree on the general direction of this patch, so let's use simplytest.me and/or manual testing for now :)

And please don't start with doxygen nitpicking yet...

amateescu’s picture

I forgot to say that this patch depends on #1982138: Clean out field_ui.admin.inc and has to be applied on top of the latest patch in that issue.

amateescu’s picture

Here's a combined patch for easier testing.

swentel’s picture

HELL YEAH - just did a quick spin, this is SO COOL, especially the user part. Are you working on this in a branch ?

Wim Leers’s picture

Intriguing :)

To me, it is not entirely clear what the exact use cases are for form modes. View modes have very clear use cases, and it's easy to think of examples. Can you provide some practical examples for what form modes brings us?
You mention #1911080: Replace menu node form additions with entity reference field, but to me it's not immediately obvious how this helps address that. You also mention multistep entity forms; I presume each step would then be a form mode of its own?
So I guess what I'm asking is: can you provide more obvious examples?

I think that would help other people understand this issue better too :) Thanks!

swentel’s picture

An obvious one: User register vs user edit - there's now a stupid workaround in field api when you add fields on the user account that adds a checkbox which says 'show on user register form'. Killing that hardcoded stuff is a must.

tim.plunkett’s picture

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -26,7 +26,7 @@
- *       "profile" = "Drupal\user\ProfileFormController",
+ *       "default" = "Drupal\user\ProfileFormController",

This is an unfortunate switch, and directly conflicts with the entire premise of #2006348: Remove default/fallback entity form operation.

The magic 'default' fallback is very confusing and has led to over-re-use of form controllers.

effulgentsia’s picture

should we keep displaying 'extra fields' in the 'Manage fields' tab?

I don't have an opinion at this time about that with respect to this issue, but once #1875974: Abstract 'component type' specific code out of EntityDisplay is done, we should be able to remove the concept of 'extra fields' entirely.

bojanz’s picture

Big use case: Inline Entity Form.
An entity being managed inline doesn't need the same fields as when it's being managed standalone.

amateescu’s picture

Are you working on this in a branch ?

Funny you ask that. Today I deleted my entire D8 dev environment because of a drush experiment gone bad :) Luckily enough, I had backups at home and also the current version of the patch on the desktop, so I just lost a bit of git history. I'll push everything to a branch in the Field API sandbox after I finish this post.


To me, it is not entirely clear what the exact use cases are for form modes. View modes have very clear use cases, and it's easy to think of examples. Can you provide some practical examples for what form modes brings us?

Example 1 - what core gets right now.

We replace this:
user_field_before.png

With this:
user_field_after.png

Example 2 - Multistep entity forms (wizards):
You are correct, each step will be a form mode.

wizard_step1.png
wizard_step2.png

Example 3 - #1911080: Replace menu node form additions with entity reference field:

That issue needs this patch and another one for Entity reference, an Inline/Embeddable Entity Form widget. If we get them both, we'll be able to expose a form mode that will be used by the IEF widget to only show a restricted set of (relevant) fields of the menu_link entity form, when embedded in the node form.


This is an unfortunate switch, and directly conflicts with the entire premise of #2006348: Remove default/fallback entity form operation.

While it may be unfortunate, we just need to try and find a compromise. The entire form mode concept relies on having a default form controller as a fallback. One thought that comes to mind is that after all the Entity/Field/whatever NG work is done, we could fallback to EntityFormController, which should be able to properly handle all entity fields by then.


should we keep displaying 'extra fields' in the 'Manage fields' tab?

I don't have an opinion at this time about that with respect to this issue, but once #1875974: Abstract 'component type' specific code out of EntityDisplay is done, we should be able to remove the concept of 'extra fields' entirely.

Unfortunately.. not really, I already tried :) #1954188: Revaluate the concept of 'extra fields'


Fixed a couple of things in this new patch and implemented settingsSummary() for the File and Image widgets. Also, I'll have to move the part about settingsSummary() from 'followups' to 'remaining tasks'. Without this completed, we won't be able to edit any widget settings :/

There are still a couple of quirks here and there, like settings not being taken from $form_state anymore, but I'm done for today. Eagerly waiting for some actual patch reviews :)

tim.plunkett’s picture

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.phpundefined
@@ -0,0 +1,60 @@
+class EntityDisplayModeBase extends ConfigEntityBase {

abstract

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.phpundefined
@@ -7,36 +7,16 @@
+class DisplayOverview extends DisplayOverviewBase implements ControllerInterface {

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.phpundefined
@@ -0,0 +1,732 @@
+abstract class DisplayOverviewBase extends OverviewBase {

Do we actually need another base class?

---

Because of the OverviewBase refactoring/renames, it's immensely hard to see if anything changed, and what it was.
The parts outside of that (deletions and search/replace) look straightforward.

amateescu’s picture

I'll leave the abstract thing for when there'll be more stuff to fix.

Right now, yes, we still need OverviewBase because FieldOverview uses it. It's mentioned in the OP in followup tasks that we could move FieldOverview to be a ListController, at which point we will be able to merge in/remove OverviewBase.

About the hard to review part:

DisplayOverview has turned into abstract DisplayOverviewBase which DisplayOverview (yeah, same name, confusing) and FormDisplayOverview extend. What changed inside it is:
- moved the generation of a field and an extra field outside of buildForm() to the buildFieldRow()/buildExtraFieldRow() helper methods
- moved everything that was getting formatter-specific info to helper methods (about 10 of them)

amateescu’s picture

Issue summary: View changes

Added an example issue where form modes were mentioned.

swentel’s picture

Today I deleted my entire D8 dev environment because of a drush experiment gone bad :)

Haha, good one :) Got the branch locally, I'll play around with it today a bit as well and - if applicable - commit obvious fixes.

swentel’s picture

Issue summary: View changes

Moved a task from 'followups' to 'remaining tasks' to be done in this patch.

swentel’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

More changes:

  • Lots of doc-block fixes
  • Add 'Field settings' link in dropbutton
  • Alter comment menu for the 'Form display' tab
  • Fix bug on 'Manage display' screen where format setting showed up
  • Removes 'Extra fields' on the 'Manage fields' page - kept the rest alphabetical, makes sense.
  • Summaries for text(area), test widget, date, entity reference, email, link, number, onOff, taxonomy, telephone: this now allows you to configure the widget settings

Note, if you want to play around with this beauty, it's easier to clone from http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/2014... (also added to issue summary)

swentel’s picture

Issue summary: View changes

Updated issue summary.

amateescu’s picture

swentel’s picture

Title: Oficially introduce form modes » Officially introduce form modes

Fixing title

amateescu’s picture

FileSize
210.39 KB

Implemented all the remaining tasks from the issue summary so I think we're ready for some final reviews!

amateescu’s picture

Issue summary: View changes

Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 2014821-form_modes-17.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review

Finally, after 4 tries the testbot had mercy on this patch :)

bojanz’s picture

Status: Needs review » Needs work

This already needs a reroll, core moves fast.

I'm not the perfect person to review the code of this, but the removal of the user register form checks already shows what a nice cleanup this is.
It is going to offer a whole world of possibilities for Inline Entity Form, since users will now be able to select which fields they want to edit when they're managing an entity inline (VS separately).

amateescu’s picture

Status: Needs work » Needs review
FileSize
210.38 KB

Here we go.

yched’s picture

I'm afraid you guys shouldn't wait for me to get this reviewed :-(. I'm going afk in like four days, and I'm buried deep down in #1969728: Implement Field API "field types" as TypedData Plugins. @swentel is hell bent on the patch though, and he's as qualified as me to get this in.

FWIW, I totally dig how this is reorganizing the code for the "manage" screens.
Thanks a ton @amateescu for the awesome job, really sorry I can't help.

swentel’s picture

This is just a first, quick review. Overall this patch is a beauty, the cleanup, but especially, the consistency in Field UI in code for form and display is like what we once started to work on in #1786198: Make consistent regions in code for fields UI overview screens, where we tried to introduce regions as well. This alone is going to make life less stressfull in contrib.

From the issue summary:

what sorting criteria to use in the 'Manage display' fields? right now the order is alphabetical

This sounds weird. On manage display, we sort by weight no ?

More will follow during the weekend when I'll be playing around with it on a local install and battle testing it, and also looking at the full code as it's sometimes hard to tell from dreditor.

A few things

+++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBaseInterface.phpundefined
@@ -81,4 +81,15 @@ public function removeComponent($name);
+  public function getRenderer($field_name);

This is fantastic, this is paving the way which we are almost exactly exploring in #1875974: Abstract 'component type' specific code out of EntityDisplay

+++ b/core/modules/field/field.moduleundefined
@@ -614,6 +615,41 @@ function field_bundle_settings($entity_type, $bundle, $settings = NULL) {
+function field_form_mode_settings($entity_type, $bundle) {
+  $cache = &drupal_static(__FUNCTION__, array());
+
+  if (!isset($cache[$entity_type][$bundle])) {
+    $bundle_settings = field_bundle_settings($entity_type, $bundle);
+    $settings = $bundle_settings['form_modes'];

The call to field_bundle_settings will be refactored in #1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable, so we can get rid of the variable, no biggie to keep this as is in this patch initially though.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.phpundefined
@@ -49,584 +29,188 @@ public static function create(ContainerInterface $container) {
+    $label_position = array_search('plugin', array_keys($field_row));
+    $field_row = array_slice($field_row, 0, $label_position, TRUE) + $label + array_slice($field_row, $label_position, count($field_row) - 1, TRUE);

This array_search and array_slice concerns me a little. It reminds me of doing freaky stuff with DS and/or Field group. I'm going to look at the code whether we can't come up with a saner solution.

+++ b/core/modules/node/lib/Drupal/node/Tests/NodeTypeInitialLanguageTest.phpundefined
@@ -75,10 +75,10 @@ function testNodeTypeInitialLanguageDefaults() {
+    // Tests if the language field can be rearranged on the manage form display tab.

Nitpick, 80 chars.

+++ b/core/modules/options/lib/Drupal/options/Tests/OptionsWidgetsTest.phpundefined
@@ -522,39 +522,37 @@ function testOnOffCheckbox() {
     // is stored and has the expected effect

Nitpick, missing '.' at the end.

swentel’s picture

I've also asked yoroy to have a look at this patch and briefly explained him what this patch does.

swentel’s picture

Issue summary: View changes

Updated remaining tasks.

amateescu’s picture

what sorting criteria to use in the 'Manage display' fields? right now the order is alphabetical

This sounds weird. On manage display, we sort by weight no ?

It sounds weird because it is weird :P It was just a silly mistake though, I meant to say the 'Manage fields' tab. Corrected in the issue summary too.

Thanks for starting the review, I'll wait for full thing before doing any updates :)

swentel’s picture

Right, I was almost assuming that :) I'm totally fine with alphabetical.

yoroy’s picture

FileSize
18.09 KB

Exciting patch! Applied succesfully etc.
But it's hard to review with the similarly named tabs, makes it hard to tell what you are looking at :-)

yoroy’s picture

FileSize
32.09 KB

ignore the above attachment

amateescu’s picture

I suspect you're talking about the node / comment tabs? That's a pre-existing problem in HEAD :( Try looking at the UI for users (admin/config/people/accounts/fields).

yoroy’s picture

Ok, playing around on http://d8:8888/admin/config/people/accounts/form-display

It's confusing to see the same items in different tabs. Lack of context makes it hard to understand what exactly you are editing here.

Drag & drop seems to be broken. I move 'Admin overlay' to the disabled section by using the 'Widget' select list. Moving it back up via drag and drop doesn't work. The 'drop' doesn't seem to register (the value select list isn't updated). I can leave the field in the enabled area but after saving it still shows up in the disabled area

Also, I have no idea what the 'Administrative overlay' field means to do. I see no difference between that one enabled or disabled. Tried the same with disabling timezone field, but that also still showed up on my user edit form.

So far, powerful but confusing, lots of dots to connect across multiple screens and page refreshes in order to grasp what is going on. I'll have to spend some more time with this, but that's it for now.

larowlan’s picture

amateescu’s picture

FileSize
3.55 KB
203.63 KB

Rerolled after #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets and fixed nitpicks from #23 as well as the annoying 'No fields are present yet.' message which appeared all the time on the 'Manage fields' tab since we removed the JS that was hiding it. Added a @todo to bring it back in #1963340: Change field UI so that adding a field is a separate task.

Re #30:
Thanks for taking a look at this!

It's confusing to see the same items in different tabs. Lack of context makes it hard to understand what exactly you are editing here.

The same thing happens in 'Manage display'... you see the same set of fields for different view modes.

Drag & drop seems to be broken.

Drag & drop works just fine, you probably need to clear the browser cache :)

Also, I have no idea what the 'Administrative overlay' field means to do. I see no difference between that one enabled or disabled. Tried the same with disabling timezone field, but that also still showed up on my user edit form.

The 'Administrative overlay' field was there before this patch, but I agree it's confusing and we should open a new issue to find a better label for this extra field.

The point about hiding extra fields (like 'Administrative overlay' and 'Timezone') is a very good one though, because those fields are usually added in hook_form_alter()s and are not controlled by EntityFormController. So I guess we need to either change the way they're added to entity forms, or implement a form alter at the latest stage possible so we can hide them.

yched’s picture

Extra fields not being hidden :
That's my remark in the second half of #1875992-95: Add EntityFormDisplay objects for entity forms : any code that currently adds an "extra-field" in a form (be it in a form controller method or in a form_alter()) should check the visibility assigned in the $form_display first. That's how it works on the entity_view() side.
OR, we could treat things a bit differently on the form side, and have an additional step that runs after the alter() hooks and automatically adds #access = FALSE to all the extra fields that are supposed to be hidden.
But currently, yes, you can configure an extra field to be hidden in a form, but it will still stay visible. In current HEAD, nothing lets you do that through the UI, this is what this patch does - but the wrong behavior is already in HEAD.

amateescu’s picture

Right, so I ended up with the same conclusion after forgetting that you already warned about this in the initial issue :) Anyway, from those two options, I think I prefer the one that does a bit more hand-holding and hide them automatically in an "post alter" step.

But since this issue is currently followed by 42 people, I'd definitely like to hear more opinions :)

yoroy’s picture

is ycheds link the issue to learn about extra fields? i'm not sure what they are and why they are 'extra'.

amateescu’s picture

@yoroy, nope, extra fields is a concept that we already have in D7. The best documentation that I can find for them is the one from the doxygen of hook_field_extra_fields().

But for the purposes of testing this patch, you don't really need to worry about extra fields. If you want to see something that works in the current version of the patch, just hide a 'regular' field, like the user picture. Or add a new field and hide that one :)

yched’s picture

from those two options, I think I prefer the one that does a bit more hand-holding and hide them automatically in an "post alter" step

The approach of "let each extra field check its own visibility itself" was adopted on the display side because this saves some CPU (not generate possibly costly render data if it's going to be hidden).
Form rendering is less performance critical, and a $form entry being sometimes present / sometimes absent might have nastier effects on the validate / submit flow, so going with "always add, automatically set #access = FALSE on the stuff that should be hidden" on forms specifically might be best indeed.

swentel’s picture

I think I prefer the one that does a bit more hand-holding and hide them automatically in an "post alter" step

+1 one on that

Sorry for the promised update, got completely distracted, will be for the next 3 days as well.

One other thing that I'm not sure of (I should test I know) - can you 'disable' a form mode from the UI from being configured, just like you can with view modes ? So in case 'register' is not used, it falls back on the default one. In case that's ok, please ignore this question completely.

amateescu’s picture

FileSize
5.69 KB
207.96 KB

Ok, so I had to fix two bugs in HEAD:

  1. related to @swentel's question above - the 'Custom display settings' element was not appearing if we only had one custom view/form mode
  2. EntityDisplayBase::getComponents() was doing a strict (===) check on a property coming from config, which, on write, converts everything to strings...

And, finally, moved the code that was assigning weights and added the code that hides extra fields in a form process callback.

Status: Needs review » Needs work

The last submitted patch, 2014821-form_modes-39.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
584 bytes
207.96 KB

Stupid mistake..

Status: Needs review » Needs work

The last submitted patch, 2014821-form_modes-41.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
207.95 KB

Chasing HEAD.. can't seem to catch it, or a working testbot for that matter :)

Status: Needs review » Needs work

The last submitted patch, 2014821-form_modes-43.patch, failed testing.

yoroy’s picture

I tested the patch in #39 today and spent an hour with amateesco discussing this with amateescu in IRC.

At first glance this looked really worrisome! Another two tabs on Field UI really makes a mess and hits the limits of what that UI can contain right out of the box.

Amateescu had two important points that took away many of the ux concerns (boiling down to overloading an already overcrowded UI):

- #731724: Convert comment settings into a field to make them work with CMI and non-node entities which would remove the comment fields and comments display tabs from the Field UI for content types. This makes room for the additional Form tabs this patch introduces
- #1963340: Change field UI so that adding a field is a separate task would create new specific point of entry for an important task in Field UI. A general issue with Field UI is that most of the magic (all those powerful fields) is hidden in dropdowns somewhere at the bottom of a complex ui

While I of course can't force it, I consider those two issues must-haves in order for this to go in. The form modes in this patch add more complexitiy to an already crowded and hard to grasp interface. The other two move some of that complexity to seperate screens, which balances things out nicely.

To be clear: I really think we should try to land this in core, it's powerful framework stuff with a lot of potential.

So for the patch itself we mostly should look into some better labels for the tabs and probably introduce a line of help text to explain that "over there you arrange the display of the content, over here you arrange the display of the form that content is created with."

Any other changes will quickly hit 'redesign Field UI' territory. Which is not the scope of this issue but that one.

Go go go! :-)

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
208.12 KB

Should be better now.

plach’s picture

Status: Needs review » Needs work

Just a (probably) dumb question from an outsider perspective, but wouldn't having a first-level "Fields" tab with 4 second-level subtabs help de-clutter these screens?

plach’s picture

Status: Needs work » Needs review
amateescu’s picture

Thanks @yoroy for the write-up :)

There are some points there that I want to address:
1) This patch only adds one tab to the Field UI interface, 'Manage form display'.
Only in the case of nodes two tabs are added, one for nodes and one for comments. This is where #731724: Convert comment settings into a field to make them work with CMI and non-node entities helps and gets rid of comment-related tabs from the content type edit UI.

2) The patch from #1963340: Change field UI so that adding a field is a separate task is indeed a must-have, but we can't really consider it as a dependency for this one, as it will be *a lot* easier to do after this one lands, because we refactor/simplify most of the code for 'Manage fields' here.

amateescu’s picture

@plach, then we would need some third-level tabs to display form and view modes :)

yoroy’s picture

Good clarifications by amateescu. Yes, I didn't mean to suggest technically dependant, just that for an acceptable UX we should get those other two issues done as well.

plach’s picture

Ah, right, the screenshots didn't show the second-level. I still need to study more :D

amateescu’s picture

FileSize
2.32 KB
375.85 KB

Added some help text for the node and user 'Manage form display' tabs. I don't have any ideas for renaming the tab labels so I'm waiting for suggestions on that :)

amateescu’s picture

FileSize
210.08 KB

Forgot to merge HEAD.

Dries’s picture

I'm supportive of this patch. It looks cool. I haven't looked at it in detail yet (no code review).

cweagans’s picture

I am SO EXCITED for this patch. I'm hoping to circle back to this for a code review at some point this week.

amateescu’s picture

FileSize
209.53 KB

Thanks, guys! Let the reroll games begin :)

Status: Needs review » Needs work

The last submitted patch, 2014821-form_modes-57.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
210.39 KB

Fixed a new test and a merge conflict.

Status: Needs review » Needs work
Issue tags: -Usability, -Killer Developer Features, -Field API, -sprint

The last submitted patch, 2014821-form_modes-59.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

#59: 2014821-form_modes-59.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +Killer Developer Features, +Field API, +sprint

The last submitted patch, 2014821-form_modes-59.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
210.47 KB

Rerolled.

amateescu’s picture

Issue summary: View changes

Fixed wrong tab name.

swentel’s picture

Issue summary: View changes

Updated issue summary.

swentel’s picture

Issue summary: View changes

Updated issue summary.

amateescu’s picture

FileSize
211.29 KB

@swentel did a live review and we found some small issues, corrected in ef59fea and 4a16ce8.

He also cleaned up the issue summary and added a small followup issue (which needs to fix a current bug in HEAD, not introduced by this patch).

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Let's do this!

amateescu’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2014821-form_modes-66.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
211.21 KB

Rerolled because #2029471: Regression: Content types do not respect all permissions defined by Field UI was committed. The fail above passes locally and doesn't look related.. let's see if this one has better luck.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

tsphethean’s picture

Adding follow up to deprecate functions in field.info.inc #2029835: Deprecate procedural functions which have been moved to the Field class

tsphethean’s picture

Issue summary: View changes

Updated issue summary.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/field_ui/field_ui.jsundefined
@@ -7,108 +7,6 @@
-    // If possible, keep the same widget selected when changing field type.

+++ b/core/modules/field_ui/field_ui.services.ymlundefined
@@ -8,3 +8,7 @@ services:
+  access_check.field_ui.forn_mode:

forn_mode?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.phpundefined
@@ -0,0 +1,760 @@
+abstract class DisplayOverviewBase extends OverviewBase {

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -19,79 +17,35 @@
+        //'message' => t('No fields are present yet.'),

This should implement an interface and all the abstract protected functions should be moved there.

We need to implement a testWidgetUI based on testFormatterUI

tim.plunkett’s picture

Protected methods cannot be on an interface.

alexpott’s picture

@timplunkett good point... and if we need to make anything public this is an API addition so something we can do later....

amateescu’s picture

Status: Needs work » Needs review
FileSize
9.41 KB
217.06 KB

Here we go, less typos and more tests :) Thanks for the review!

Status: Needs review » Needs work

The last submitted patch, 2014821-form_modes-74.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
217 KB

@swentel found some debug() leftovers..

amateescu’s picture

Ugh, mixup here.. I just asked for a retest of #74 because #76 doesn't add anything new :)

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, RTBC if green, apart from a little one:

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageDisplayTest.phpundefined
@@ -96,6 +96,73 @@ function testFormatterUI() {
+    // Assert that hook_field_formatter_settings_summary_alter() is called.
+    $this->assertText('field_test_field_widget_settings_summary_alter');

Alex, you ok with changing 'hook_field_formatter_settings_summary_alter' to 'hook_field_widget_settings_summary_alter' on commit if nothing else comes up ?
Patch in #76 is the right one.

alexpott’s picture

Title: Officially introduce form modes » Introduce form modes UI and their configuration entity
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed 4586ea1 and pushed to 8.x. Thanks!

effulgentsia’s picture

Title: Introduce form modes UI and their configuration entity » Change notice: Introduce form modes UI and their configuration entity

Yay!

tim.plunkett’s picture

Status: Active » Needs review
FileSize
1.17 KB

Same as the case below it.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Oops, sorry about that :/ And thanks, Tim!

alexpott’s picture

Status: Reviewed & tested by the community » Active

Committed 4b22498 and pushed to 8.x. Thanks!

Thanks @timplunkett

nod_’s picture

Issue tags: +JavaScript

This changes JS file it gets the JavaScript tag.

andypost’s picture

Status: Active » Reviewed & tested by the community
Issue tags: -JavaScript, -Usability, -sprint
+++ b/core/modules/image/lib/Drupal/image/Plugin/field/widget/ImageWidget.phpundefined
@@ -53,6 +53,30 @@ public function settingsForm(array $form, array &$form_state) {
+    $image_style_setting = $this->getSetting('preview_image_style');
+    if (isset($image_styles[$image_style_setting])) {

Added follow-up #2030207: Do not load all field instances when replacing image style

andypost’s picture

Status: Reviewed & tested by the community » Active
Issue tags: +JavaScript, +Usability, +sprint

revert

andypost’s picture

Issue summary: View changes

Updated issue summary.

andrewmacpherson’s picture

yched’s picture

yched’s picture

Issue summary: View changes

added follow-up link to #2029857

andrewmacpherson’s picture

yched’s picture

Note : opened #2049485: Remove traces of the 'user_register_form' field setting to remove 'user_register_form' from the existing config entries and config schemas

amateescu’s picture

Assigned: amateescu » Unassigned
catch’s picture

catch’s picture

Issue summary: View changes

adding follow-up issue #2041225

xjm’s picture

Tagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release.

The patch for this issue was committed on June 27, 2013, meaning that the change record for this (very delectable) issue has been outstanding for more than seven months.

geerlingguy’s picture

I'm working on a change record... will update when complete.

geerlingguy’s picture

Title: Change notice: Introduce form modes UI and their configuration entity » Introduce form modes UI and their configuration entity
Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

Here's my initial stab at the change record: https://drupal.org/node/2186197 - please see how you like it, edit it to improve it... since there weren't any real API changes, more just additions/subtractions, I didn't have any Drupal 7 vs. 8 code examples.

Status: Fixed » Closed (fixed)

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

rlmumford’s picture

For those that are interesting in this feature, something similar can be achieved on Drupal 7 using the Flexiform module. https://drupal.org/project/flexiform

giorgio79’s picture

Just gave form modes a spin, and created a form mode for a content type. I don't see where can we access the alternative form mode other than the default? I would have expected an option to be presented when clicking on "Add Content" to select "Add xy content with xy form mode"? Raised it as an issue at #2530086: How can we access the new form modes?

yched’s picture

@giorgio79 : Answered over there