Split from #2200355: Move functions in field.form.inc into Core/Field
The role of field.module in D8 is supposed to be "support configurable fields".
The notions of field types, widgets, formatters, are used by "base fields" as well, and have already moved to Core/Field.
The supporting code for output (theme functions, CSS) still lives in field.module. It 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.
Proposed plan :
- field_theme() -> merge 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 to system/css and be added in system_page_build() ?
Comment | File | Size | Author |
---|---|---|---|
#21 | move_field_theming-2201693-21.patch | 1.13 KB | plopesc |
Comments
Comment #1
yched CreditAttribution: yched commentedre: field.module.css : Should that be declared and added to the page as a CSS-only "library" now ?
Comment #2
sunTechnically yes. — But given this:
I'd say that those styles should be moved into system.module.css + system.theme.css instead, because there is no point in loading an additional CSS file on every page. :-)
Comment #3
plopescFirst approach.
About the CSS, I moved the lines related to field forms to
system.module.css
and lines related to field presentation and labels tosystem.theme.css
. Not sure if this is the best approach.Regards
Comment #4
yched CreditAttribution: yched commentedThanks @plopesc!
Looks good to me - @sun, what say you ? :-)
Comment #5
sunThanks @plopesc!
Now that I see the actual CSS, all of those styles are theme styles, so they belong into
system.theme.css
, and we can copy them 1:1.Make sure to move the existing CSS literally 1:1 — your new CSS in this patch is using an indentation of 4 spaces instead of 2 and the CSSDoc comment headers are not correct. ;)
Comment #6
plopescFixing CSS issues in #3
Comment #7
andypostnot sure how this file related to
theme_field()
, the template still lives in field modulemaybe file follow-up to make this more consistent?
Comment #8
yched CreditAttribution: yched commented@andypost #7:
1. "the template still lives in field module"
Hmm, no it doesn't ?
2. Not clear what consistency issue you mean, but yeah, out of scope here. This is just moving code around without changing any functionnality.
I'll leave @sun set to RTBC.
Comment #9
sunThanks, @plopesc!
Comment #10
alexpottmove_field_theming-2201693-6.patch no longer applies.
Comment #11
swentel CreditAttribution: swentel commentedRerolled
Comment #12
catchIt's not great having system module implementing the theme suggestions hook. There's no other place to do that?
Comment #13
sunThe only other way I could see would be to move those "default" suggestions directly into the corresponding function that collects the suggestions (i.e., in /core/includes/theme.inc or ThemeHandler)
That might indeed be better than system.module.
Comment #14
plopescSystem module currently implements
hook_theme_suggestions_HOOK()
for html and page to provide their theme_hook_suggestions.In that case, should this implementations be moved also to theme.inc or ThemeHandler?
Maybe we could left this issue as it is now and then decide which is the best place for these implementations and move all at once.
Anyway, attaching patch re-rolled, it currently does not apply.
Moreover, while we are here, could we consider add changes proposed in #2160065: Automatically add theme hook suggestions based on view mode for fields??
Regards
Comment #15
plopescRe-rolling
Comment #16
e0ipsoRe-rolling again with @plopesc.
Comment #17
yched CreditAttribution: yched commentedThanks @e0ipso !
Back to RTBC.
Comment #18
alexpottCommitted 13242b5 and pushed to 8.x. Thanks!
Don't think this needs a change notice since the theme names have not changed - just the code has moved.
Removed unused uses during commit.
Comment #20
rodrigoaguileraI was getting 404s because of references to the old files
Comment #21
plopescHello,
Thank you @rodrigoaguilera for your review!
The problem was that implementations of
hook_page_build()
andhook_theme_suggestions_field()
should have been removed in the previous patch.Those methods were re-added in one of the patch iteration, sorry :(
Comment #22
swentel CreditAttribution: swentel commentedugh, right.
Comment #24
swentel CreditAttribution: swentel commented21: move_field_theming-2201693-21.patch queued for re-testing.
Comment #25
swentel CreditAttribution: swentel commentedBot error - hopefully
Comment #26
andypostMarked as duplicate #2226019: The not exist field.module.css is added to the page
Comment #27
SweetchuckThe
clearfix
class is all the time added to the wrapper DIV (I do not known where) no matter what is the label position, above, inline or hidden.The problem is that the
clearfix
class is duplicated when the label position is "inline".Comment #28
SweetchuckComment #30
alexpott27: move_field_theming-2201693-27.patch queued for re-testing.
Comment #31
star-szrThe latest patch looks suspect to me because it removes the hook_theme_suggestions_HOOK() implementation.
The patch from #16 unfortunately undid a bunch of the work from #1987398: field.module - Convert theme_ functions to Twig, here is a follow-up to clean that up: #2226233: Redo changes from field.module to Twig conversion issue that were clobbered
Comment #32
plopesc@Cottser Sorry for the inconveniences :(
I removed the
hook_theme_suggestions_HOOK()
implementation in #21 because it is duplicated. It was moved form field module to system module.Regards.
Comment #33
star-szrThanks for clarifying @plopesc, that makes sense!
I'm not sure about the additional
CSSmarkup change made in #27, from my perspective #21 looks closer to what we need to close out this issue and I'm unable to reproduce the issue described with inline fields. @Sweetchuck can you please open a separate issue if you're able to reproduce and describe this behaviour? Thanks!Re-RTBC'ing #21.
Comment #34
alexpottCommitted patch in #21 c40d117 and pushed to 8.x. Thanks!