#1852966: Rework entity display settings around EntityDisplay config entity (change notice) introduced EntityDisplay objects on the entity_view() side. An EntityDisplay (config entity) object centrally stores the list of components (fields, "extra fields", contrib additions like field_groups...) displayed for a given entity type & bundle in a given view mode, along with the corresponding display options (weight, formatter, formatter settings...).
This removed the various scattered locations where those settings were stored in D7:
- in $instance['display'] for each individual field,
- in the 'field_bundle_settings_[entity_type]_[bundle]' variable ('display' entry) for 'extra fields',
- in other places for contrib additions that are not one of the two above (field_group ...)

This issue is about applying a similar pattern on the "form" side.
It is a pre-requisite for #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets, since it will provide a location to store "assignement of widgets + settings to base (non-Field-API) fields" configuration.
(The existing EntityDisplay objects already provide such a location for formatters on base fields)

Patch summary

  • Introduces EntityFormDisplay config entities.
    Most of the associated methods and business logic is shared with EntityDisplay objects through a common abstract EntityDisplayBase class.
  • Just like EntityDisplay objects are associated to an entity bundle and a view mode, EntityFormDisplay objects, at the API level, are associated to an entity bundle and a "form mode".
    "Form modes" are typically intended to differenciate between "user edit" and "user register" forms, or possibly "add" and "edit" forms.
    The patch only adds support for "form modes" at the EntityFormDisplay storage level, but does not have core entities use them for now. Only the 'default' form mode is actually used - thus, no visible functionnal change on this regard.
  • Modifies EntityFormController so that the correct EntityFormDisplay is fetched, lets modules alter it in one pass (instead of one alter invocation per field previously) and placed in $form_state for the rest of the form building callstack to use.
  • Widget plugin objects are instanciated from (and persisted in) the EntityFormDisplay object rather than each $instance object. This part is 1:1 with what is done for formatters and EntityDisplay objects.
  • Modifies Field UI to store widget configuration in EntityFormDisplay rather than $instance['widget'].
  • Removes $instance['widget'] and the rest of the 'field_bundle_settings_[entity_type]_[bundle]' variable,
    provides the corresponding upgrade path, and related test.
  • Provides the CMI file for article nodes (entity.form_display.node.article.default.yml) in standard.profile's default config
    (Page node type only contains the "body" field, which is created programmatically)

Remaining tasks

None (see followups below)

User interface changes

None.

API changes

