#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();
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.
Comment | File | Size | Author |
---|---|---|---|
#144 | entity_form_display-1875992-144.patch | 214.53 KB | amateescu |
#136 | entity_form_display-1875992-136.patch | 215.4 KB | amateescu |
#136 | interdiff.txt | 1021 bytes | amateescu |
#134 | entity_form_display-1875992-134.patch | 214.4 KB | amateescu |
#129 | entity_form_display-1875992-129.patch | 214.41 KB | amateescu |
Comments
Comment #1
yched CreditAttribution: yched commentedIt's probably best to wait after #1875974: Abstract 'component type' specific code out of EntityDisplay though.
Comment #2
amateescu CreditAttribution: amateescu commentedI 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 :/
Comment #3
effulgentsia CreditAttribution: effulgentsia commented+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!
Comment #4
amateescu CreditAttribution: amateescu commentedThis 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...
Comment #6
swentel CreditAttribution: swentel commentedAwesome indeed! Adding new patch which should have lots more test passes, will push when this comes back.
Comment #8
swentel CreditAttribution: swentel commentedGreat, pushed eb225c7
Comment #9
swentel CreditAttribution: swentel commentedThis should be even better.
Comment #11
swentel CreditAttribution: swentel commentedPushed 2ae74d1
Comment #12
swentel CreditAttribution: swentel commentedMore test fixes, pushed following:
b313e5c
1db995b
Looking into the 3 biggest test failures now
Comment #14
swentel CreditAttribution: swentel commentedFixed and pushed user registration and multiform. RDF chokes on rdf_modules_installed() - ah rdf and entities, fun ..
Comment #15
swentel CreditAttribution: swentel commentedComment #17
swentel CreditAttribution: swentel commentedPushed couple of more fixes, done for today.
Comment #19
amateescu CreditAttribution: amateescu commentedThe failures from
ContactPersonalTest
are caused by this issue: #1856556: Convert user contact form into a contact category/bundle.Comment #20
amateescu CreditAttribution: amateescu commentedOh, we have the same problem in EntityDisplay. This should do it for now..
Comment #22
amateescu CreditAttribution: amateescu commentedFixed 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.
Comment #24
amateescu CreditAttribution: amateescu commentedI 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..
Comment #25
swentel CreditAttribution: swentel commentedThis is weird - if I changed following line in testOptionsAllowedValuesBoolean() (line 222)
to
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.
Comment #26
yched CreditAttribution: yched commentedCurrent 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()
Comment #27
swentel CreditAttribution: swentel commentedRight - 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 ;)
Comment #28
swentel CreditAttribution: swentel commentedOk, 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
orfield_ui_field_edit_form_submit
- or at least, that's were it is reset.Comment #29
swentel CreditAttribution: swentel commentedOk, 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.
Comment #30
swentel CreditAttribution: swentel commentedAlso, 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.
Comment #31
swentel CreditAttribution: swentel commentedThis should do it - the widget type got lost. I'll push everything, merge with core when this turns green.
Comment #33
amateescu CreditAttribution: amateescu commentedI just realized that I completely skipped the handling of extra fields in the initial patch, so 'needs work' is the correct status for now.
Comment #34
amateescu CreditAttribution: amateescu commentedThe result of me being lazy is that I opened #1954188: Revaluate the concept of 'extra fields' instead :)
Comment #35
amateescu CreditAttribution: amateescu commentedIn the meantime, let's get a green patch here.
Comment #36
amateescu CreditAttribution: amateescu commentedOk, 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()
andEntityFormDisplay::getWidget()
to be sure that I haven't messed up something there :)Comment #37
effulgentsia CreditAttribution: effulgentsia commented#36: 1875992-36.patch queued for re-testing.
Comment #39
amateescu CreditAttribution: amateescu commentedRerolled for #1801356: Entity reference autocomplete using routes.
Comment #40
andypost#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
Comment #41
amateescu CreditAttribution: amateescu commentedYou missed the reroll from #39?
Comment #42
andypost@amateescu no, thats what symplytest.me shows when I test patch #39
Comment #43
amateescu CreditAttribution: amateescu commented#39: 1875992-39.patch queued for re-testing.
Comment #44
effulgentsia CreditAttribution: effulgentsia commentedRe #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.
Comment #45
yched CreditAttribution: yched commentedStarting to review - @amateescu : could you push the reroll / conflict resolution from #39 to the sandbox ?
Comment #46
amateescu CreditAttribution: amateescu commentedSure, 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.
Comment #47
yched CreditAttribution: yched commentedInitial review, I only looked at the surroundings for now, didn't get to the "meaty" parts :-)
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.
$this->form_display_options does not seem really needed as a separate $this property ?
Yay !!
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.
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 ?)
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.
Why is the extra param needed ?
Nitpick : would make more sense to add 'entity' at the beginning of the list ?
Same for EntityUnitTestBase.php, FieldAccessTest.php
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 ?
"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.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 ?
Why is this commented out ?
Hm, really ? :-)
Comment #48
amateescu CreditAttribution: amateescu commentedThanks for starting the review :)
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.
That's just to be consistent with how the rest of the test was written.
Yay indeed :)
Removed those parts and created EntityFormDisplayTest.
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.
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.
Fixed all three.
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..
Fixed.
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 :(
Removed it along with the creation of an entity_display from above which wasn't needed as well.
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.Comment #49
andypostAs I already mention on IRC the more suitable name would be
buildMode
because EntityDisplay operates by components having some renderable array ($build
) as resultComment #51
amateescu CreditAttribution: amateescu commentedFor 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.
Comment #52
yched CreditAttribution: yched commentedIf 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 ?
Copy / paste - should be EntityFormDisplay ?
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.
Comment #53
amateescu CreditAttribution: amateescu commentedFixed 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..
Now, about this:
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.
Comment #55
yched CreditAttribution: yched commentedSorry 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.
Comment #56
yched CreditAttribution: yched commentedReview for the parts I didn't look so far :
Misses empty line between last method and closing curly brace.
// Clear the persisted formatter, if any.
Leftover mention of "formatter"
Looks like a needless change ?
Why is this needed ? It seems we didn't need anything similar for entity_displays so far ?
Besides, ConfigStorageController::resetCache() is a no-op ;-)
We can't completely remove the variables themselves, because there's still the configuration of view modes in there.
Ouch. Care to open an issue to remove that t() ? (there's another one for the $field['type'] 3 lines above)
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.
Comment #57
amateescu CreditAttribution: amateescu commentedRe #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 thatentity_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..Comment #58
yched CreditAttribution: yched commentedhook_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.
Comment #59
swentel CreditAttribution: swentel commentedtagging
Comment #60
amateescu CreditAttribution: amateescu commentedI 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).
Can we leave that for a followup? I'll open new issues for points 7 and 8 too.
Comment #61
amateescu CreditAttribution: amateescu commentedComment #63
amateescu CreditAttribution: amateescu commentedHope I got`em all.
Comment #65
amateescu CreditAttribution: amateescu commentedFixed and tested the upgrade path.
Comment #67
amateescu CreditAttribution: amateescu commentedAnd, 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.
Comment #68
swentel CreditAttribution: swentel commentedMan, I love this patch, went through scanning for doc problems, could only see this for now:
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 ?
Comment #70
amateescu CreditAttribution: amateescu commentedFixed.
Brought back in EntityFormTest.
Or we could do it here and re-roll when the other one gets in :/ Which I did.
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!
Comment #71
yched CreditAttribution: yched commentedNot clear to me why that makes it not needed ?
Comment #73
amateescu CreditAttribution: amateescu commentedFixed 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.
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.
Comment #74
swentel CreditAttribution: swentel commentedGood point, we can always revisit that for both in follow up in case we feel we really need it.
Comment #75
yched CreditAttribution: yched commented#73: ah, sure, makes sense. Thanks for the explanation.
Comment #76
amateescu CreditAttribution: amateescu commentedRerolled on top of latest HEAD.
Comment #78
amateescu CreditAttribution: amateescu commentedFixed merge conflicts with #1967106: FieldInstance::getField() method.
Comment #79
amateescu CreditAttribution: amateescu commentedFixed another (new) merge conflict with #1955678: Remove formatter and widgets legacy plugin.
Comment #80
yched CreditAttribution: yched commentedThanks 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.
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)
We want to test $body_instance['widget'], not $body_instance['form'] ?
Since you modify the phpdoc, let's switch it over to {@inheritdoc}
$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)
Something doesn't look fully right here. Maybe this property should be renamed 'mode' in Overviewbase ?
Redundant ? The second line should do a $entity_form_display->getComponent() ?
This data is only used within an if() branch below --> move that entity_get_form_display() call within the if() as well ?
Comment is not relevant now :-)
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.
The form display is already available in $form['#entity_form_display'] ?
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).
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.
Fine, but then the rest of the code needs separate if (isset($data['display']) and if (isset($data['widget']) checks ?
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.*
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.
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.
Comment #81
amateescu CreditAttribution: amateescu commentedFixed 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..
Comment #82
swentel CreditAttribution: swentel commentedRE: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.
Comment #84
amateescu CreditAttribution: amateescu commentedActually saving the entity that you're working with always helps..
Comment #85
yched CreditAttribution: yched commentedThanks for the quick updates, @amateescu !
Still mulling over "form modes" for now, meanwhile, rest of the review :
Not sure why the mergeDeep is needed ? We don't do that on the EntityDisplay side ?
That's already in #1974474: Make sure within field_attach_*() that we are working with a BC entity - really needed here ?
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'].
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.
There's no such check in EntityDisplay::getFormatter(), why is this needed here ? Maybe add a comment about that ?
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
$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:
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 ?
Comment #86
amateescu CreditAttribution: amateescu commentedSince 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().
Comment #87
yched CreditAttribution: yched commentedNo 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...).
Right - only for internal cleanup :-), but that still leaves the API a bit unclear.
OK, we might want to adjust that in a followup.
Comment #88
amateescu CreditAttribution: amateescu commentedOk.. 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()..
Comment #89
tim.plunkettThe 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...
Comment #90
yched CreditAttribution: yched commentedSo, 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.
- 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.
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 ;-)
Comment #91
amateescu CreditAttribution: amateescu commentedThat's what I was trying to say all along, thanks for expressing it better :)
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.
Comment #92
amateescu CreditAttribution: amateescu commentedIn 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.
The new patch also fixes that crap from #91.
Comment #94
amateescu CreditAttribution: amateescu commentedSo 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
Comment #95
yched CreditAttribution: yched commentedNitpick of all nitpicks:
EntityFormDisplayTest::testFieldComponent():
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():
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 ?
Comment #96
yched CreditAttribution: yched commentedAlso :
- 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())
Comment #97
amateescu CreditAttribution: amateescu commentedImplemented 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.
Comment #99
amateescu CreditAttribution: amateescu commentedThat's for posting patches at 3am :/
Comment #101
amateescu CreditAttribution: amateescu commented...
Comment #102
amateescu CreditAttribution: amateescu commentedRe-rolled for #1952652: Instance components are not removed from entity display when the field/instance is deleted..
Comment #103
andypostThe 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
Please clarify in summary details of form_mode vs mode.
It's not clear what is going to change...
Comment #104
yched CreditAttribution: yched commentedThe refactors in EntityFormController go in the right direction I think, but a couple remarks:
build():
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 :
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():
- 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 ?
Comment #105
andypostNow it needs serious merge with commited #1913618: Convert EntityFormControllerInterface to extend FormInterface
Comment #106
amateescu CreditAttribution: amateescu commentedThis was done by #1913618: Convert EntityFormControllerInterface to extend FormInterface.
#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.
Not related to this patch?
Done.
P.S. This is only a pseudo interdiff because of the merge conflicts with #1913618: Convert EntityFormControllerInterface to extend FormInterface.
Comment #107
yched CreditAttribution: yched commentedYeah, #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.
Comment #108
amateescu CreditAttribution: amateescu commentedNo 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)
Comment #109
yched CreditAttribution: yched commented@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.
Comment #110
yched CreditAttribution: yched commentedOK, I guess not... I'll try to write one.
Comment #110.0
yched CreditAttribution: yched commentedPatch summary.
Comment #111
yched CreditAttribution: yched commentedIssue summary added.
As per #109, RTBC.
Comment #111.0
yched CreditAttribution: yched commentedRemaining tasks
Comment #112
yched CreditAttribution: yched commentedReroll.
Comment #113
fagoI 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.
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.
Yep, let's clarify this relationship in its own issue.
Comment #114
yched CreditAttribution: yched commentedReroll.
Comment #115
yched CreditAttribution: yched commented#114: entity_form_display-1875992-114.patch queued for re-testing.
Comment #116
effulgentsia CreditAttribution: effulgentsia commentedThe issue summary looks good to me. Thanks!
Comment #117
yched CreditAttribution: yched commentedReroll again.
Was about to add the "Avoid commit conflicts" tag, but it's already there ;-)
Comment #119
tim.plunkettThis is causing an undefined index error
Comment #120
yched CreditAttribution: yched commentedTest added in #1549506: Edit and delete links should be hidden for locked fields needed to be adapted to the new location for $widget['settings'].
Comment #121
alexpottWe need to performance test this change... some xhprof numbers would be nice.
Comment #122
yched CreditAttribution: yched commentedReroll.
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.
Comment #123
amateescu CreditAttribution: amateescu commentedThanks @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.
[Update] And here's one with the APC classloader instead of the default one.
This time, wt and cpu are consistently improved throughout all xhprof runs, and the memory 'regression' is consistent.
Comment #124
yched CreditAttribution: yched commentedThanks @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...)
Comment #125
amateescu CreditAttribution: amateescu commentedI 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).Comment #127
andypostBack to RTBC
Comment #128
andypostLast merge is broken
Comment #129
amateescu CreditAttribution: amateescu commentedEasy fix. This interdiff is not pushed to the usual branch because @yched's merge from #122 is also not pushed yet.
Comment #130
yched CreditAttribution: yched commentedYes, 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... :-)
Comment #131
andypostBack to RTBC. I see no reason to extend tests because we use this for core and this functionality already covered with tests
seems follow-up should inject entityManager
Comment #132
effulgentsia CreditAttribution: effulgentsia commentedLooks 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.
Comment #134
amateescu CreditAttribution: amateescu commentedRerolled.
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.
Comment #136
amateescu CreditAttribution: amateescu commentedThe 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!Comment #137
aspilicious CreditAttribution: aspilicious commentedHmmm...
+ \Drupal::moduleHandler()->alter('entity_form_display', $form_display, $form_display_context);
Shouldn't the module handler be injected?
Comment #138
amateescu CreditAttribution: amateescu commentedWe have #1909418: Allow Entity Controllers to have their dependencies injected for that.
Comment #139
Wim Leers#136: Thanks, that's very interesting :)
Comment #140
alexpottWe need to add profiling for a node with a comment form.
Comment #141
alexpottMethod:
EDIT: Removed perf testing cause @msonnabaum pointed out xdebug needs to be disabled.
Comment #142
alexpottRedid testing without xdebug enabled
Comment #143
yched CreditAttribution: yched commentedSo that's +0.5% CPU, +2% peak memory.
Sounds reasonable ? Temptatively setting back to RTBC ?
Comment #144
amateescu CreditAttribution: amateescu commentedRerolled and fixed a conflict with #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield).
Comment #145
alexpottCommitted 518c4d1 and pushed to 8.x. Thanks!
Comment #146
alexpottCommitted 518c4d1 and pushed to 8.x. Thanks!
Comment #147
effulgentsia CreditAttribution: effulgentsia commentedCommitted, so no conflicts to avoid anymore.
Comment #148
larowlanDraft notice here https://drupal.org/node/2012166
Comment #149
andypost@amateescu Looks enough ;)
Comment #150
swentel CreditAttribution: swentel commentedRemoving tag
Comment #150.0
swentel CreditAttribution: swentel commentedFormatting
Comment #151
amateescu CreditAttribution: amateescu commentedHere's the form modes followup: #2014821: Introduce form modes UI and their configuration entity
Comment #152.0
(not verified) CreditAttribution: commentedAdded the form modes followup.