Updated: Comment #31

Problem/Motivation

Most base fields still use one-off ad-hoc render / form API elements in entity views and forms instead of leveraging the generic widget and formatter rendering pipelines, and thus do not benefit of the generic features it provides :

  1. integration with in-place editing
  2. "field translatability" hints in forms
  3. (optionally) allowing site admins to configure different widgets / formatters

Proposed resolution

- Make base fields use widgets and formatters, by using setDisplayOptions() in the field definition.
- When it makes sense (mostly : when the base field was exposed as an "extra field" in hook_field_extra_fields() in HEAD so far), make this configurable in "Manage displays" screens by using setDisplayConfigurable()
- remove the exising ad-hoc render / form API elements

Remaining tasks

See Child Issues

User interface changes

Entity base fields use widgets and formatters. For those where it makes sense, the setDisplayConfigurable() flag can be used, which makes the widget/formatter configurable in Field UI's "Manage display" screens.

API changes

Resulting render structures for entity views and entity forms are changed.

Original report by @Wim Leers

This issue is the sister issue to #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title, which is about getting rid of custom code for formatting and forms for the node.created entity property.

This issue is about converting all other entity properties: node.title, node.author, taxonomy_term.name, and so on.

CommentFileSizeAuthor
#28 widgets-and-formatters-2010930-28.patch14.25 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,353 pass(es), 473 fail(s), and 92 exception(s). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers’s picture

effulgentsia’s picture

This issue is about converting all other entity properties: node.title, node.author, taxonomy_term.name, and so on.

Let's get a list of what all those other ones are into the issue summary, not just for node, but for all core entity types. We can find them by searching HEAD for function baseFieldDefinitions(). The question then becomes do we all agree that the scope of this issue should only be about *rendered* base fields, as per the issue title, and if so, how do we define that? For example, should we consider user.mail as a (potentially) rendered base field, even though it's normally considered private information and not typically rendered anywhere? Alternatively, "rendered" might be the wrong criterion here, so any thoughts on what the correct criterion should be is welcome. For example, while in-place editing only cares about rendered fields, translation UIs care about all editable fields. But maybe editable, non-rendered fields should be done in a separate issue?

Also, looks like user.signature and term.description are in process of being converted to configurable fields: #1548204: Remove user signature and move it to contrib and #569434: Remove taxonomy term description field; provide description field for forum taxonomy, so we should probably leave them out of the scope of this issue.

effulgentsia’s picture

Alternatively, "rendered" might be the wrong criterion here, so any thoughts on what the correct criterion should be is welcome.

I left a comment on #1798140-11: Agree upon terminology for configurable fields asking for help figuring this question out.

effulgentsia’s picture

Title: Apply formatters and widgets to rendered entity base fields (all besides node.created) » Apply formatters and widgets to rendered entity base fields (all besides node.title)

#1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title is now pretty far along with node.title as the pilot use case. So retitling this issue accordingly.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Issue summary: View changes
Status: Postponed » Active
Issue tags: +sprint
Related issues: +#1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title

#1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title just landed, so now we can finally work on this patch! :)

I should be able to implement support for other base fields in an analogous manner.

yched’s picture

Support for widgets/formatters on node.title currently relies on a smart hack (altering the EntityDisplay object at runtime, making it treat 'title' both as an "extra fields" and as a "field").

That was needed to move forward, but before we start spilling similar hacks in more places for more base fields, it would be best to tackle #2144919: Allow widgets and formatters for base fields to be configured in Field UI, which defines the final "clean, non hackish way" for a base field to leverage widgets & formatters.

effulgentsia’s picture

In case it's helpful, here's my recommendation on how to approach this, but I'm not attached to it being done this way:

Step 1) Identify all the fields we want to cover in this issue, and get to consensus on that list (see #2).

Step 2) Start with a patch that converts one field of each type, so that we can see what unique issues come up per type (e.g., node.created may turn up some tricky stuff due to being a date and node.uid may turn up some tricky stuff due to being an entity (user) reference) and work through those issues. At this stage, I think interim patches that continue with the hook_entity_display_alter() approach until #2144919: Allow widgets and formatters for base fields to be configured in Field UI is completed is fine, since that's just a few lines of easy code that can be removed from the patch once that issue is in.

Step 3) Finish by getting all the other fields converted, by which time, #2144919: Allow widgets and formatters for base fields to be configured in Field UI will hopefully be done.

yched’s picture

Agreed, makes lots of sense.

Converting more base fields will probably involve changing their field type (like you had to do for node.title), since most base fields currently still use "old" basic field types that were designed for base fields only and predate "merge (configurable) Field API with the new EntityField API" - and those have no widgets/formatters at all...