Quite similar to the API changes introduced by EntityDisplays:

  • New entity_get_form_display() API function to access EntityFormDisplay objects:
     entity_get_form_display('node', 'article', 'default')
       ->setComponent('body', array(
         'type' => 'text_textarea_with_summary',
         'weight' => 1,
       ))
       ->setComponent('field_image', array(
         'type' => 'hidden',
       ))
       ->save();
    
  • The weights configured in the EntityFormDisplay are automatically assigned, thus widgets don't need to take care of that themselves, and don't receive the $weight in their constructor.
  • $instance['widget'] is gone, widget properties need to be explicitly assigned by manipulating the corresponding EntityFormDisplay object.
  • hook_field_widget_properties_alter() is removed in favor of the new hook_entity_form_display_alter().
  • The field_info_max_weight() function is removed, replaced by EntityFormDisplay::getHighestWeight().
  • Followups

    • Support hiding "extra fields":UI + default setting in hook_field_extra_field() + make all the code that adds the actual elements in $form check the configured visibility from the EntityFormDisplay first
    • Decide how core can actually leverage form modes (what they are exactly, how they are exposed / added, UI...): #2014821: Introduce form modes UI and their configuration entity
    • Introduce config schema for EntityFormDisplay (and EntityDisplays that still lack them), move over the existing config schemas for $instance['widget'] and widget settings for the various widget types.
    CommentFileSizeAuthor
    #144 entity_form_display-1875992-144.patch214.53 KBamateescu
    #136 entity_form_display-1875992-136.patch215.4 KBamateescu
    #136 interdiff.txt1021 bytesamateescu
    #134 entity_form_display-1875992-134.patch214.4 KBamateescu
    #129 entity_form_display-1875992-129.patch214.41 KBamateescu
    #129 interdiff.txt663 bytesamateescu
    #123 EFD_xhprof.png14.45 KBamateescu
    #122 entity_form_display-1875992-122.patch214.41 KByched
    #120 entity_form_display-1875992-120.patch214.03 KByched
    #120 interdiff.txt1.97 KByched
    #117 entity_form_display-1875992-117.patch213.39 KByched
    #114 entity_form_display-1875992-114.patch213.33 KByched
    #112 entity_form_display-1875992-112.patch213.33 KByched
    #106 1875992-106.patch213.45 KBamateescu
    #106 interdiff.txt1.71 KBamateescu
    #102 1875992-102.patch214.47 KBamateescu
    #101 1875992-101.patch214.48 KBamateescu
    #101 interdiff.txt830 bytesamateescu
    #99 1875992-99.patch214.46 KBamateescu
    #99 interdiff.txt631 bytesamateescu
    #97 1875992-97.patch214.47 KBamateescu
    #97 interdiff.txt10.38 KBamateescu
    #94 1875992-94.patch211.78 KBamateescu
    #92 1875992-91_AFTER_field-ui-1946404-53.patch208.62 KBamateescu
    #92 1875992-91_PLUS_field-ui-1946404-53.patch293.75 KBamateescu
    #91 1875992-91.patch207.11 KBamateescu
    #91 interdiff.txt16.69 KBamateescu
    #86 1875992-86.patch207.73 KBamateescu
    #86 interdiff.txt9.83 KBamateescu
    #84 1875992-84.patch207.84 KBamateescu
    #84 interdiff.txt1.66 KBamateescu
    #81 1875992-81.patch207.44 KBamateescu
    #81 interdiff.txt26.69 KBamateescu
    #79 1875992-79.patch197.59 KBamateescu
    #78 1875992-78.patch198.61 KBamateescu
    #78 interdiff.txt1.45 KBamateescu
    #76 1875992-76.patch198.74 KBamateescu
    #73 1875992-73.patch198.62 KBamateescu
    #73 interdiff.txt2.93 KBamateescu
    #70 1875992-70.patch198.78 KBamateescu
    #70 interdiff.txt11.4 KBamateescu
    #67 1875992-67.patch192.13 KBamateescu
    #67 interdiff.txt17.77 KBamateescu
    #65 1875992-65.patch179.79 KBamateescu
    #65 interdiff.txt8.01 KBamateescu
    #63 1875992-63.patch175.25 KBamateescu
    #63 interdiff.txt6.71 KBamateescu
    #60 1875992-60.patch170.03 KBamateescu
    #60 interdiff_1.txt2.67 KBamateescu
    #60 interdiff_2.txt6.59 KBamateescu
    #53 1875992-53.patch173.42 KBamateescu
    #53 interdiff.txt3.52 KBamateescu
    #48 1875992-48.patch172.49 KBamateescu
    #48 interdiff.txt30.24 KBamateescu
    #39 1875992-39.patch172.56 KBamateescu
    #36 1875992-36.patch172.41 KBamateescu
    #36 interdiff.txt40.07 KBamateescu
    #35 1875992-35.patch153.78 KBamateescu
    #35 interdiff.txt607 bytesamateescu
    #31 1875992-31.patch153.82 KBswentel
    #31 interdiff.txt1.53 KBswentel
    #22 1875992-22.patch153.45 KBamateescu
    #22 interdiff.txt3.01 KBamateescu
    #20 1875992-20.patch151.41 KBamateescu
    #20 interdiff.txt1.36 KBamateescu
    #17 1875992-17.patch151.16 KBswentel
    #17 interdiff.txt4.42 KBswentel
    #14 1875992-14.patch148.25 KBswentel
    #14 interdiff.txt1.42 KBswentel
    #12 1875992-12.patch146.82 KBswentel
    #12 interdiff.txt1.2 KBswentel
    #9 1875992-9.patch146.21 KBswentel
    #9 interdiff.txt569 bytesswentel
    #6 1875992-6.patch146.22 KBswentel
    #6 interdiff.txt1.53 KBswentel
    #4 1875992.patch145.41 KBamateescu
    Support from Acquia helps fund testing for Drupal Acquia logo

    Comments

    yched’s picture

    Status: Active » Postponed
    amateescu’s picture

    Assigned: Unassigned » amateescu
    Status: Postponed » Active

    I was working on this for a few days without knowing that an issue already exists. Since I'm almost done with the patch, I guess we can leave the 'component type' refactoring for later :/

    effulgentsia’s picture

    +1 for this. I added this as a related issue to #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets in that the two combined get us to where configuring widgets on nonconfigurable fields will be possible. Thanks for the work on this, @amateescu. Looking forward to seeing the patch!

    amateescu’s picture

    Status: Active » Needs review
    FileSize
    145.41 KB

    This is what I have so far.

    I see some failures locally in the options widgets and options field ui tests, probably becaue #1751234: Convert option widgets to Plugin system is not in yet. Also, while working on this patch I found this little gem #1950326: Entity displays are not renamed/deleted when a bundle is renamed/deleted. for which swentel was kind enough to provide a patch quickly, but I still had to disable a test here until that's committed.

    Should I mention that for this shiny new configuration entity to be 'useful', we need #1465774: Provide a 'Hidden' field widget?

    Edit: Pushed this patch to http://drupalcode.org/sandbox/yched/1736366.git/shortlog/refs/heads/1875...

    Status: Needs review » Needs work

    The last submitted patch, 1875992.patch, failed testing.

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    1.53 KB
    146.22 KB

    Awesome indeed! Adding new patch which should have lots more test passes, will push when this comes back.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-6.patch, failed testing.

    swentel’s picture

    Great, pushed eb225c7

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    569 bytes
    146.21 KB

    This should be even better.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-9.patch, failed testing.

    swentel’s picture

    Pushed 2ae74d1

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    1.2 KB
    146.82 KB

    More test fixes, pushed following:

    b313e5c
    1db995b

    Looking into the 3 biggest test failures now

    Status: Needs review » Needs work

    The last submitted patch, 1875992-12.patch, failed testing.

    swentel’s picture

    FileSize
    1.42 KB
    148.25 KB

    Fixed and pushed user registration and multiform. RDF chokes on rdf_modules_installed() - ah rdf and entities, fun ..

    swentel’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 1875992-14.patch, failed testing.

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    4.42 KB
    151.16 KB

    Pushed couple of more fixes, done for today.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-17.patch, failed testing.

    amateescu’s picture

    The failures from ContactPersonalTest are caused by this issue: #1856556: Convert user contact form into a contact category/bundle.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    1.36 KB
    151.41 KB

    Oh, we have the same problem in EntityDisplay. This should do it for now..

    Status: Needs review » Needs work

    The last submitted patch, 1875992-20.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    3.01 KB
    153.45 KB

    Fixed the rest of the failures, except for the options stuff. I had to borrow the fix from #1950326: Entity displays are not renamed/deleted when a bundle is renamed/deleted. though.. that one should probably go in sooner so we'll just need a reroll here.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-22.patch, failed testing.

    amateescu’s picture

    I tried to combine this patch with #1751234: Convert option widgets to Plugin system and I still get those three failures locally, so the problem is somewhere in here. Yay for one less dependency..

    swentel’s picture

    This is weird - if I changed following line in testOptionsAllowedValuesBoolean() (line 222)

        $field = field_info_field($this->field_name);
    

    to

        $field = field_read_field($this->field_name);
    

    then OptionsFieldUITest will pass - so it sounds like a caching issue, but looking at the patch I don't understand why immediately, digging some further.

    yched’s picture

    Current test works because that feild_info_field() call is the first one on the tester side after caches were cleared by field_create_field() in createOptionsField() -> field info is rebuilt there, and includes the result of the UI manipulations on the tested side.

    The patch adds code in createOptionsField() that fetches field_info data and thus rebuilds the cache --> the feild_info_field() call re-reads that cached data that dates from before the UI actions :-)

    There should be a field_info_cache_clear() after the drupalPost() / drupalGet() stuff in OptionsFieldUITest()

    swentel’s picture

    Right - duh - completely overlooked the call to field_info_field_types(). Now on to the last failure in testOnOffCheckbox() in OptionsWidgetsTest - and it's not a clear cache thing afaics ;)

    swentel’s picture

    Ok, I can reproduce this manually, for some reason, the widget is reset / or not saved ok and jumps back to radioboxes/checkboxes instead of on_off, so the problems is either in field_ui_field_edit_form_validate or field_ui_field_edit_form_submit - or at least, that's were it is reset.

    swentel’s picture

    Ok, so it's a global problem in field_ui admin definitely, bug can be reproduce manually when using the datetime field, it will always fallback to the default widget.

    swentel’s picture

    Also, let's remember we need to cleanup the form display configuration when we delete a field/instance, my guess is that we don't do this with the entity display either looking at the code.

    swentel’s picture

    Status: Needs work » Needs review
    FileSize
    1.53 KB
    153.82 KB

    This should do it - the widget type got lost. I'll push everything, merge with core when this turns green.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-31.patch, failed testing.

    amateescu’s picture

    Title: Add EntityDisplay objects for entity forms » Add EntityFormDisplay objects for entity forms

    I just realized that I completely skipped the handling of extra fields in the initial patch, so 'needs work' is the correct status for now.

    amateescu’s picture

    The result of me being lazy is that I opened #1954188: Revaluate the concept of 'extra fields' instead :)

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    607 bytes
    153.78 KB

    In the meantime, let's get a green patch here.

    amateescu’s picture

    FileSize
    40.07 KB
    172.41 KB

    Ok, since extra fields aren't going away anytime soon, I took another approach here: turned EntityDisplay into a base class because both EntityDisplay and EntityFormDisplay needed 90% the same code.

    If the testbot agrees, I think this is now ready for final reviews! I'd like to ask for special attention for field_behaviors_widget() and EntityFormDisplay::getWidget() to be sure that I haven't messed up something there :)

    effulgentsia’s picture

    #36: 1875992-36.patch queued for re-testing.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-36.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    172.56 KB
    andypost’s picture

    #36: 1875992-36.patch queued for re-testing.

    Patch needs re-roll
    Fatal error: Uncaught exception 'Drupal\Core\Config\StorageException' with message 'Missing configuration file: system.logging' in /home/s2d6a416a5a4da40/www/core/lib/Drupal/Core/Config/InstallStorage.php:54 Stack trace: #0 /home/s2d6a416a5a4da40/www/core/lib/Drupal/Core/Config/FileStorage.php(73): Drupal\Core\Config\InstallStorage->getFilePath('system.logging') #1 /home/s2d6a416a5a4da40/www/core/lib/Drupal/Core/Config/FileStorage.php(82): Drupal\Core\Config\FileStorage->exists('system.logging') #2 /home/s2d6a416a5a4da40/www/core/lib/Drupal/Core/Config/Config.php(410): Drupal\Core\Config\FileStorage->read('system.logging') #3 /home/s2d6a416a5a4da40/www/core/lib/Drupal/Core/Config/Config.php(204): Drupal\Core\Config\Config->load() #4 /home/s2d6a416a5a4da40/www/core/includes/errors.inc(156): Drupal\Core\Config\Config->get('error_level') #5 /home/s2d6a416a5a4da40/www/core/includes/bootstrap.inc(2271): error_displayable() #6 [internal function]: _drupal_exception_handler(Object(Drupal\Core\Config\StorageException)) #7 {main} in /home/s2d6a416a5a4da40/www/core/lib/Drupal/Core/Config/InstallStorage.php on line 54

    amateescu’s picture

    You missed the reroll from #39?

    andypost’s picture

    @amateescu no, thats what symplytest.me shows when I test patch #39

    amateescu’s picture

    #39: 1875992-39.patch queued for re-testing.

    effulgentsia’s picture

    Re #40: it appears to be a simplytest.me error. I get the same error when just trying to get simplytest.me run HEAD (on the initial screen, you can remove the patch, before clicking Launch). Works fine locally though and via testbot.

    yched’s picture

    Starting to review - @amateescu : could you push the reroll / conflict resolution from #39 to the sandbox ?

    amateescu’s picture

    Sure, I pushed everything. It seems that I was working in a different branch locally so I had to delete and recreate the initial branch created by @swentel.

    yched’s picture

    Initial review, I only looked at the surroundings for now, didn't get to the "meaty" parts :-)

    1. Rename of $entity_display->viewMode to $entity_display->displayMode.
      Ok, this is moved to a more generic EntityDisplayBase superclass used for both EntityDisplay and EntityFormDisplay, so viewMode doesn't fit anymore.
      Maybe just "mode" then ? No biggie, and no strong opinion myself, just throwing the idea out there.
    2. +++ b/core/modules/datetime/lib/Drupal/datetime/Tests/DateTimeFieldTest.phpundefined
      @@ -58,15 +58,19 @@ function setUp() {
      +    $this->form_display_options = array(
      +      'type' => 'datetime_default',
      +    );
      +    entity_get_form_display($this->instance['entity_type'], $this->instance['bundle'], 'default')
      +      ->setComponent($this->field['field_name'], $this->form_display_options)
      +      ->save();
      

      $this->form_display_options does not seem really needed as a separate $this property ?

    3. +++ b/core/modules/field/lib/Drupal/field/FieldInstance.phpundefined
      @@ -37,37 +37,6 @@ public function __construct(array $definition) {
      -  public function getWidget() {
      

      Yay !!

    4. +++ b/core/modules/field/lib/Drupal/field/Tests/FieldInfoTest.phpundefined
      @@ -170,14 +166,16 @@ function testInstancePrepare() {
      +    $entity_form_display = entity_get_form_display('test_entity', 'test_bundle')
      +      ->setComponent($field_definition['field_name'])
      +      ->save();
      ...
      -    $data['widget']['settings'] = 'unavailable_widget';
      -    $data['widget']['settings'] = array();
      ...
           // Check that the default widget is used and expected settings are in place.
      -    $widget = $instance->getWidget();
      +    $widget = entity_get_form_display('test_entity', 'test_bundle')->getWidget($instance_definition['field_name']);
           $this->assertIdentical($widget->getPluginId(), $field_type['default_widget'], 'Unavailable widget replaced with default widget.');
           $widget_type = $widget->getDefinition();
           $this->assertIdentical($widget->getSettings(), $widget_type['settings'] , 'All expected widget settings are present.');
      

      There shouldn't be any tests about widgets in this method now.
      The part about the part about 'unavailable_widget' and "check that default widget is used with default settings instead" should get out of this test, and be dealt with in the tests for EntityFormDisplay.

    5. +++ b/core/modules/field/field.api.phpundefined
      @@ -777,38 +776,6 @@ function hook_field_widget_WIDGET_TYPE_form_alter(&$element, &$form_state, $cont
      -function hook_field_widget_properties_alter(array &$widget_properties, array $context) {
      

      Cool !
      hook_field_widget_properties_ENTITY_TYPE_alter() should go too, though (I know, confusing, they aren't next to each other in the file...)

      Also, I don't see any hook_entity_form_display_alter() to replace them though ?
      On the EntityDisplay side, hook_field_display_alter() got removed because we have hook_entity_display_alter(), that is invoked at render time by EntityRenderController.

      (this also means that implementations that are removed in field_test, and the corresponding tests that are removed in Drupal\field_ui\Tests\AlterTest, will need to move to tests on this new hook_entity_form_display_alter() hook ?)

    6. Related to the absence of hook_entity_form_display_alter() : the patch doesn't change anything on the FormController side ? It seems this should be where the logic about a "display object", that holds the definition of the components of a form, would live ?
      Like the EntityDisplay objects are handled by the EntityRenderController ?
      Then again, as I said above, I didn't review the central parts of the patch yet.
    7. +++ b/core/modules/field/field.api.phpundefined
      @@ -1873,21 +1840,24 @@ function hook_field_storage_pre_update(\Drupal\Core\Entity\EntityInterface $enti
      -function hook_field_info_max_weight($entity_type, $bundle, $context) {
      +function hook_field_info_max_weight($entity_type, $bundle, $context, $context_mode) {
      

      Why is the extra param needed ?

    8. +++ b/core/modules/hal/lib/Drupal/hal/Tests/NormalizerTestBase.phpundefined
      @@ -30,7 +30,7 @@
      -  public static $modules = array('entity_test', 'entity_reference', 'field', 'field_sql_storage', 'hal', 'language', 'rest', 'serialization', 'system', 'text', 'user');
      +  public static $modules = array('entity_test', 'entity_reference', 'field', 'field_sql_storage', 'hal', 'language', 'rest', 'serialization', 'system', 'text', 'user', 'entity');
      

      Nitpick : would make more sense to add 'entity' at the beginning of the list ?
      Same for EntityUnitTestBase.php, FieldAccessTest.php

    9. +++ b/core/modules/node/lib/Drupal/node/Plugin/views/wizard/Node.phpundefined
      @@ -278,9 +278,10 @@ protected function build_filters(&$form, &$form_state) {
           foreach ($bundles as $bundle) {
             foreach (field_info_instances($this->entity_type, $bundle) as $instance) {
      +        $widget = entity_get_form_display($instance['entity_type'], $instance['bundle'], 'default')->getComponent($instance['field_name']);
               // We define "tag-like" taxonomy fields as ones that use the
               // "Autocomplete term widget (tagging)" widget.
      -        if ($instance['widget']['type'] == 'taxonomy_autocomplete') {
      +        if ($widget['type'] == 'taxonomy_autocomplete') {
                 $tag_fields[] = $instance['field_name'];
      

      Woah. This tries to find what are the freetagging taxo fields existing on the entity type, by looking for fields that use the taxo_autocomplete widget.
      That won't play too nice with form modes ?

    10. +++ b/core/modules/options/lib/Drupal/options/Tests/OptionsFieldUITest.phpundefined
      @@ -258,6 +262,15 @@ protected function createOptionsField($type) {
      +    // Grab the default widget for the field type.
      +    $field_type_definition = field_info_field_types($field['type']);
      +    $plugin_id = $field_type_definition['default_widget'];
      +    entity_get_form_display('node', $this->type, 'default')
      +      ->setComponent($this->field_name, array(
      +        'type' => $plugin_id,
      +      ))
      +      ->save();
      

      "Grab the default widget for the field type" should not be needed, entity_get_form_display()->setComponent($this->field_name)->save() will just use the default widget.

    11. Changes in link.module / options.module :
      Hmm. We should probably push for #1796316: Convert Link/URL widgets / formatters to plugin system and #1751234: Convert option widgets to Plugin system to get in first.
      Then again - are those changes really needed if we revert the changes in LegacyWidget, and keep that class populating $instance['widget'] just like the legacy code expects ?
    12. +++ b/core/modules/text/lib/Drupal/text/Tests/TextFieldTest.phpundefined
      @@ -62,14 +62,16 @@ function testTextFieldValidation() {
      +//    entity_get_display('test_entity', 'test_bundle', 'default')
      +//      ->setComponent($this->field['field_name'])
      +//      ->save();
      

      Why is this commented out ?

    13. +++ b/core/modules/entity/lib/Drupal/entity/Plugin/Core/Entity/EntityDisplay.phpundefined
      @@ -7,13 +7,14 @@
      +use Drupal;
      

      Hm, really ? :-)

    amateescu’s picture

    FileSize
    30.24 KB
    172.49 KB

    Thanks for starting the review :)

    1. Maybe just "mode" then ? No biggie, and no strong opinion myself, just throwing the idea out there.

      I just thought 'displayMode' would be a bit more meaningful than 'mode' but don't really care either, so changed to 'mode'. If we decide to go back, we just need to revert c1ca463d.

    2. $this->form_display_options does not seem really needed as a separate $this property ?

      That's just to be consistent with how the rest of the test was written.

    3. Yay !!

      Yay indeed :)

    4. There shouldn't be any tests about widgets in this method now.

      Removed those parts and created EntityFormDisplayTest.

    5. hook_field_widget_properties_ENTITY_TYPE_alter() should go too, though (I know, confusing, they aren't next to each other in the file...)

      Also, I don't see any hook_entity_form_display_alter() to replace them though ?

      Right, that has been a @todo on my desktop since I started the initial patch and I guess it got lost among other such todos for various other patches, sorry for that :/ I'll have to leave it for the next reroll because I ran out of time today.

    6. Will take care of that along with the new hook from above.
    7. Why is the extra param needed ?

      Before, the $context param could receive either 'form' or a view mode (pretty confusing, eh?). Not it can be either 'form' or 'display' and the new paramater will have to be the form or view mode.

    8. Nitpick : would make more sense to add 'entity' at the beginning of the list ?
      Same for EntityUnitTestBase.php, FieldAccessTest.php

      Fixed all three.

    9. Woah. This tries to find what are the freetagging taxo fields existing on the entity type, by looking for fields that use the taxo_autocomplete widget.
      That won't play too nice with form modes ?

      That whole guessing part is a bit shady, so yeah, it won't play nice with form modes, but guessing on the 'default' form mode won't make it any worse I guess..

    10. "Grab the default widget for the field type" should not be needed,

      Fixed.

    11. Changes in link.module / options.module :
      Hmm. We should probably push for #1796316: Convert Link/URL widgets / formatters to plugin system and #1751234: Convert option widgets to Plugin system to get in first.
      Then again - are those changes really needed if we revert the changes in LegacyWidget, and keep that class populating $instance['widget'] just like the legacy code expects ?

      I removed those changes and tests for the link field passed, but the ones for option widgets failed. Apllied #1751234: Convert option widgets to Plugin system locally and it fixes them, so I'll leave them failing in here until those conversions are committed. I tried to push for them to get in at least twice in IRC.. no luck so far :(

    12. Why is this commented out ?

      Removed it along with the creation of an entity_display from above which wasn't needed as well.

    13. Right, we shouldn't 'use' Drupal in namespaced classes but prefix it instead. Fixed.

    Another thing that's needed in the next reroll is a test that EntityFormDisplay::getWidget() returns the hidden widget for a field that's not found in the config entity.

    andypost’s picture

    As I already mention on IRC the more suitable name would be buildMode because EntityDisplay operates by components having some renderable array ($build) as result

    Status: Needs review » Needs work

    The last submitted patch, 1875992-48.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review

    For any other reviewers that might wait until the patch is green and didn't read the all the comments, those 6 failures from #48 are expected(!) until #1751234: Convert option widgets to Plugin system is committed.

    yched’s picture

    - DateTimeFieldTest.php: $this->form_display_options does not seem really needed as a separate $this property ?
    - That's just to be consistent with how the rest of the test was written.

    If I'm not mistaken, the test uses $this->some_property for values it will need to access in other methods. That's not the case here, so convoluted--.

    Extra param in hook_field_info_max_weight(): ah right, makes sense.

    EntityDisplay::getFormatter() / EntityFormDisplay::getWidget() :
    #1875974: Abstract 'component type' specific code out of EntityDisplay will need to abstract that to getRenderer() or something anyway. Wondering if it makes sense / makes things easier to do it here as a method on the base class ?

    EntityFormDisplayTest.php :
    +   * Tests the behavior of a field component within an EntityDisplay object.
    

    Copy / paste - should be EntityFormDisplay ?

    LegacyWidget.php:
    +      // hook_field_widget_form() implementations read widget properties directly
    +      // from $instance. Put the actual properties we use here, which might have
    +      // been altered by hook_field_widget_property().
    

    Pedantry++ : the second sentence is not needed anymore :-). The reason we override $instance['widget'] is not that the correct values might be different from what's already there, it's that there's noting in $instance['widget'] anymore now.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    3.52 KB
    173.42 KB

    Fixed the small stuff from #52 in the new patch below.

    About abstracting EntityDisplay::getFormatter() / EntityFormDisplay::getWidget() into a getRenderer() method on the base class, I actually tried that the first time I created the base class, but I ended up with something like this and I didn't find it really useful..

      public function getRenderer($field_name, array $plugin_options = NULL) {
        if (isset($this->plugins[$field_name])) {
          return $this->plugins[$field_name];
        }
    
        // Instantiate the plugin object using the provided options.
        if ($plugin_options) {
          $plugin = $this->pluginManager->getInstance($plugin_options);
        }
        else {
          $plugin = NULL;
        }
    
        // Persist the formatter object.
        $this->plugins[$field_name] = $plugin;
        return $plugin;
      }
    

    Now, about this:

    Related to the absence of hook_entity_form_display_alter() : the patch doesn't change anything on the FormController side ? It seems this should be where the logic about a "display object", that holds the definition of the components of a form, would live ?
    Like the EntityDisplay objects are handled by the EntityRenderController ?

    That's correct, I didn't change anything on the FormController side, just on the field api side (field_attach_form() and friends) because I wanted to be as least intrusive as possible. Doing it in the FormController would basically mean doing something like #1374116: Move bundle CRUD API out of Field API for this family of field_attach_* functions/hooks and will increase the patch size considerably. Wouldn't it be wiser to save that for a followup?

    I also remembered why hook_entity_form_display_alter() was only a @todo on my desktop, and that's because I thought it needlessly duplicates hook_entity_load(). I'm happy to talk more about it when you have some time though :)

    Edit: stopped the testbot manually, we already know that we'll get the same 6 failures.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-53.patch, failed testing.

    yched’s picture

    Sorry for being slow on this one. The Field / CMI patch eats the limited core time I have right now :-(.

    Re #53:

    getRenderer(): ok, sounds prematurate. Will be something for #1875974: Abstract 'component type' specific code out of EntityDisplay

    As to the question of leaving FormController aside:

    Hm. The main architectural selling point for the EntityDisplay patch was "let display settings (the EntityDisplay object) be pulled by the RenderController, altered as a whole, and injected into each function / method responsible for rendering the various components".

    That was done in two steps (issue summaries for the two issues linked blow are pretty detailed on that) :

    The Initial patch, #1852966: Rework entity display settings around EntityDisplay config entity, did :
    a) pull the EntityDisplay in the RenderController,
    b) alter it as a whole,
    c) thus, get rid of hook_field_display_alter() and hook_field_extra_fields_display_alter()
    d) inject the EntityDisplay it into field_attach_[prepare_view|view]()
    e) leave the rest of the render callstack (EntityRenderControllerInterface::buildContent(), hook_entity_view() and friends...) untouched, i.e pulling display settings themselves for the components they deal with, e.g with field_extra_fields_get_display().

    Then #1875970: Pass EntityDisplay objects to the whole entity_view() callstack did the rest of the job:
    f) reworked the rest of the callstack to receive the EntityDisplay as well,
    g) removed field_extra_fields_get_display()
    h) Have RenderController take care of assigning weights (does so in viewMultiple()), and remove the #pre_render callback that was previously doing it. Code adding sub-render-arrays for some component use the EntityDisplay they receive to determine if the component is visible, and don't bother with the weight.
    i) Item h) takes care of assigning weights for fields too, so formatters don't need to, thus stop passing $weight to formatter plugins.

    I think we agree that we should target a similar end result for EntityFormDisplays and the form rendering flow. How best to split that in terms of initial patch and followups can be discussed, but:
    - Current patch does the equivalent of c) (remove hook_field_widget_properties_alter()), but we can't do that without a) and b) (an alter step on the EntityFormDisplay object before it gets used by the various parts that add components to the form)
    - We'll need reviewers and committers here too. IMO it would be easier for the various people that worked on or reviewed the EntityDisplay patches (and would help committers feel more confident about it too) if the EntityFormDisplays work mirrored the steps that were taken over there.

    yched’s picture

    Status: Needs review » Needs work

    Review for the parts I didn't look so far :

    1. EntityDisplay.php, EntityFormDisplay.php, EntityDisplayBase.php:
      Misses empty line between last method and closing curly brace.
    2. EntityDisplayBase.php
      // Clear the persisted formatter, if any.
      Leftover mention of "formatter"
    3. FieldInfo::prepareInstance() :
      -    if (field_behaviors_widget('default value', $instance) == FIELD_BEHAVIOR_DEFAULT && !isset($instance['default_value'])) {
      +    if (!isset($instance['default_value']) && field_behaviors_widget('default_value', $instance) == FIELD_BEHAVIOR_DEFAULT) {
      

      Looks like a needless change ?

    4. field_info_cache_clear() :
      +  entity_get_controller('entity_form_display')->resetCache();
      

      Why is this needed ? It seems we didn't need anything similar for entity_displays so far ?
      Besides, ConfigStorageController::resetCache() is a no-op ;-)

    5. field_update_8002() misses a part where we cleanup the 'form' entries in the old variables (needs to rework a bit with how the part about displays does its own cleanup).
      We can't completely remove the variables themselves, because there's still the configuration of view modes in there.
    6. Given that field_bundle_settings() doesn't contain any info about extra fields now, it seems there is some logic that needs to be changed or diteched in FieldInfo::prepareExtraFields(). It even might be that the whole method can go away.
    7. FieldOverview::buildForm()
      -          '#title' => t($widget_types[$instance['widget']['type']]['label']),
      +          '#title' => t($widget_types[$widget_configuration['type']]['label']),

      Ouch. Care to open an issue to remove that t() ? (there's another one for the $field['type'] 3 lines above)

    8. - field_ui_inactive_message()
      Gee, the current code in HEAD is complete nonsense, inactive fields have nothing to do with widgets.
      The message should be "%field (@field_name) field requires the %field_type field_type provided by %field_type_module module".
      We need the name of the field type and its module in here, not the name of the widget / widget module.
      Then, no need to fetch entity_get_form_display() here.
    amateescu’s picture

    Re #55: I'm perfectly ok with doing all that in this patch, just wanted a confirmation before increasing the patch size to >200K :)

    The only thing that I think should be discussed is "b) alter it as a whole". Are you implying that this does more than could be done in a hook_entity_(form_)display_load()? You might say that entity_get_(form_)display() doesn't always return an existing entity, but since it goes through the extra step of doing a entity_create() for you, it can also fire the load hook just after that..

    yched’s picture

    hook_entity_(form_)display_load() can't do the job. This would alter a Display:
    - only when it is loaded from config, i.e it does not run on objects created on the fly at runtime (and I don't think we want to blur the lines but making it run at create() time too)
    - every time it's loaded from config - i.e the changes made in there will most likely end up being saved back in the config, since in 99% cases, updating a config entity happens after loading the current from config. More generally, see #1270608-110: File-based configuration API for why alter-on-load is a bad pattern for stuff that gets saved back somewhere (config or db).

    hook_entity_(form_)display_alter() is something different. It's altering a run time object before it gets used, and in a context where it doesn't get saved back afterwards. It's much more akin to hook_form_alter() or hook_query_alter(), if you will.
    Remove the module that does the alter, and the effect is just gone.

    swentel’s picture

    Issue tags: +Field API

    tagging

    amateescu’s picture

    FileSize
    6.59 KB
    2.67 KB
    170.03 KB

    I planned some time yesterday night to finish the work needed here but I barely made it through a very painful re-roll due to all the Field API patches that landed recently :/ So I'll post an updated patch for now that only includes HEAD chasing (interdiff_1) and the review points from #56 (interdiff_2).

    Given that field_bundle_settings() doesn't contain any info about extra fields now, it seems there is some logic that needs to be changed or diteched in FieldInfo::prepareExtraFields(). It even might be that the whole method can go away.

    Can we leave that for a followup? I'll open new issues for points 7 and 8 too.

    amateescu’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 1875992-60.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    6.71 KB
    175.25 KB

    Hope I got`em all.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-63.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    8.01 KB
    179.79 KB

    Fixed and tested the upgrade path.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-65.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    17.77 KB
    192.13 KB

    And, finally, here's the last missing part from @yched's review: the EntityFormController handling.

    This new patch does a), b), h) and i) from #55. I'm not sure we can do d) and f) as well.

    Passing the $display object through the whole render callstack works for entity_view because it all happens in the same request, but forms have to be able to use the *altered* $form_display object through multiple requests (validate, submit) so we have to persist the object in $form_state. Now, if we already must have and *keep* it there, I see no reason to also add it to the entire field_attach_form* family.

    swentel’s picture

    Man, I love this patch, went through scanning for doc problems, could only see this for now:

    +++ b/core/includes/entity.incundefined
    @@ -730,12 +730,110 @@ function entity_get_render_display(EntityInterface $entity, $view_mode) {
    + *   The entity for which the form is being rendered.
    + * @param string $form_mode
    + *  The form mode being rendered.
    

    Indentation of 'The form mode being rendered.' is missing a space.

    The hook_field_widget_properties_alter() test is gone.

    We should probably also remove the widget if a field instance is deleted, the same applies to entity displays, which is handled over at #1952652: Instance components are not removed from entity display when the field/instance is deleted. - could be follow up or merged in the other patch depending who gets in first.

    Is it possible we miss the entity form display configuration for page in the standard install ? Or don't we really need it ?

    Status: Needs review » Needs work

    The last submitted patch, 1875992-67.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    11.4 KB
    198.78 KB

    Indentation of 'The form mode being rendered.' is missing a space.

    Fixed.

    The hook_field_widget_properties_alter() test is gone.

    Brought back in EntityFormTest.

    We should probably also remove the widget if a field instance is deleted, the same applies to entity displays, which is handled over at #1952652: Instance components are not removed from entity display when the field/instance is deleted. - could be follow up or merged in the other patch depending who gets in first.

    Or we could do it here and re-roll when the other one gets in :/ Which I did.

    Is it possible we miss the entity form display configuration for page in the standard install ? Or don't we really need it ?

    We don't need it because it contains only the body field.

    Also fixed the merging of EntityFormController::form() with EntityFormControllerNG::form(), which I shouldn't have tried in the first place.

    Oh, and I have nothing but kind words for whoever let that Contact entity type not have a bundle!

    yched’s picture

    - Is it possible we miss the entity form display configuration for page in the standard install ? Or don't we really need it ?
    - We don't need it because it contains only the body field

    Not clear to me why that makes it not needed ?

    Status: Needs review » Needs work

    The last submitted patch, 1875992-70.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    2.93 KB
    198.62 KB

    Fixed the last two failing tests. First one was my bad, I hid the test field for all entity_test forms, when just testing the size alter was enough to test that hook_entity_form_dispay_alter() works.

    Not clear to me why that makes it not needed ?

    Because the body field is still added programatically, and #1970206: Move fields and instances to standard profile config didn't add a config file for page either.

    swentel’s picture

    Because the body field is still added programatically, and #1970206: Move fields and instances to standard profile config didn't add a config file for page either.

    Good point, we can always revisit that for both in follow up in case we feel we really need it.

    yched’s picture

    #73: ah, sure, makes sense. Thanks for the explanation.

    amateescu’s picture

    FileSize
    198.74 KB

    Rerolled on top of latest HEAD.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-76.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    1.45 KB
    198.61 KB

    Fixed merge conflicts with #1967106: FieldInstance::getField() method.

    amateescu’s picture

    FileSize
    197.59 KB

    Fixed another (new) merge conflict with #1955678: Remove formatter and widgets legacy plugin.

    yched’s picture

    Thanks for the amazing work, @amateescu !

    Ok, new review, 1st part :-)
    As before, I'm starting at the periphery, I didn't go into the EntityFormController part yet, will try to finish in the next couple days.

    1. testEntityFormDisplayUpgrade.php
      +    $displays = array(
      +      'default' => config('entity.form_display.node.article.default')->get(),
      +    );

      Test structure is copied from the equivalent testEntityDisplayUpgrade, but keying by 'form mode' is not needed here, and obfuscates the code flow.
      Even if there are multiple 'form modes' at some point in D8, we'll never have to test that on a D7 upgrade.
      (also applies to the rest of the test)

    2. testEntityFormDisplayUpgrade.php
      +    // Check that the display key in the instance data was removed.
      +    $body_instance = field_info_instance('node', 'body', 'article');
      +    $this->assertTrue(!isset($body_instance['form']));
      

      We want to test $body_instance['widget'], not $body_instance['form'] ?

    3. DateTimeDatelistWidget::__construct() / DateTimeDatelistWidget::__construct()
      Since you modify the phpdoc, let's switch it over to {@inheritdoc}
    4. CustomBlockFieldTest::testBlockFields():
      $form_display_options and $display_options are not useful as standalone vars, let's inline them in the setComponent() calls where they are used.
      (I know, $display_options was there before the patch :-), but let's cleanup rather than unify on a needless pattern)
    5. FieldOverview::__construct()
      -    $this->view_mode = 'form
      +    $this->view_mode = 'default';

      Something doesn't look fully right here. Maybe this property should be renamed 'mode' in Overviewbase ?

    6. FieldOverview::buildForm
      +    $entity_form_display = entity_get_form_display($this->entity_type, $this->bundle, $this->view_mode);
      (...)
      +      $widget_configuration = entity_get_form_display($instance['entity_type'], $instance['bundle'])->getComponent($instance['field_name']);

      Redundant ? The second line should do a $entity_form_display->getComponent() ?

    7. field_ui_existing_field_options():
      +          $widget = entity_get_form_display($instance['entity_type'], $instance['bundle'], 'default')->getComponent($instance['field_name']);
      

      This data is only used within an if() branch below --> move that entity_get_form_display() call within the if() as well ?

    8. field_ui_widget_type_form_submit()
      +  // Update the stored instance with the incoming values.
      +  $entity_form_display = entity_get_form_display($entity_type, $bundle)
      +    ->setComponent($field_name, array(
      +      'type' => $form_values['widget_type'],
      +    ));

      Comment is not relevant now :-)

    9. field_ui_field_edit_form()
        $form['#field'] = $field;
        $form['#instance'] = $instance;
      +  $form['#entity_form_display'] = $entity_form_display = entity_get_form_display($entity_type, $bundle);
      

      Nitpick: let's split that in two lines, assigning $entity_form_display in the same lines where $field and $instance are defined a couple lines above.

    10. field_ui_default_value_widget()
      -  $element += $instance->getWidget()->form($entity, LANGUAGE_NOT_SPECIFIED, $items, $element, $form_state);
      +  $element += entity_get_form_display($instance['entity_type'], $instance['bundle'])->getWidget($instance['field_name'])->form($entity, LANGUAGE_NOT_SPECIFIED, $items, $element, $form_state);

      The form display is already available in $form['#entity_form_display'] ?

    11. field_ui_field_edit_form_submit()
      +  // Handle widget settings.
      +  $entity_form_display->setComponent($instance['field_name'], $form_state['values']['instance']['widget'])->save();

      Looks like this will overwrite the weight currently assigned ?
      Should be: $options = $display->getComponent(): $options['settings'] = $form_state['values']['instance']['widget']['settings']; $display->setComponent($options).
      Then, the $form['instance']['widget']['type'] element in field_ui_field_edit_form() is not needed anymore, can go away (and the $widget_configuration var in there too).

    12. ImageStyleStorageController::replaceImageStyle()
             // Loop through all fields searching for image fields.
             foreach ($instances as $instance) {
      -        if ($instance['widget']['module'] == 'image') {
      +        $entity_form_display = entity_get_form_display($instance['entity_type'], $instance['bundle'], 'default');
      +        $widget_definition = $entity_form_display->getWidget($instance['field_name'])->getDefinition();
      +        if ($widget_definition['module'] == 'image') {

      Existing code is weird already, but gets even weirder (and costly) now that the widget is somewhere else.
      This should be done by checking the field type, not the widget type.
      if ($instance->getField()->type == 'image') { should do the job here.
      Then, the code needs to manipulate the FormDisplay a couple lines below, but because it rightly needs to change some widget settings, that's alright. And we only need to load the display if we are on an image field, less loading.

    13. field_update_8002()
      -     if (!isset($data['display'])) {
      +     if (!isset($data['widget']) && !isset($data['display'])) {
             continue;
           }

      Fine, but then the rest of the code needs separate if (isset($data['display']) and if (isset($data['widget']) checks ?

    14. Keeping up with HEAD:
      core/modules/field/tests/modules/field_test_config/config/field.instance.test_entity.test_bundle.field_test_import.yml
      core/modules/field/tests/modules/field_test_config/staging/field.instance.test_entity.test_bundle.field_test_import_staging.yml
      Those need their 'widget' section removed.
      Same for the field.instance.* files shipped as default config for the "standard" install profile in core/profiles/standard/config/field.instance.*
    15. Also, #1953404: Add config schema to field and instance config entities landed, /core/modules/field/config/schema/field.schema.yml now describes the schema for the $instance['widget'] entry.
      I'd suggest we don't care about that yet, leave it untouched for now, and let the followups in http://drupal.org/project/issues/search/drupal?issue_tags=Field%20config... go their way. Then, once EntityFormDisplays are in, we'll need an issue to add config schema for entity.(form_)display.*.yml, and move the schema for 'widgets' in there.
    16. Code calling entity_get_form_display() is a mix between mixes passing "default' as a 3rd argument, or passing no argument and relying on 'default' being the default value (and the only form mode currently).
      This translates the more general fact that it's not fully clear to which extent the patch actually introduces support for 'form modes', or just paves the way for them to be supported later.
      I'd strongly suggest going in separate steps, focus this patch on EntityFormDisplays, and save actual support for "multiple form modes" for a later patch.
      Then, no 3rd argument at all when calling entity_get_form_display(), and maybe even no 3rd argument in the function signature for now ?
      In that same logic, entity_get_render_form_display() is not really needed just now. Let's see when we *really* want to introduce form displays, in a different issue IMO.
    amateescu’s picture

    FileSize
    26.69 KB
    207.44 KB

    Fixed points 1-14 from the review above and agreed that we should deal with 15 in those specific followups.

    Now.. 16 is a thorny issue for me :/ The whole reason for which I got involved here is form modes.. I really think that they are a byproduct of *this* conversion, and the followup patch needs to expose a hook and the UI for them and settle grounds with the current entity form 'operations'. It's my fault that I made that third argument optional in the first place just to make my life easier in tests, and then I've been very inconsistent with its usage :(

    If we decide to 'hide' this functionality, the only result is that the next patch will have a harder time being accepted, or maybe even rejected..

    swentel’s picture

    RE:16 I don't see many problems with the 3rd argument actually. Consistent usage is something different, that's a fair point.

    Having this one already there, means the API is there, and we have form modes, we just don't use them in core - yet. I think we'll have bigger battles over the UI, and maybe even get rejected on that, but then at least contrib can start working on solutions, and we don't have to wait for a smaller patch that just adds that 3rd argument.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-81.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    1.66 KB
    207.84 KB

    Actually saving the entity that you're working with always helps..

    yched’s picture

    Thanks for the quick updates, @amateescu !

    Still mulling over "form modes" for now, meanwhile, rest of the review :

    1. field_ui_field_edit_form_submit()
        $options['settings'] = NestedArray::mergeDeep($options['settings'], $form_state['values']['instance']['widget']['settings']);
      

      Not sure why the mergeDeep is needed ? We don't do that on the EntityDisplay side ?

    2. field_attach_form()
      +  // Enable BC if necessary.
      +  $entity = $entity->getBCEntity();

      That's already in #1974474: Make sure within field_attach_*() that we are working with a BC entity - really needed here ?

    3. WidgetPluginManager::getInstance()
      $configuration += array(
            'instance' => $instance,
            'form_mode' => $options['form_mode'],
          );

      The 'form_mode' entry is not used anywhere. FormatterPluginManager sets $configuration['view_mode'] because FormatterFactory takes it and passes it as the $view_mode to FormatterBase::__construct().
      But for now Widget plugins don't have a similar parameter, and WidgetFactory does nothing of $configuration['form_mode'].

    4. EntityFormDisplay::__construct()
      +    $this->pluginManager = \Drupal::service('plugin.manager.field.widget');
      

      This means EntityFormDisplay need special care on serialize(), because serializing the plugin manager means serializing a lot of things, including the CacheDiscoveryDecorator and its cached array of all widget plugin definitions.
      + we don't want to serialize the widget plugin objects that are persisted in the EntityFormDisplay either.
      Pending what happens with #1977206: Default serialization of ConfigEntities, we probably need to add the same \Serializable / serialize() / unserialize() stuff that is currently in Field and FieldInstance.

    5. EntityFormDisplay::getWidget()
          // Check if this field actually exists.
          if ($instance = field_info_instance($this->targetEntityType, $field_name, $this->bundle)) {

      There's no such check in EntityDisplay::getFormatter(), why is this needed here ? Maybe add a comment about that ?

    6. EntityFormDisplay::getWidget()
            // Try to get the widget configuration from the stored display properties.
            if (!$configuration = $this->getComponent($field_name)) {
              // We don't store hidden fields but we need to use the 'Hidden' widget
              // for them.
              $configuration = array(
                'type' => 'hidden',
                'settings' => array(),
                'weight' => 0,
              );
            }

      So yeah, this is area is a bit confusing.
      EntityDisplays use removeComponent() for "set to hidden". For Field API fields, this results in nothing being saved in the CMI file.
      Then, when reading an existing EntityDisplay object, "absent" is treated as "hidden", and that works because this how we want it to be for fields that were created after the EntityDisplay was last written.

      For EntityFormDisplays, removeComponent() is never used by existing code, instead it's setComponent('type' => 'hidden'), and this gets written in the CMI file.
      removeComponent() is only mentioned in the phpdoc for entity_get_form_display(), probably as a result of copy/paste from entity_get_display().
      So it's not clear whether removeComponent() is actually supported for EntityFormDisplays and what it means.

      At any rate,
      - The comment is not up to date (we do store hidden widgets)
      - 'weight' => 0 is not needed anymore here, widgets don't care about their weights now

    7. EntityFormController[NG]::form()
          if (!empty($info['fieldable'])) {
            // Get the entity_form_display object for this form mode (operation).
            $form_display = entity_get_render_form_display($entity, $this->operation);

      $info['fieldable'] should only control the call to field_attach_form(), but EntityFormDisplay objects, like EntityDisplay objects, are relevant whether the entity type is fieldable or not ?

      Might be related to the next item:

    8. field_attach_extract_form_values()
        if (isset($form_state['form_display'])) {
          $form_display = $form_state['form_display'];
        }
        else {
          // @todo Remove this hack when http://drupal.org/node/1856556 is fixed.
          $form_display = $form_state['form_display'] = entity_get_form_display($entity->entityType(), $entity->entityType());
        }

      Odd. Why is #1856556: Convert user contact form into a contact category/bundle causing $form_state['form_display'] to not be set ?
      And why is field_attach_extract_form_values() precisely the right place to work around this ?

    amateescu’s picture

    FileSize
    9.83 KB
    207.73 KB
    1. Right, it's not :)
    2. I forgot to clean that up when I gave up unifying EntityFormController::form() with EntityFormControllerNG::form(). Removed.
    3. Just overzealous I think :/ Removed.
    4. Copied here for now, we can remove them when that patch lands.
    5. See below.
    6. This was too much thinking ahead on my part, when 'Manage fields' would have the same capabilities as 'Manage display' (wrt to the hidden region), and when we'll actually be able to *not* store fields that use the hidden widget. The purpose of this and the code from 5. was to check that the field instance actually exists so it can return the hidden widget.

      Since we're not there yet, I reverted those parts of the code, and now it's 100% identical to EntityDisplay::getFormatter(). Would you like to merge them now, knowing that we'll have to split them again in the next patch?

      And removeComponent() for EntityFormDisplay *is* used :) See FieldInstance::delete().

    7. Moved the code outside the 'fieldable' check and this fixed the need for that hack from 8. me-- :)
    8. Fixed by 7.
    yched’s picture

    now [EntityFormDisplay::getWidget()] is 100% identical to EntityDisplay::getFormatter(). Would you like to merge them now, knowing that we'll have to split them again in the next patch?

    No strong feeling here, but I'd tend to go for "yes", if they are 100% similar, and since I think those methods will eventually need to be abstracted to a getRenderer() or getPlugin() or something in #1875974: Abstract 'component type' specific code out of EntityDisplay, to account for the various component types and allow contrib component types (DS, field groups...).

    And removeComponent() for EntityFormDisplay *is* used :) See FieldInstance::delete().

    Right - only for internal cleanup :-), but that still leaves the API a bit unclear.
    OK, we might want to adjust that in a followup.

    amateescu’s picture

    Ok.. so they're not 100% similar because FormatterPluginManager::getInstance() expects a 'view_mode' key in $options, while the widget plugin manager does not, so EntityDisplay::getFormatter() has an extra line compared to EntityFormDisplay::getWidget()..

    tim.plunkett’s picture

    The changes to field_ui.admin.inc clash heavily with #1946404: Convert forms in field_ui.admin.inc to the new form interface, which is essentially done (@swentel can't RTBC anymore, said he'd ask @yched).
    I'd be happy to help reroll this...

    yched’s picture

    So, regarding form modes :

    Took me some time to finally realize that the patch does not really add them, since :
    - EntityFormController::form() asks for entity_get_render_form_display($entity, $this->operation)
    - entity_get_render_form_display($entity, $form_mode) ditches the $form_mode param and always uses 'default'
    - field_update_8002() migrates all $instance['widget'] in entity.form_display.[entity_type].[bundle].default.yml

    So, right:
    - the patch is about changing the storage format and runtime flow of "entity form components" settings - (fields and "extra fields")
    - everything is in place to support form modes, but the patch doesn't actually introduces them - which is just fine IMO -, and leaves explicit @todos for "introduce form modes".

    The UI impact, removal of the 'user_register_form' field setting, and stuff like "how are form modes defined exactly, how can you add more, can "create" and "edit" be two separate form modes"... are stuff for which we need community input, and an issue with 90 comments and a 200kb patch is not a great place to debate this.

    So it's just a question of how we shape the intermediate state that we wish to commit for this issue.

    1. Since current patch only uses "default', and since "what form modes are" is not within the scope of the issue, I think it would make more sense that:
      - EntityFormController::form() makes no assumptions about that yet, and just calls entity_get_render_form_display($entity, 'default');
      - entity_get_render_form_display() returns the $form_mode it was asked for. The @todo in there simply becomes "Form modes don't have custom settings yet, so just return the display for the form mode that was requested."
      I.e : storage and API are ready for form modes, but existing client code still only uses 'default' for now.
    2. Re: my own #80 item 16: No strong opinion on whether the code calling entity_get_form_display() should explicitly pass 'default' for the $form_mode param, or skip it and let 'default' be the default value in the function signature - other that we probably should try to be consistent ;-)
    3. The parts about "configuration of the form mode" in the phpdocs for entity_get_form_display() / entity_get_render_form_display() are just not true yet (there is currently no such thing as "form mode configuration"), so we should remove them for now, so that what gets committed here is self-consistent.
    4. WidgetBase::formSingleElement():
      +        'entity_form_display' => entity_get_form_display($instance['entity_type'], $instance['bundle']),
      

      If the motto of the patch is "the form display can be found in $form_state", then hook_field_widget_form_alter() receives $form_state too, and doesn't need this new entry in its $context param ? (plus, it's not reflected in the phpdoc for the hook ;-)

    amateescu’s picture

    FileSize
    16.69 KB
    207.11 KB

    Took me some time to finally realize that the patch does not really add them

    ...

    The UI impact, removal of the 'user_register_form' field setting, and stuff like "how are form modes defined exactly, how can you add more, can "create" and "edit" be two separate form modes"... are stuff for which we need community input, and an issue with 90 comments and a 200kb patch is not a great place to debate this.

    That's what I was trying to say all along, thanks for expressing it better :)

    1. Makes sense, changed.
    2. I chose to explicitly specify 'default' everywhere so the next patch won't have to.
    3. Fixed.
    4. Fixed.

    The changes to field_ui.admin.inc clash heavily with #1946404: Convert forms in field_ui.admin.inc to the new form interface, which is essentially done (@swentel can't RTBC anymore, said he'd ask @yched).

    I was talking to @swentel the other day about this conflict, and while he preferred that EntityFormDisplay got in first, I also realize that rerolling this one is much easier than the field_ui conversion, so I have no problems with getting that in first and rerolling this one after.

    amateescu’s picture

    In fact, here it is, rerolled on top of #1946404-53: Convert forms in field_ui.admin.inc to the new form interface. The after patch should be apply-able after that one is in, and the plus patch contains both of them and should pass.. hopefully.

    
    
    +++ b/core/modules/entity_reference/lib/Drupal/entity_reference/EntityReferenceAutocomplete.php
    @@ -75,7 +75,7 @@ public function getMatches($field, $instance, $entity_type, $entity_id = '', $pr
    +      $widget = entity_get_form_display($instance['entity_type'], $instance['bundle'])->getComponent($instance['field_name'], 'default');
    
    +++ b/core/modules/field/field.info.inc
    @@ -144,7 +144,7 @@ function _field_info_collate_types_reset() {
    +  if ($component = entity_get_form_display($instance['entity_type'], $instance['bundle'])->getComponent($instance['field_name'], 'default')) {
    

    The new patch also fixes that crap from #91.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-91_PLUS_field-ui-1946404-53.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    Issue tags: +Avoid commit conflicts
    FileSize
    211.78 KB

    So the reroll against #1946404: Convert forms in field_ui.admin.inc to the new form interface was pretty much ok, but #1947892: Improve DX with EntityAccessControllerInterface is what messed up a lot more things. Because of that, I can't provide any usable interdiff..

    Edit: I also had to start a new branch: 1875992-entity_form_display-new

    yched’s picture

    Nitpick of all nitpicks:
    EntityFormDisplayTest::testFieldComponent():

        $this->assertEqual($widget->randomValue, $random_value );
    

    Extra space before the closing parenthesis. (Copy/pasted from EntityDisplayTest which has the same error in HEAD - since patch touches that class too, let's fix it in both places ?)

    phpdoc for hook_field_extra_fields():

    - *   - visible: The default visibility of the element. Only for 'display'
    - *     context.
    + *   - visible: The default visibility of the element.

    OK, we should probably add that it defaults to TRUE if nothing is specified. That's why existing extra fields don't need to change their definition to explicitly declare the new property.

    More substantially :
    Since EntityFormDisplays can now specify that some "extra field" components in forms should be hidden, then every code that adds the actual $form['some_extra_field'] = array(...) elements in entity forms should now do that only if ($form_display->getComponent('some_extra_field')) (i.e only if is the element is visible).
    That's how things work on the entity view side (that's item h) in #55). Nothing else will hide the elements automagically.

    This is going to be a bit more work, those "extra field" form components are added in different places:
    - for "extra fields" defined by the entity type itself, this is typically done in overrides of EntityFormController::form(),
    But for instance, CommentFormController::form() currently adds its extra fields *before* calling parent::form(), meaning the $entity_form_display hasn't been retrieved yet.
    CategoryFormController::form() calls parent first.
    - for "extra fields" defined by 3rd party code, it seems hook_form_alter() + some magic to determine that "this is an entity form" is the current way to go.
    That's what translation_entity.module currently does (translation_entity_form_alter()), and the form additions end up being handled in EntityTranslationController::entityFormAlter() (er, even though I see nothing in there that matches the "extra field" defined in translation_entity_field_extra_fields()...)

    Since the UI currently doesn't let you hide "extra fields" in forms, pushing that to a followup should be fine and is probably the best approach.
    But then it means the "visibility" property advertized in hook_field_extra_fields() for 'form' extra fields is just not working yet, and should be only added in that same followup ?

    yched’s picture

    Also :

    + * @todo Make the $form_mode parameter non-optional by introducing the form
    + * modes concept.
    + *
    + * @return \Drupal\entity\Plugin\Core\Entity\EntityFormDisplay
    + *   The EntityFormDisplay object associated to the form mode.
    + */
    +function entity_get_form_display($entity_type, $bundle, $form_mode = 'default') {

    - As we established earlier, as far as the API is involved, the form modes concept is already introduced, it's just the current code in core that doesn't leverage it. So IMO that @todo can be removed here, and replaced by an inline @todo comment in EntityFormController::form() where it calls entity_get_render_form_display().
    - Since you moved all callers to provide 'default' as an explicit 3rd param, maybe the default value can be ditched here ? (consistent with entity_get_display())

    amateescu’s picture

    FileSize
    10.38 KB
    214.47 KB

    Implemented the review points from #95 and #96.

    Your analysis about extra fields from #95 got me thinking that EntityFormController::form() is not really a good place to set up the form display, so I completely refactored that part to resemble the entity building process.

    Status: Needs review » Needs work

    The last submitted patch, 1875992-97.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    631 bytes
    214.46 KB

    That's for posting patches at 3am :/

    Status: Needs review » Needs work

    The last submitted patch, 1875992-99.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Needs review
    FileSize
    830 bytes
    214.48 KB

    ...

    amateescu’s picture

    andypost’s picture

    The primary idea to use entityDisplay to store widget settings is great!
    But the summary does not provides enough details of changes to modes - please update it (could be used latter in change notice) and it makes easy to review this patch

    +++ b/core/includes/entity.api.phpundefined
    @@ -502,6 +502,27 @@ function hook_entity_display_alter(\Drupal\entity\Plugin\Core\Entity\EntityDispl
    + *   - form_mode: The form mode, e.g. 'default', 'profile', 'register'...
    ...
    +  if ($context['entity_type'] == 'user' && $context['form_mode'] == 'register') {
    
    +++ b/core/includes/entity.incundefined
    @@ -688,7 +688,7 @@ function entity_get_display($entity_type, $bundle, $view_mode) {
    -      'viewMode' => $view_mode,
    +      'mode' => $view_mode,
    
    +++ b/core/modules/entity/entity.installundefined
    @@ -37,7 +37,46 @@ function _update_8000_entity_get_display($entity_type, $bundle, $view_mode) {
    +function _update_8000_entity_get_form_display($entity_type, $bundle, $form_mode) {
    ...
    +    'mode' => $form_mode,
    

    Please clarify in summary details of form_mode vs mode.
    It's not clear what is going to change...

    yched’s picture

    The refactors in EntityFormController go in the right direction I think, but a couple remarks:

    build():

    -    // During the initial form build, add the entity to the form state for use
    -    // during form building and processing. During a rebuild, use what is in the
    -    // form state.
    -    if (!$this->getEntity($form_state)) {
    +    // During the initial form build, add the entity and the form display to the
    +    // form state for use during form building and processing. During a rebuild,
    +    // use what is in the form state.
    +    if (!$this->getEntity($form_state) || !$this->getFormDisplay($form_state)) {
           $this->init($form_state, $entity);
         }

    With the two conditions, this looks a bit sloppy.
    So ok, in practice, both the entity and the the EFD should be set or not set at the same time.
    But what the code does is "if either one is not set, run init(), that will set both",

    Something like "if (needs init) { do the init }" would be clearer.
    if (!isset($form_state['controller'])) {$this->init();} would work IMO.

    Then, build() does :

        $entity = $this->getEntity($form_state);
        $form = $this->form($form, $form_state, $entity);
    

    So I guess it could also get() the EFD and pass it along as a new param in form() ? Would be more consistent with how it's done on the EntityRenderController side.
    We can't inject the EFD in the rest of the callstack like we did in EntityRenderController, since "the rest of the callstack" here is hook_form_alter(), and we're not changing the signature of that one, but injecting it in form() would still make sense IMO.

    init():

        // Add the controller to the form state so it can be easily accessed by
        // module-provided form handlers there.
        $form_state['controller'] = $this;
        $this->setEntity($entity, $form_state);
        $this->prepareEntity($entity);
    
        // @todo Allow the usage of different form modes by exposing a hook and the
        // UI for them.
        $form_display = entity_get_render_form_display($entity, 'default');
        $this->prepareFormDisplay($form_display, $entity, 'default');
        $this->setFormDisplay($form_display, $form_state);
    

    - There are now three distinct actions in there, there should be an empty line and a dedicated code comment before $this->setEntity()
    - prepareEntity() is there just to allow entity-type specific massaging in subclasses. I don't think I see the need to mirror this as a separate prepareFormDisplay() method, I think inlining the drupal_alter() here before the set() should be fine.
    If the method stays, though, then maybe set(); prepare(); for consistency ?

    andypost’s picture

    amateescu’s picture

    Assigned: amateescu » Unassigned
    FileSize
    1.71 KB
    213.45 KB

    Something like "if (needs init) { do the init }" would be clearer.
    if (!isset($form_state['controller'])) {$this->init();} would work IMO.

    This was done by #1913618: Convert EntityFormControllerInterface to extend FormInterface.

    So I guess it could also get() the EFD and pass it along as a new param in form() ? Would be more consistent with how it's done on the EntityRenderController side.

    #1913618: Convert EntityFormControllerInterface to extend FormInterface removed the $entity parameter that was passed to form(), so I don't think adding $form_display now is a good idea.

    - There are now three distinct actions in there, there should be an empty line and a dedicated code comment before $this->setEntity()

    Not related to this patch?

    - prepareEntity() is there just to allow entity-type specific massaging in subclasses. I don't think I see the need to mirror this as a separate prepareFormDisplay() method, I think inlining the drupal_alter() here before the set() should be fine.

    Done.

    P.S. This is only a pseudo interdiff because of the merge conflicts with #1913618: Convert EntityFormControllerInterface to extend FormInterface.

    yched’s picture

    Yeah, #1913618: Convert EntityFormControllerInterface to extend FormInterface reshuffled this area quite a bit.

    Just wondering, though : since
    - $form_state['form_display'] was so far mirroring what the controller was doing with $form_state['entity'],
    - the latter just disappeared in favor of $this->entity / $form_state['controller']->getEntity();...
    then it seems the EFD should follow the same pattern ? Not placed in form state, but stored as a property of the controller and accessed through getFormDisplay() ?

    Other than that, looks good to me :-) But as @andypost wrote, an issue summary would help reviewrs / committers.

    amateescu’s picture

    No strong opinions about that, and I'll be away for two weeks so anyone can go crazy with it. (that's why I unassigned myself earlier)

    yched’s picture

    @amateescu: Ok, I'm fine with RTBCing #106 - is there any chance you can still wrap up an issue summary before you leave ?
    I have limited core time right now and I'm trying to save every bit to move #1969728: Implement Field API "field types" as TypedData Plugins forward.

    yched’s picture

    is there any chance you can still wrap up an issue summary before you leave ?

    OK, I guess not... I'll try to write one.

    yched’s picture

    Issue summary: View changes

    Patch summary.

    yched’s picture

    Status: Needs review » Reviewed & tested by the community

    Issue summary added.

    As per #109, RTBC.

    yched’s picture

    Issue summary: View changes

    Remaining tasks

    yched’s picture

    Reroll.

    fago’s picture

    I looked through the patch and I overall like where this goes and the analogy to EntityDisplay. The only thing that seems a bit weird to me is the usage of entity.module vs the Core component as I do not really see a reason nor a clear line on what goes into entity.module and what into the component - especially as we can provide plugins from the Core component also and so with the data type plugins.

    Howsoever, the patch only continues what we already have here and makes it consistent with EntityDisplay. So let's deal with that in a follow-up.

    The patch only adds support for "form modes" at the EntityFormDisplay storage level, but does not have core entities use them for now. Only the 'default' form mode is actually used - thus, no visible functionnal change on this regard.

    Yeah, this and the already existing form operations overlap partly I think. E.g., we already have user profile and register operations which would qualify as form_modes of the same form also.

    Follow-up: Decide how core can actually leverage form modes (what they are exactly, how they are exposed / added, UI...)

    Yep, let's clarify this relationship in its own issue.

    yched’s picture

    Reroll.

    yched’s picture

    effulgentsia’s picture

    The issue summary looks good to me. Thanks!

    yched’s picture

    Issue tags: -Field API
    FileSize
    213.39 KB

    Reroll again.

    Was about to add the "Avoid commit conflicts" tag, but it's already there ;-)

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, entity_form_display-1875992-117.patch, failed testing.

    tim.plunkett’s picture

    +++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
    @@ -130,7 +132,7 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
    -          '#title' => $widget_types[$instance['widget']['type']]['label'],
    +          '#title' => $widget_types[$widget_configuration['type']]['label'],
    

    This is causing an undefined index error

    yched’s picture

    Status: Needs work » Reviewed & tested by the community
    FileSize
    1.97 KB
    214.03 KB

    Test added in #1549506: Edit and delete links should be hidden for locked fields needed to be adapted to the new location for $widget['settings'].

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs work
    Issue tags: +needs profiling

    We need to performance test this change... some xhprof numbers would be nice.

    yched’s picture

    Reroll.
    Will try to come up with some benchmarks later today - if not, won't be able to tackle those myself before this week-end's Portland sprint.

    amateescu’s picture

    FileSize
    14.45 KB

    Thanks @yched for keeping this patch alive! :)

    Here's an xhprof run from my local dev environment, first run is without the patch and the second one is with the patch applied. The wall time and cpu numbers can't be trusted because I had various results for them, but function calls and memory usage numbers are consistent.

    EFD_xhprof.png

    [Update] And here's one with the APC classloader instead of the default one.

    Only local images are allowed.

    This time, wt and cpu are consistently improved throughout all xhprof runs, and the memory 'regression' is consistent.

    yched’s picture

    Status: Needs work » Needs review
    Issue tags: -needs profiling

    Thanks @amateescu !

    If CPU time can't be trusted with xhprof, then I guess some ab runs would be needed. Also, would help if you specify which page the bench was run on, and with whiwh setup (how many fields on the page, how many different widgets...)

    amateescu’s picture

    I updated my comment in #123 with another comparison, this time using the apc class loader.

    The bench was done on the node/add/article page from the standard profile, so 1 extra field (title) and 3 regular fields (tags, image, body).

    Status: Needs review » Needs work

    The last submitted patch, entity_form_display-1875992-122.patch, failed testing.

    andypost’s picture

    Status: Needs work » Reviewed & tested by the community

    Back to RTBC

    andypost’s picture

    Status: Reviewed & tested by the community » Needs work
    Issue tags: +Needs reroll

    Last merge is broken

    amateescu’s picture

    Status: Needs work » Needs review
    Issue tags: -Needs reroll
    FileSize
    663 bytes
    214.41 KB

    Easy fix. This interdiff is not pushed to the usual branch because @yched's merge from #122 is also not pushed yet.

    yched’s picture

    Yes, couldn't push it earlier, git.d.o consistently rejected my credentials. Dunno if that was a temporary glitch, can't try again right now. If you have better luck... :-)

    andypost’s picture

    Status: Needs review » Reviewed & tested by the community

    Back to RTBC. I see no reason to extend tests because we use this for core and this functionality already covered with tests

    +++ b/core/includes/entity.incundefined
    @@ -707,12 +707,99 @@ function entity_get_render_display(EntityInterface $entity, $view_mode) {
    +function entity_get_form_display($entity_type, $bundle, $form_mode) {
    +  // Try loading the entity from configuration.
    +  $entity_form_display = entity_load('entity_form_display', $entity_type . '.' . $bundle . '.' . $form_mode);
    ...
    +    $entity_form_display = entity_create('entity_form_display', array(
    
    +++ b/core/lib/Drupal/Core/Entity/EntityFormController.phpundefined
    @@ -117,6 +119,20 @@ protected function init(array &$form_state) {
    +    $form_display = entity_get_render_form_display($this->entity, 'default');
    ...
    +    \Drupal::moduleHandler()->alter('entity_form_display', $form_display, $form_display_context);
    

    seems follow-up should inject entityManager

    effulgentsia’s picture

    I updated my comment in #123 with another comparison, this time using the apc class loader.

    Looks like that image didn't make it into that comment? Underneath the sentence "[Update] And here's one with the APC classloader instead of the default one.", I just see a red X with tooltip text that says "Only local images are allowed."

    Can you post a new comment in order to get the file onto d.o.? Thanks.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, entity_form_display-1875992-129.patch, failed testing.

    amateescu’s picture

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

    Rerolled.

    Re #132: That's because I attached the file in a comment that was never posted and d.o deleted it after some time :( @alexpott said that he wants to do his own round of benchmarks so I'll wait for him.

    Status: Reviewed & tested by the community » Needs work

    The last submitted patch, entity_form_display-1875992-134.patch, failed testing.

    amateescu’s picture

    Status: Needs work » Reviewed & tested by the community
    FileSize
    1021 bytes
    215.4 KB

    The recent test coverage added in #1996378: Edit broken because of #1043198 and routing system bug + missing test coverage shows us that we were breaking edit.module with this patch... oops :)

    For now I just copied the code from EntityFormController, but after looking around in EditFieldForm it occured to me that, in a followup, we can totally drop most of the custom code in there and just rely on the regular form controllers + use hook_entity_form_display_alter() to hide all fields except the one that we want to edit. #1465774: Provide a 'Hidden' field widget FTW!

    aspilicious’s picture

    Hmmm...

    + \Drupal::moduleHandler()->alter('entity_form_display', $form_display, $form_display_context);

    Shouldn't the module handler be injected?

    amateescu’s picture

    Wim Leers’s picture

    #136: Thanks, that's very interesting :)

    alexpott’s picture

    Status: Reviewed & tested by the community » Needs review
    Issue tags: +needs profiling

    We need to add profiling for a node with a comment form.

    alexpott’s picture

    Method:

    1. Enable apc classloader
    2. Install standard profile
    3. Enable anonymous commenting
    4. Create an article node
    5. View node as anon multiple times throwing away the first request

    EDIT: Removed perf testing cause @msonnabaum pointed out xdebug needs to be disabled.

    alexpott’s picture

    Redid testing without xdebug enabled

    +-----------------------------------------+------------+------------+------------+------------+------------+
    | namespace                               |        min |        max |       mean |     median |       95th |
    +-----------------------------------------+------------+------------+------------+------------+------------+
    | Calls                                   |            |            |            |            |            |
    |                                         |            |            |            |            |            |
    | al-8x-head-drupal8alt-dev-xhprof        |     36,291 |     36,291 |     36,291 |     36,291 |     36,291 |
    | al-entityformmode-drupal8alt-dev-xhprof |     36,979 |     36,979 |     36,979 |     36,979 |     36,979 |
    |                                         |            |            |            |            |            |
    | Wall time                               |            |            |            |            |            |
    |                                         |            |            |            |            |            |
    | al-8x-head-drupal8alt-dev-xhprof        |    189,848 |    196,913 |    191,699 |    191,029 |    195,484 |
    | al-entityformmode-drupal8alt-dev-xhprof |    190,710 |    198,050 |    192,884 |    192,566 |    195,939 |
    |                                         |            |            |            |            |            |
    | CPU time                                |            |            |            |            |            |
    |                                         |            |            |            |            |            |
    | al-8x-head-drupal8alt-dev-xhprof        |    166,876 |    175,817 |    169,452 |    168,752 |    174,276 |
    | al-entityformmode-drupal8alt-dev-xhprof |    168,675 |    174,615 |    170,255 |    169,785 |    172,792 |
    |                                         |            |            |            |            |            |
    | Memory usage                            |            |            |            |            |            |
    |                                         |            |            |            |            |            |
    | al-8x-head-drupal8alt-dev-xhprof        | 17,573,216 | 17,573,216 | 17,573,216 | 17,573,216 | 17,573,216 |
    | al-entityformmode-drupal8alt-dev-xhprof | 17,949,272 | 17,949,272 | 17,949,272 | 17,949,272 | 17,949,272 |
    |                                         |            |            |            |            |            |
    | Peak memory usage                       |            |            |            |            |            |
    |                                         |            |            |            |            |            |
    | al-8x-head-drupal8alt-dev-xhprof        | 17,738,728 | 17,738,728 | 17,738,728 | 17,738,728 | 17,738,728 |
    | al-entityformmode-drupal8alt-dev-xhprof | 18,118,112 | 18,118,112 | 18,118,112 | 18,118,112 | 18,118,112 |
    |                                         |            |            |            |            |            |
    +-----------------------------------------+------------+------------+------------+------------+------------+
    
    yched’s picture

    Status: Needs review » Reviewed & tested by the community

    So that's +0.5% CPU, +2% peak memory.
    Sounds reasonable ? Temptatively setting back to RTBC ?

    amateescu’s picture

    alexpott’s picture

    Title: Add EntityFormDisplay objects for entity forms » Change notice: Add EntityFormDisplay objects for entity forms
    Priority: Normal » Critical
    Status: Reviewed & tested by the community » Active
    Issue tags: +Needs change record

    Committed 518c4d1 and pushed to 8.x. Thanks!

    alexpott’s picture

    Committed 518c4d1 and pushed to 8.x. Thanks!

    effulgentsia’s picture

    Issue tags: -Avoid commit conflicts

    Committed, so no conflicts to avoid anymore.

    larowlan’s picture

    Status: Active » Needs review
    andypost’s picture

    Title: Change notice: Add EntityFormDisplay objects for entity forms » Add EntityFormDisplay objects for entity forms
    Priority: Critical » Normal
    Status: Needs review » Fixed

    @amateescu Looks enough ;)

    swentel’s picture

    Issue tags: -Needs change record

    Removing tag

    swentel’s picture

    Issue summary: View changes

    Formatting

    amateescu’s picture

    Status: Fixed » Closed (fixed)

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

    Anonymous’s picture

    Issue summary: View changes

    Added the form modes followup.