The role of field.module in D8 is supposed to be "support configurable fields".
The notions of field types, widgets, formatters, are shared with "base fields", and have already moved to Core/Field

Widgets, though, still rely on functions in field.form.inc :.

field_form_get_state() / field_form_set_state() / field_form_element_after_build() :
Handle clustering of $form_state to support forms with several nested entities.
The D8 shape of those is probably related to what happens with #1728816: Ensure multiple entity forms can be embedded into one form.
See also thoughts in #2095195-43: Remove deprecated field_attach_form_*().

field_add_more_submit() / field_add_more_js() :
Handle the "add more" button.
[edit: taken care of in #2201709: Move support for "add more" button into WidgetBase]

Those don't belong in field.module anymore, and should move out if we want to be able to make it a "non required" module #2199637: Replace "required" flag of Field module with proper dependencies

CommentFileSizeAuthor
#16 2200355-remove_field.form_.inc-16.patch17.52 KByched
#13 interdiff.txt1.98 KByched
#13 2200355-remove_field.form_.inc-13.patch17.52 KByched
#10 interdiff.txt979 bytesyched
#10 2200355-remove_field.form_.inc-10.patch17.5 KByched
#9 2200355-9.patch17.64 KBandypost
#9 interdiff.txt1.41 KBandypost
#8 2200355-remove_field.form_.inc-8.patch17.13 KByched
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Place for theme_field_multiple_value_form() is interesting
Others could live in WidgetBase methods

yched’s picture

yched CreditAttribution: yched commented

re: theme_field_multiple_value_form() ;
On the display side, field.module's template_preprocess_field() / theme_field() / field.html.twig have the same flaw: they're now used for base fields and called from Core/Field, thus they all belong to system.module rather than field.module

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Small correction: System module is only a (needless) courier for registering the theme functions of the Drupal core base system.

Drupal core's base system theme functions live in includes/theme.inc and are defined by drupal_common_theme().

That's where the field theme functions should be moved into (unless we can get rid of them entirely, but let's leave that for a follow-up issue).

yched’s picture

yched CreditAttribution: yched commented
Issue summary: View changes

@sun : ok, so, if I get things right:

- field_theme() -> merged into drupal_common_theme() in theme.inc
- template_preprocess_field(), theme_field(), theme_field_multiple_value_form() -> theme.inc
- field.html.twig -> system/templates
- field_theme_suggestions_field() -> system.module

There's also field.module.css, that's added by field.module's hook_page_build(), would need to move into system_page_build() ?

That would be best in an issue of its own - I opened #2201693: Field output supporting code should move out of field.module.

yched’s picture

yched CreditAttribution: yched commented

+ patch for field_add_more_submit() / field_add_more_js() in #2201709: Move support for "add more" button into WidgetBase

yched’s picture

yched CreditAttribution: yched commented
Issue summary: View changes
yched’s picture

yched CreditAttribution: yched commented
Issue summary: View changes
yched’s picture

yched CreditAttribution: yched commented
Status: Active » Needs review
FileSize
2200355-remove_field.form_.inc-8.patch17.13 KB

Here's a patch.

field.form.inc still contained :

- _field_sort_items_value_helper() : helper for usort
--> moved to includes/theme.inc, that's where it's used, in template_preprocess_field_multiple_value_form()

- field_form_element_after_build() : FAPI #after_build callback
--> moved to a static method in WidgetBase, like the rest of the supporting FAPI callables used by WidgetBase

- field_form_get_state() / field_form_set_state() + supporting _field_form_state_parents()
--> moved to static methods in WidgetBaseInterface / WidgetBase (static because we need them to be callable from various supporting FAPI #callbacks, that are better off static themselves)

And one less .inc file in field.module ! The last remaining one is field.purge.inc, that should be removed by #2282119: Make the Entity Field API handle field purging.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
interdiff.txt1.41 KB
2200355-9.patch17.64 KB

Awesome! RTBC+1

Patch fixes minor nitpicks:
1) delete unneeded file
2)

+++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
@@ -89,7 +89,7 @@
-   * accessed by field_form_get_state() and field_form_set_state().
+   * accessed by $widget::getWidgetState() and setWidgetState().

looks like full namespace is needed here

yched’s picture

yched CreditAttribution: yched commented
FileSize
2200355-remove_field.form_.inc-10.patch17.5 KB
interdiff.txt979 bytes

1) Hm, dunno what happened when I rolled the patch, I did remove the field.form.inc file and the corresponding include locally, but it looks like it didn't make it to the patch I posted :-/.
Anyway - yeah, totally.

2) Yup, better indeed.

Additionnal doc fix - the current doc for field_form_get_state() was wrong, we don't place 'constraint_violations' in $form_state. We did at some point, but not anymore.

yched’s picture

yched CreditAttribution: yched commented

So, we need someone to RTBC this :-)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

Two quickies to get this to RTBC:

  1. +++ b/core/includes/theme.inc
@@ -2494,6 +2494,17 @@ function template_preprocess_field_multiple_value_form(&$variables) {
+ * Callback for usort() within field-multiple-value-form.html.twig.

    Shouldn't this refer to the template preprocess function? 

  2. +++ b/core/lib/Drupal/Core/Field/WidgetBase.php
@@ -429,6 +446,38 @@ public function flagErrors(FieldItemListInterface $items, ConstraintViolationLis
+  protected static function widgetStateParents(array $parents, $field_name) {

    Can we add a "get" prefix here?

    Functions without get/set/... prefix are supposed to "do" something.

yched’s picture

yched CreditAttribution: yched commented
FileSize
2200355-remove_field.form_.inc-13.patch17.52 KB
interdiff.txt1.98 KB

1) The current docs refer to the tpl, but sure, makes more sense I guess
2) Yup

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs review » Reviewed & tested by the community

Thanks!

yched’s picture

yched CreditAttribution: yched commented

Created draft change records :
https://www.drupal.org/node/2294431
https://www.drupal.org/node/2294435

yched’s picture

yched CreditAttribution: yched commented
FileSize
2200355-remove_field.form_.inc-16.patch17.52 KB

Straight reroll after #2293723: Generate lighter $form[$field] structures.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 5f9c6c3 and pushed to 8.x. Thanks!

  • alexpott committed 5f9c6c3 on 8.x 
    Issue #2200355 by yched, andypost: Move functions in field.form.inc into...
yched’s picture

yched CreditAttribution: yched commented

Thanks - I see you have published the change records.

Status: Fixed » Closed (fixed)

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