So, that's some work in itself, and yep, the sooner it starts, the better

effulgentsia’s picture

Gábor Hojtsy’s picture

#2111887: Regression: Only title (of base fields) on nodes is marked as translatable would also need this issue for an ideal solution. Currently even though that patch works for making base fields multilingual, it cannot make the UI match it (most properties still say "all languages"), because the form elements used for those base fields are in custom nested/named structures, so pairing up form elements with the base fields is not trivial. We can make up our own matching there, but if they become widgets, there is less of a point (although it all comes down to timeframes :/).

Wim Leers’s picture

#10: interesting! That means both this issue and the issue you link to need to be fixed before we can have in-place editing of node author & date!

yched’s picture

Gábor Hojtsy’s picture

Nono, #2111887: Regression: Only title (of base fields) on nodes is marked as translatable enables translation configuration for author and date, but it is in no way required to make them use widgets and formatters. The two are not related. The UI for displaying them as translated or applying to all languages becomes *way* easier and cleaner once they are widgets, since then we don't need to match up base field names with arbitrary nested/names form elements.

Gábor Hojtsy’s picture

In other words, *this* issue would help resolve the UI concerns in #2111887: Regression: Only title (of base fields) on nodes is marked as translatable so probably the other way around (although we can work around this there, but it will/would be ugly).

effulgentsia’s picture

Hm. Actually, #2111887-25: Regression: Only title (of base fields) on nodes is marked as translatable brings up an interesting snag. Currently, the form element for the node.uid field is $form['author']['name'].

1) For widgets, we typically follow a 1:1 mapping of field name to (root) element name, so if we want to continue with that, this would become $form['author']['uid']. Are there any objections to doing that rename?

2) We don't have Fieldgroup in core, so we either need to add it, or implement the grouping into the 'author' element in some custom way. Thoughts?

@Gabor: If we accept doing the rename, can #2111887: Regression: Only title (of base fields) on nodes is marked as translatable move forward by doing that rename there, without waiting on the widget work here? Or does the fieldgroup issue also need to get figured out for that issue to proceed?

yched’s picture

"This would become $form['uid']", you mean ?

Not sure of the impact of the fieldgroup thing (is that related to translatability ?), but field groups in core sounds a bit out of reach :-)

We could:
- Either make "author" not configurable in Field UI - the widget is uses is hardcoded in the field definition
- Or keep it configurable in Field UI, but reordering it has no effect, the form element gets form_altered into the fieldset ?

effulgentsia’s picture

"This would become $form['uid']", you mean ?

Whether we add Fieldgroup to core (which I agree is a long shot, unless it's possible to strip it down to something very minimal and allow the contrib module to add the bells and whistles not needed by any core use cases), or do either of #16's alternate options, the end result will still be that it's $form['author']['uid'] after all the alters have run. So the key question of #15.1 is whether there's any reason to not rename the element from HEAD's $form['author']['name'] to what a widget would do by default: $form['uid'] which some other TBD code would alter to $form['author']['uid']?

effulgentsia’s picture

Issue tags: +beta target

Given that this will involve changing the field types of some base fields, and changing form element names in commonly altered forms, I strongly suggest trying to get it done before beta. If we get close to beta without this being close, we may need to split out a patch containing name and type changes to get in by beta, and allow the rest to happen after, but hopefully, we can just get the whole thing done by beta and not have to worry about that :)

Gábor Hojtsy’s picture

Priority: Major » Critical

"If this blocks the beta it by definition blocks the release" (@catch via #2047633-115: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit). Or is there a semantic difference between "beta target" and "beta blocker"?

Gábor Hojtsy’s picture

As for the form element names, I think widgets work with top level keys on the form based on field names, no? So if at least the custom form elements are named according to the base fields (but are nested under other elements), so long as any nested element that is named like a field can be considered a representation of the field in the form (and the same key name is not reused on *any* level in the form for other things), that would be enough for [#8215017], however I think the widget solution would eventually make them appear on the root of the form, no?

effulgentsia’s picture

Priority: Critical » Major

There is a semantic difference between "beta target" and "beta blocker". The latter will block beta, which does also mean, block release, and therefore, implies critical. "beta target" means there are very compelling reasons to get into a beta, and if that fails, the issue needs to be reevaluated as to whether or not it needs to be postponed to 9.x.

Possibly, this issue should be "critical" and "beta blocker" on its own merits, but I'm not aware of a D8 committer approving that. Adjust the attributes accordingly if you believe it's appropriate.

effulgentsia’s picture

however I think the widget solution would eventually make them appear on the root of the form, no?

Not necessarily. The part of the code that invokes the widgets to add their elements does result in those items being initially put at the root of the form, but modules like Field group end up running follow up code (an alter hook, I think, though maybe in D8, it will get integrated into the rendering pipeline some other way than an alter hook) that can move the elements into whatever organization is needed. If we do this issue without relying on Field group (per #16), then there will be code in NodeFormController or similar to do that grouping in a hard-coded way.

so long as any nested element that is named like a field can be considered a representation of the field in the form (and the same key name is not reused on *any* level in the form for other things), that would be enough for [#8215017]

Yes. That is reasonably safe to assume. We don't actually require widgets to comply with that (perhaps we should, but that's a separate issue), but WidgetBase does comply with it, and so long as no one argues against #15.1, we should comply with it for all core widgets. Therefore, see #2111887-28: Regression: Only title (of base fields) on nodes is marked as translatable.

effulgentsia’s picture

Yes. That is reasonably safe to assume.

Oops. I should point out one caveat: if an entity form is inlined into a bigger form (e.g., a form that lets you edit multiple entities at once), then the uniqueness is only guaranteed within the section of the form that corresponds to a single entity. There are variables around to assist with determining that, but I don't recall them off the top of my head.

yched’s picture

We don't actually require widgets to comply with that [nesting elements in an entry named after the field], but WidgetBase does comply with it,

Right, currently it's not enforced, it's just the base, usually non overriden, default.
Actually, I've been thinking a couple times that this wrapping shouldn't be the job of the widgets / formatters , but should be left to the caller code.
Widgets / formatters would be more flexible to use standalone (no forced wrapper to unwrap), and calling code could then enforce whatever it sees fit.
Just never found the time :-/.

But sure, separate issue - I opened #2151693: Widgets / formatters should return unwrapped $elements

andypost’s picture

andypost’s picture

RumpledElf’s picture

I'm going to follow this thread because I'm interested in this and DrupalSouth made me want to contribute. I'll pop into next week's core mentoring.

Berdir’s picture

Status: Active » Needs review
FileSize
14.25 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 62,353 pass(es), 473 fail(s), and 92 exception(s). View

Worked a bit on this, so far, we only have widgets for string/text field types.

This will obviously break tons of tests that submit forms as the field names change.

Status: Needs review » Needs work

The last submitted patch, 28: widgets-and-formatters-2010930-28.patch, failed testing.

andypost’s picture

Wim Leers’s picture

Title: Apply formatters and widgets to rendered entity base fields (all besides node.title) » [META] Apply formatters and widgets to rendered entity base fields (all besides node.title)
Issue summary: View changes

Newly created child issues:

Next up:

  1. Making sure that base field types other than string/text are updated as well.
  2. Fixing this for other entity types also, such as Node.
Wim Leers’s picture

Issue summary: View changes

Fixed all base fields' formatters & widgets for the Node entity type, whose new widgets & formatters we can apply to other entity types as well.

yched’s picture

@Wim Leers+++++++++++++++++++++++++++++++++

Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Wim Leers’s picture

Issue summary: View changes
Berdir’s picture

Issue summary: View changes
Berdir’s picture

Issue summary: View changes
xjm’s picture

Status: Needs work » Active

Meta, so active. :)

xjm’s picture

The summary is a little confusing. We're not making them configurable fields -- right? Just converting the custom code for rendering the elements to widgets and formatters?

Also, is it really true that base fields aren't translatable? I thought there was lots of work to make that possible.

yched’s picture

Issue summary: View changes

@xjm : yeah, it seems most of those "apply formatters / widgets to [entity_type] base fields" were created with a shaky phrasing about "making base fields configurable" :-). Some of them have been corrected, but not all of them it seems.

So yes, those tasks really are about making existing base fields in existing entity types move from one-off ad-hoc render / form API elements to widgets and formatters - and not even necessarily in a "configurable way": in some cases the widget / formatter will just be hardcoded and not changeable in "Manage display" screens.

Adjusted the summary above.

yched’s picture

+ Adjusted the summaries of the child issues that are still open to simply refer to this one.

Wim Leers’s picture

#2226493: Apply formatters and widgets to Node base fields finally landed :)

Now let's get the last few done!

effulgentsia’s picture

Completing this issue might be the best way to resolve #2002180: Entity forms skip validation of fields that are edited without widgets, so adding it as a related issue, but see #2002180-45: Entity forms skip validation of fields that are edited without widgets for why I'm not yet promoting this issue to critical.

mgifford’s picture

Assigned: Wim Leers » Unassigned

Just unassigning issues that haven't been developed for a bit in the D8 queue.

xjm’s picture

Category: Task » Plan
Wim Leers’s picture

Issue tags: -sprint, -beta target

Thank you for reviving this @xjm :)

(Removing tags that are now irrelevant.)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

heddn’s picture

Issue summary: View changes

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.