Part of #1949932: [META] Unify entity fields and field API and followup to #1950632-42: Create a FieldDefinitionInterface and use it for formatters and widgets:
In both cases (formatters / widgets), the rendering / form building flow for entities need to be reworked so that formatters / widgets are invoked on "base fields" as well. Right now, they are only invoked for "configurable fields", within field_attach_[prepare_view|view]() / field_attach[form|extract_form_values|form_validate]() respectively.
This issue is the start of that.
Patch summary
Rendering pipeline:
- Patch changes field_invoke_method*() so that they iterate on all fields (base + configurable) of a entity, instead of just configurable fields so far. Those functions, along with field_attach_form*() / field_attach_view*(), are up for removal / replacement by OO versions in #2095195: Remove deprecated field_attach_form_*(), but this patch here keeps them in field.module for now.
- As a result, widgets/formatters get invoked on any (base or config) field for which the EntityDisplay used for rendering contains the configuration of a widget/formatter. Free support for "In place editing" ensues.
- For now, Field UI "Manage display" screens still only list configurable fields, so EntityDisplay objects cannot be saved with widgets/formatters for base fields, thus all of them, except node title, are still rendered with their own custom code, as in HEAD. #2144919: Allow widgets and formatters for base fields to be configured in Field UI has been opened to allow Field UI integration.
- Patch moves the drupal_alter() step from the callers of entity_get_render_display() / entity_get_render_form_display() into the functions themselves, because per below, now that we have a use case of an alter implementation, we need to ensure that it always runs.
For node title:
- Patch removes the custom $form['title'] element
- Uses hook_entity_display_alter() to runtime-alter the EntityDisplay before rendering a node, adding hardcoded widget/formatter display options for the 'title' component
- Updates all WebTests posting node forms accordingly ($edit['title'] -> $edit['title[0][value]'])
- Since node title is now themed as a field, the patch adds a theme_field__node__title() to override theme_field()'s generic wrappers with a lighter-weight
<span>
wrapper. Prior to this patch, the node title was output (in node listings) with no wrapper of its own (only the<h2>
and<a>
output by node.html.twig), so this results in a new<span>
tag where there wasn't one before, but that's needed to hold the attributes required for in-place editing to work.
Comment | File | Size | Author |
---|---|---|---|
#208 | core-js-edit-funkyselector-1988612-208.patch | 777 bytes | nod_ |
#201 | interdiff.txt | 906 bytes | effulgentsia |
#201 | formatters_widgets-base_fields-1988612-201.patch | 112.07 KB | effulgentsia |
#199 | interdiff.txt | 3.5 KB | effulgentsia |
#199 | formatters_widgets-base_fields-1988612-199.patch | 111.7 KB | effulgentsia |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedThis patch is rebased on top of #1950632-44: Create a FieldDefinitionInterface and use it for formatters and widgets. It's still quite ugly, but demonstrates the beginnings of integrating formatters with base fields.
Comment #2
effulgentsia CreditAttribution: effulgentsia commentedNote that one of the intentional features of this is that being rendered with a formatter automatically means going through theme('field'), which is one (but not the only) necessary part of integrating with Edit module.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedObviously, we'd rather set this to 'datetime'. But that requires other changes to how node.created is stored / registered with EntityNG.
Also, the extra 'field_definition' parent key should hopefully be made unnecessary once #1969728: Implement Field API "field types" as TypedData Plugins is solved, since that's all about unifying the concepts of data type and field type.
Comment #4
Wim LeersThat's not the entire truth.
The point is that edit module needs *some* way of setting a data- attribute on each in-place editable field. Currently that happens via
theme_field()
, but it doesn't really matter. As long as it happens. Ideally of course, nonconfigurable and configurable fields are treated the same, can be handled the same way via a single API.It feels strange that non-storage metadata is being added to the entity's storage controller, no? But maybe that's what you're pointing out with #3, that this is non-ideal and will go away once #1969728: Implement Field API "field types" as TypedData Plugins lands?
Comment #5
yched CreditAttribution: yched commentedI take this as part of the "proof of concept", just as way quick way to get a "field definition" from a base field for now.
I'm not completely sure how #1969728: Implement Field API "field types" as TypedData Plugins will be able to unify that. In the scenario over there, a "field type" is mostly a class extending FieldItem. In order to build such an object, you need an actual entity, meaning an actual bundle.
And #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets shows that we need to generate "field definitions" in situations when we have no bundle (no $instance).
For example, #1969728: Implement Field API "field types" as TypedData Plugins turns hook_field_schema($field) into a *static* schema($field) method on the ConfigurableFieldItem class.
We'll need to generate it from base field definitions too, and, right, the "how" is not fully decided yet.
Comment #6
Wim LeersExpanding scope. This issue should get rid of custom form API code for the node date entirely.
Currently blocked on #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets.
Comment #7
Wim LeersComment #8
Wim LeersOnce this is done, we should work on #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title).
Comment #9
effulgentsia CreditAttribution: effulgentsia commentedDiscussed this at DrupalCon with yched and swentel. The approach we came up with is to make EntityDisplay responsible for then entire entity_view() render array. An important step in that direction is being done in #1875974: Abstract 'component type' specific code out of EntityDisplay. Once that and #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets are both in, I'll reroll a patch for this issue that uses that.
Comment #10
Wim Leers#1950632: Create a FieldDefinitionInterface and use it for formatters and widgets got committed, We can now start working on this! I should be able to work on this in the next few days, unless somebody beats me to it.
Comment #11
yched CreditAttribution: yched commentedNow that #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets is in, the other part of "make widgets and formatters work on all fields" would be to have them receive EntityNG Field objects, instead of currently plain $items arrays, in their form(), extractFormValues(), prepareView(), view()... methods.
Once #1969728: Implement Field API "field types" as TypedData Plugins is done, widgets and formatters are going to be the only thing that work with the old style arrays.
Aside from API consistency, avoiding that conversion / format change would possibly be a performance win - especially since :
- all those methods are called through a single set of helpers : field_invoke_method() / field_invoke_method_multiple() (they currently work only on Field API fields, that would be changed by making the EntityDisplay responsible for rendering)
- some of those methods (Widget::extractFormValues() & Formatter::prepareView()) do some writing to the $items, and thus the invoke helpers currently do the job of assigning the array values back into the FieldItem objects - for *all* methods, since they don't carry any knowledge of which methods write to the values or not.
It shouldn't be a very complex task in itself, but doing it on all formatters and widgets affects a lot of code...
It should be possible to be smart and temporarily allow both a BC and NG version of each method, with the invoke helpers being smart and calling the right one based on a method_exists('[method]BC') check, thus allowing the "official API" to be ready for code freeze, and leaving the deprecated versions still working and converted one by one. That approach is probably needed anyway in order for a patch to be just workable without having to convert everything as a first step.
Now the awkward part - I'm going afk in two days now and won't be around to work on this before code freeze :-/.
I don't want to sound like slacking off and assigning work, but ... @wim, do you think you could help on that ? ;-)
Comment #12
Wim Leers@yched Okay, so it seems that #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets alone was not sufficient to allow this issue to be applied? Or you're saying that it is technically possible, but it'll be hackish/complex, and once we do what you say in #11, it'll become trivial and clean?
In any case: yes, I can definitely help with that! If possible, it'd be great if you could convert one part of the code, then along with the description in #11, I should be able to drive it home. Just having read #11, I have *some* idea of what needs to happen, but I just don't know Field API well enough to say that a light bulb went on and I know exactly what needs to happen. As with #1950632, I can probably get myself up to speed, but that takes considerably more time than if there's an example to start from :)
I presume/hope that effulgentsia already knows *exactly* what you mean, in which case I could also just talk to him to get this done.
You're in the best position to judge if #11 is enough to get me started :)
Comment #13
yched CreditAttribution: yched commented@Wim:
It should still be technically possible (formatters and widgets currently work, even though they accept field values in a format that's different than the one in which EntityNG holds them), and so #11 should not delay the work on "make EntityDisplay responsible for the entire entity_view() render array and #1875974: Abstract 'component type' specific code out of EntityDisplay" that @eff mentions in #9.
But this dance between "old (array) format for field values handed to widgets / formatters" and "new (FieldItem objects) native format for field values" is very far from ideal performance wise, and it possibly only works right now because there's an EntityBC layer that we switch the $entity to before calling the field_invoke_method() / field_invoke_method_multiple() helpers that bridge to the widgets & formatters. But that BC layer is intended to go away at some point...
Thanks a lot for accepting to have a try at this! I'll try to whip up an initial patch for directions tomorrow.
Comment #14
yched CreditAttribution: yched commented@Wim (and / or anyone willing to help ;-)
I opened #2021817: Make widgets / formatters work on EntityNG Field value objects. with an initial patch and additional explanations.
Comment #15
effulgentsia CreditAttribution: effulgentsia commentedA work in progress, but wanting a bot check. Also, decided to switch to node.title instead of node.created as the first use case. So far, this only does the formatter part. Still need to do the widget part.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedThere's some stuff here that will need a bit of cleanup, but let's see if bot likes it more.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedComment #20
effulgentsia CreditAttribution: effulgentsia commentedComment #22
effulgentsia CreditAttribution: effulgentsia commentedComment #24
effulgentsia CreditAttribution: effulgentsia commentedThis will hopefully be green. Next step, widgets. Also, this contains a bunch of code that just adds integration between #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets and #1969728: Implement Field API "field types" as TypedData Plugins, since those issues were done in parallel. I may split that out into a separate issue to unclutter it from this one.
Comment #25
andypostreally interesting... inheritance?
Would be needed for comment-field form settings
is not it needs injection?
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedThis implements the widget side. All the BC decoration is quite ugly. Looking forward to when that's removed. I still need to respond to #25, but first, let's see what bot thinks of this.
Comment #28
effulgentsia CreditAttribution: effulgentsia commentedUgh. So many of our tests hardcode assumptions about input element names. Would be nice to centralize to some helper functions that in the future could be changed in one place. Not doing that yet though. Meanwhile, this fixes some of them.
Comment #29
effulgentsia CreditAttribution: effulgentsia commented#28 went in the wrong direction. Instead of fixing every test, we can instead make a custom widget that conforms to test expectations. That's what this patch does.
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedThis includes the start of making in-place editing work on node title.
@Wim Leers: When I'm on the home page viewing node teasers, I can go into "Quick edit" mode, and see via Firebug Net tab that metadata is being sent back for both title and body, but there's only an outline around body, and not on title. Can you debug that?
Comment #33
effulgentsia CreditAttribution: effulgentsia commentedFixed the 3 test failures. Still need to get in-place editing working, fill in the missing docs, update the issue summary, and respond to #25. More reviews also welcome at this point for anyone willing to do so without an updated issue summary.
Comment #34
Wim LeersFixed #31.
Bugs
Found while testing:
node/1
), where the node title is explicitly not rendered as part of the node, but lifted into the page title. Fixing that could be a follow-up though.en
, but it should beund
, like for the other entity fields.Review
How do you plan to fix this? I guess we need some way of detecting if a field is nonconfigurable, and if so, solely the entity access check should be performed.
Do you plan to make this part of
FieldDefinition
?Yay!
Is this temporary? If not, then I don't understand it.
Comment #35
effulgentsia CreditAttribution: effulgentsia commented#34.1 fixed and now in-place editing of node titles (when viewing teasers, see #34.2) works!!
Re the access check removals in #34, yes, that still needs fixing. Just wanted to see things working first.
Comment #36
Wim LeersYay :D
Comment #37
PanchoStraight reroll of #35 which didn't apply anymore after #1754246: Languages should be configuration entities landed.
Comment #38
PanchoStraight reroll is not quite enough after #1754246: Languages should be configuration entities. Small fix should do it though.
Confirm that in-place editing works again.
Still no widget settings in "Manage Form Display", and from "Manage Fields" and "Manage Display", the Title field is still completely removed (wasn't it at least listed some days ago?). Will be next step probably.
Comment #39
PanchoNice that this part works again.
However, overall this still needs work.
Comment #40
fagohm, this really makes field definition a totally different beast than the data definition. That's a bit weird as right now it's the same except for entity field definitions supporting some additional keys. So why not just mimic that and make a DataDefinition class + a FieldDefinition class that extends it? Then the FieldDefinition class could directly implement the FieldDefinitionInterface...
Couldn't that be just the same flow? e.g. go with a regular field-invoke-multiple? Probably that needs more refactoring though, so maybe that's better topic of a follow-up.
Comment #41
effulgentsia CreditAttribution: effulgentsia commentedJust a reroll.
Comment #42
effulgentsia CreditAttribution: effulgentsia commentedThanks for all the reviews so far. This issue is going to take longer than the next 12 or so hours (when API freeze kicks in) to complete. So I spun off the API changes being introduced here into #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed which I think is straightforward enough to be able to land quickly. Please review that. Then, I believe the rest of the work here is eligible for commit post API feeze.
Comment #44
Wim LeersRerolling #41 now that #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed landed. Quite the nontrivial reroll! This is a straight reroll of #41, minus the changes committed as part of #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed, plus rebased on latest HEAD.
Comment #46
Wim Leers#44 was the wrong patch. Sorry.
Comment #47
Wim LeersVarious things were broken in #44/#46 when you actually tried to use it. In this reroll, I got everything to work again: the node form, the node view (front page and full node view), and in-place editing of the title. Various things were completely broken.
I also fixed Edit's access checking to only do entity access checking for nonconfigurable fields/base fields/properties (whatever the right name might be right now, the current code base uses both!) and do both entity and field access checking for configurable fields.
Comment #49
Wim Leers#47 has the same failures as #41, as expected, but with a much smaller patch because #2032393: Limit APIs to FieldDefinitionInterface when FieldInstance isn't needed got committed. I hope that effulgentsia can take it from here.
Comment #50
fagoI opened #2047229: Make use of classes for entity field and data definitions for that - please provide your thoughts about it there.
Comment #51
yched CreditAttribution: yched commentedNote: opened #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system
Comment #52
webchickAs far as Edit module is concerned, this is major. It blocks being able to edit node titles, authors, and other non-primary fields.
Comment #53
Wim LeersHere is a straight reroll of #47, but things are utterly broken again. Not yet able to even use the
node/add
form…Comment #54
Wim LeersSeems a pretty big change was introduced in #2021817: Make widgets / formatters work on EntityNG Field value objects., it was even spun off from this issue, specifically from #11, but never reported back here. So the patch on that issue got committed :)
This is also indirectly the cause for the node form no longer working.
Comment #56
effulgentsia CreditAttribution: effulgentsia commentedSome interim progress.
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedNode form works now. And data attribute is output for the title, but I can't select it when using Quick Edit.
Comment #59
jessebeach CreditAttribution: jessebeach commentedQuick editing a node title with the patch in #58 worked for me. Here's proof: http://www.youtube.com/watch?v=Jbxozsn4eto&feature=youtube_gdata_player
Comment #60
yched CreditAttribution: yched commentedThanks @effulgentsia.
Hm, interesting / puzzling. Why is this needed ?
Why do we need to keep a copy of $this->instances within the FieldItem object ? FieldInfo::getBundleInstances() is statically cached already (and refreshed when needed), so duplicating the info sounds more like trouble with stale stuff for no real gain ?
On a side note, I'd really want to get rid of getInstance(). It prevents using "configurable field types" on base fields, which de facto means two separate sets of field types for base fields and configurable fields, which is wrong (shameless plug to #2052135: Determine how to deal with field types used in base fields in core entities).
Opened #2066507: remove ConfigField/ConfigFieldItem::getInstance() for this.
Yeah, so, this does worry me :-). Am I correct in understanding that this is the equivalent of field_view_field() for base fields ?
The widget / formatter plugin managers seem like a weird place for this.
field_view_field() / field_view_value() suck anyway - I've been considering replacing / deprecating those with a unified view() method on FieldInterface / FieldItemInterface(). Would seem like a better fit ?
Comment #62
effulgentsia CreditAttribution: effulgentsia commentedJust addressing test failures, not #60 yet.
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedChasing HEAD.
Comment #66
Wim LeersGreat progress effulgentsia! :)
The remaining failures in
\Drupal\node\Tests\NodeValidationTest
are legitimate. Node title validation doesn't work correctly anymore: a too long title no longer triggers a violation, nor does a non-existing title. So either theNode::set('title', 'something')
API for setting the title no longer works, or retrieving the new title is what fails.I started debugging this, but given I have zero knowledge about
\Symfony\Component\Validator\Validator
, my time is probably better spent elsewhere. Hope this helps a tiny bit.Comment #67
effulgentsia CreditAttribution: effulgentsia commentedThis fixes NodeValidationTest. Hopefully doesn't introduce some other bug.
Comment #68
tim.plunkettThis should be
'%name: the text may not be longer than @max characters.'
with appropriate replacements, otherwise it will get picked up for translation as a new string.Comment #70
effulgentsia CreditAttribution: effulgentsia commentedRe #68, this patch just removes the t() from tests, since that shouldn't be there in the first place. The rest of this interdiff ain't pretty, and I'll move it into its own issue about us having various problems related to the way we integrate validation of required fields. Here for now to see if it makes bot happy.
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedRemoving a stray hunk that was only there to assist with testing.
Comment #74
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #75
yched CreditAttribution: yched commentedOuch, not pretty indeed.
I *think* changing FieldInstanceEditForm::getFieldItem() this way would work:
That results in injecting a modified $instance in the FieldItem as its fieldDefinition. This is a trick that was needed during field purge to be able to construct an $items object using a specific injected $instance. I hope at some point it will be easier to inject a FieldDefinitionInterface object into $items / $item objects, but right now it's all we have...
(then the code that currently does this job in FieldInstanceEditForm::getDefaultValueWidget() can be removed)
[edit: sorry, scratch comment about t(), didn't see you fixed it]
Comment #77
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #79
effulgentsia CreditAttribution: effulgentsia commentedTrying to outdo the ugliness from #74 with even more ugliness. Ok, so this should be green, but now I'm on to creating a separate issue to deal with all this mess related to validating required fields.
Comment #80
effulgentsia CreditAttribution: effulgentsia commentedHere's the spin-off issue: #2070429: Configurable fields aren't validated against the "required" constraint except in forms
Comment #81
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll.
Comment #82
effulgentsia CreditAttribution: effulgentsia commentedReverting the stuff that I spun off to #2070429: Configurable fields aren't validated against the "required" constraint except in forms. I'm going to follow up with a different approach for this issue. That other one still needs to be fixed on its own merits, but I don't want this one held up on it.
Comment #83
effulgentsia CreditAttribution: effulgentsia commentedJust catching up with #2050801: Unify handling of default values between base and configurable fields
Comment #84
effulgentsia CreditAttribution: effulgentsia commentedAnd, the much simpler and more correct solution to #66. I feel pretty silly now for going down the rabbit hole of the last 15 comments.
Comment #86
effulgentsia CreditAttribution: effulgentsia commentedSigh. Missed a hunk that should have been part of #82.
Comment #87
effulgentsia CreditAttribution: effulgentsia commentedOk, so I think the next steps here are:
- get #2047229: Make use of classes for entity field and data definitions in, which will remove some of what's in this patch with equivalent code that's been more thoroughly reviewed
- get #1994140: Unify entity field access and Field API access in, which will remove some of the hacks related to access checks
- address #60
- see what else is left here that's questionable
I think those can all be done in parallel. Keeping this assigned to me; I'll post more when there's an update.
Comment #88
Berdir#86: formatters_widgets-base_fields-1988612-86.patch queued for re-testing.
Comment #90
effulgentsia CreditAttribution: effulgentsia commentedReroll.
Comment #92
effulgentsia CreditAttribution: effulgentsia commentedNodeFormController now has a buildEntity() implementation in HEAD, so this merges the one added by the patch into it.
Comment #94
BerdirThat's because filter_format is now a thing provided by filter.module so that needs to be enabled in those DUBT tests.
However, this also indicates that the title is now a string field with a format. And that seems wrong?
Comment #95
effulgentsia CreditAttribution: effulgentsia commentedThanks. That explains 2 of the 3 failures. Since filter.info.yml says it's a required module, enabling it in those tests seems legitimate.
The 3rd failure is due to Symfony's ChoiceValidator::validate() throwing an exception if $constraint->choices is an empty array *before* checking for $value being null. That makes no sense. If null is allowed, then $constraint->choices being empty should not be an error. Rather than fixing Symfony, this patch just fixes it in the AllowedValuesConstraintValidator subclass.
Because text.module doesn't provide a field type that's just 'value' without 'format'. Maybe it should? But, since format is allowed to be NULL when the 'text_processing' setting is FALSE, and since filter.module is a required module, I don't think it's a problem for $node->title to support a NULL $node->title->format. If we ever decide to have text.module provide a value-only field type, then we can switch $node->title to use it.
Comment #96
jibranYay!! it is green. Let's fix all the @todo's.
Comment #97
effulgentsia CreditAttribution: effulgentsia commentedReroll, which also removes the offending code of #60.2. Next steps are to get #1994140: Unify entity field access and Field API access in and address #60.1 and #60.3.
Comment #98
effulgentsia CreditAttribution: effulgentsia commentedRegardless what bot says, this still needs work per #97.
Comment #99
effulgentsia CreditAttribution: effulgentsia commented#97: formatters_widgets-base_fields-1988612-97.patch queued for re-testing.
Comment #100
yched CreditAttribution: yched commentedDoes #75 still apply ? (avoid "required" validation errors on the widget in the "default value" UI)
Comment #101
effulgentsia CreditAttribution: effulgentsia commentedDecoupled from this issue. See #82 and #84.
Comment #102
yched CreditAttribution: yched commented@effulgentsia: OK, makes sense.
Do you want eyes on the current patch, or is it still in your yard for now ?
Comment #103
effulgentsia CreditAttribution: effulgentsia commentedI think it makes sense to at least have some variant of #2047229-17: Make use of classes for entity field and data definitions committed, and this patch rerolled without it, before further eyes on the rest of the patch are useful. #1994140: Unify entity field access and Field API access would also be nice, but not as necessary, though I wonder if that might be the quickest way to get the first one in.
Comment #104
yched CreditAttribution: yched commentedOpened #2095195: Remove deprecated field_attach_form_*() for the 1st step
Comment #105
BerdirHere's a re-roll. That was fun, really nice to see how HEAD improved since Sept 5th :)
Let's see how well this works, might have removed a few things that I shouldn't.
Comment #107
yched CreditAttribution: yched commented#2095195: Remove deprecated field_attach_form_*() contains an initial version of how the integration of widgets / formatters in forms / entity_views would work after #2019055: Switch from field-level language fallback to entity-level language fallback gets in. Still only works on configurable fields, but In the end, that same code should work on all fields (base, config) that are known by the EntityFormDisplay.
I'll be afk for the next two weeks, so I posted what I had over there.
Comment #108
BerdirImproving viewBaseField().
Comment #110
BerdirFixed editing too.
Comment #112
BerdirToo much interface. Something about validation seems to be broken for real, though.
Comment #114
BerdirI still think this change is wrong.
field_item:text means we also get a format in the structe and more importantly, it instantiates a TextProcessed instance for the computed process property that we'll never use. We already have a custom widget for the title, I'd rather add a formatter for string_field too. Also saves us the list_class definition.
I guess this is causing the test fail there. Wrong merge or was that explicitly removed?
Comment #115
effulgentsia CreditAttribution: effulgentsia commentedIn theory, this is just a straight reroll, but I did need to manually fix some conflicts. Let's start by seeing if bot finds anything horribly broken.
Comment #117
effulgentsia CreditAttribution: effulgentsia commented#115: formatters_widgets-base_fields-1988612-115.patch queued for re-testing.
Comment #119
effulgentsia CreditAttribution: effulgentsia commentedComment #121
effulgentsia CreditAttribution: effulgentsia commentedLet's see if bot likes this more. If so, I'll split these unit test changes into a separate issue.
Comment #123
effulgentsia CreditAttribution: effulgentsia commentedJust getting bot to pass again. Still need to address #60.3 and #114.1.
Comment #124
BerdirWe might want to wait with #114 until #2050835: Move Widget, Formatter, and FieldType plugin types to the Core\Field system is in, I'd be fine with doing that in a follow-up issue if this would be ready otherwise.
Comment #125
effulgentsia CreditAttribution: effulgentsia commentedHaving to write mock code like this in unit tests (#121) made me feel icky, so I opened #2112759: Add ContentEntityInterface::hasField().
Comment #126
effulgentsia CreditAttribution: effulgentsia commentedThis removes that.
Comment #128
effulgentsia CreditAttribution: effulgentsia commentedA whole bunch more appending of [0][value]. Wow, we have a lot of tests that submit node forms.
Comment #130
BerdirThe check doesn't seem to make much sense? If there's a field then there has to be a field definition. So if the field doesn't exist, this should throw an exception on get() and never get to the next method call?
Sounds like another use case for hasField() ?
What about moving this logic into the entity render controller/view builder and add a viewField() in there? Then we can hide the configurable/base logic from the outside, as long as it still exist and can refactor it within there?
This one I guess is more problematic, as we don't really have an easy way to do this.
Comment #131
effulgentsia CreditAttribution: effulgentsia commentedStill hunting down all the places that need [0][value] appended. Will address #130 after we're green again.
Comment #132
effulgentsia CreditAttribution: effulgentsia commentedComment #134
effulgentsia CreditAttribution: effulgentsia commentedComment #136
effulgentsia CreditAttribution: effulgentsia commentedThis fixes one of the failing tests, but the one that's left is real: we need to somehow allow base fields to have per-bundle definitions (similar to instances for configurable fields, but not that exactly), since node types have a configurable title label. I remember fago starting on that in Prague, but I don't know the current status of that.
Comment #137
BerdirYes, we need that, there's a @todo for it in #2047229: Make use of classes for entity field and data definitions, no issue yet for it, we should open one. The conclusions of the prague discussion are in https://docs.google.com/document/d/1A5BLd-KmrLnJW88SdLX5HG3Xm7AyinxwXNAG..., with example code to implement it, we just need to get the other issue in first, that one is big enough on it's own I think.
Comment #139
xjmComment #140
effulgentsia CreditAttribution: effulgentsia commentedRe #137, ok, I opened #2114707: Allow per-bundle overrides of field definitions and added a hack here with a @todo linking to it.
I still need to address #130 and other feedback, but let's see if we're back to green now.
Comment #141
effulgentsia CreditAttribution: effulgentsia commentedA few easy cleanups, including #130.1. Not using hasField() though until #2112759: Add ContentEntityInterface::hasField() lands.
Comment #143
effulgentsia CreditAttribution: effulgentsia commentedOops.
Comment #144
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll for HEAD changes. No actual changes.
Comment #145
effulgentsia CreditAttribution: effulgentsia commented#2112759: Add ContentEntityInterface::hasField() landed, so this incorporates that and simplifies the unit test.
Comment #146
effulgentsia CreditAttribution: effulgentsia commentedI'm in progress on the other feedback that's been raised in earlier comments.
Comment #147
effulgentsia CreditAttribution: effulgentsia commentedAlrighty. I think this incorporates all the remaining feedback (#60.3 and #130.2 and .3).
Comment #149
effulgentsia CreditAttribution: effulgentsia commentedAnd rerolled for HEAD.
Comment #151
effulgentsia CreditAttribution: effulgentsia commentedComment #153
BerdirSounds like you have some old namespaces, search for "\Core\Entity\Field\" and replace with "\Core\Field\" :)
Comment #154
effulgentsia CreditAttribution: effulgentsia commentedComment #156
effulgentsia CreditAttribution: effulgentsia commentedThis one should be green. I'm unassigning myself pending reviews. I do still need to fill in the docs where there's a @todo for docs, and I need to file followup issues for the other @todos, but other than that, is there anything else that's needed?
Comment #157
fagowow, great to see this working. Looking through the patch it seems reasonable first step, it would be good to get an overview of what it does already and what the follow-ups left are (yaml!).
Comment #157.0
fagoshortened issue summary
Comment #158
Wim LeersBerdir, fago, any chance you could review this?
Comment #159
effulgentsia CreditAttribution: effulgentsia commentedReroll.
Comment #161
effulgentsia CreditAttribution: effulgentsia commentedComment #163
effulgentsia CreditAttribution: effulgentsia commentedThis should be green again.
Comment #164
effulgentsia CreditAttribution: effulgentsia commentedThis moves theme_field__title() from field.module to theme_field__node__title() in node.module, and documents it, resolving one @todo.
Comment #165
effulgentsia CreditAttribution: effulgentsia commentedNow that #2019055: Switch from field-level language fallback to entity-level language fallback is in, this resolves one language related todo, and links the other one to the appropriate followup.
Comment #166
yched CreditAttribution: yched commentedReviewing - sorry, I'm behind on a lot of reviews, slowly catching up...
As a side note:
- Now that #2019055: Switch from field-level language fallback to entity-level language fallback is in, we can move forward on #2095195: Remove deprecated field_attach_form_*(). The goal is to replace field_attach_view() / field_attach_form(), that currently iterate only on configurable fields, with iterator methods on entity view / form controllers, that can iterate on all (base & configurable) fields of an entity indifferently. It should simplify a lot of the code in this patch, that currently has to go through hoops to execute widgets/formatters on base fields ?
- #2090509: Optimize entity_get_render_display() for the case of "multiple entity view" moves entity_get_render_display() to a public method in EntityViewBuilder, that includes the hook_entity_display_alter invocation. This should let us revert some of the changes here.
Comment #167
effulgentsia CreditAttribution: effulgentsia commentedThis resolves another todo. I think that's the last one that's in scope for this issue. The rest will be follow ups. I'll update the issue summary and add links to those follow ups shortly.
Comment #168
effulgentsia CreditAttribution: effulgentsia commentedRe #166, thanks for those links, but please tell me we don't need to hold this issue up on them :)
Comment #169
yched CreditAttribution: yched commented@effulgentsia: Definitely no need to hold this up on #2090509: Optimize entity_get_render_display() for the case of "multiple entity view", that's a detail.
#2095195: Remove deprecated field_attach_form_*() is open for debate ;-)
I have no real opinion yet, I'm still in the middle of the review. For now, I was just pointing that this other issue should settle the "final" "clean" "D8 style" version of the "iterate widgets / formatters on Entity Fields" code, and is thus closely related.
I guess it's a matter of how intrusive the patch is, and to what extent the changes made here to have this work on top of the "old" code spill over a lot of different places that will be hard to revert. Looks like it should be ok - and sure, I'd very much favor getting this in first and polishing in that other issue. At least that's the angle of my review :-)
Comment #170
yched CreditAttribution: yched commentedPartial review so far, sorry, posting what I have.
Looks like a sign something else is going wrong somewhere ?
I'll leave it to @Wim to vet this ;-)
Why this change ?
This means the method can still return NULL, which would result in EntityDisplay::getRenderer() calling Widget/FormatterPluginManager->getInstance(array('field_definition' => NULL)), which will break ?
So it looks like EntityDisplay::getRenderer() should check if ($field_definition = $this->getFieldDefinition()) before calling pluginManager->getInstance() ?
That new $options['display'] param for field_invoke_method() is not documented - but I'm not too fond of the idea.
The logic about "only iterate on fields that are actually visible in a given EntityDisplay" should already be taken care of by the $target_function closure passed to field_invoke_method().
The role of $target_function($field_definition) is "for this field, give me the object (e.g the widget plugin) on which to call the method, or NULL if none"
The functions used as $target_function, like _field_invoke_widget_target(), use $display->getRenderer(), which takes care of "NULL if the field is hidden".
So there should be no need of making field_invoke_method() aware of the notion of "EntityDisplay", this logic is abstracted and injected in the $target_function parameter already ?
Comment #171
Wim Leers#170.2: That is correct, I made that change :)
Comment #172
yched CreditAttribution: yched commentedRegarding my last remark in #170 - $options['display'] in field_invoke_method():
With the various Field API cleanups and refactors that went in the past couple weeks, we can now simplify field_invoke_method*() a lot, and get rid of most "options": run on deleted fields, run on one single field specified by UUID, and most of all get rid of the old multilingual logic - the functions are now only ever called in a context where we run on one single language, which has already been resolved upstream in the received $entity.
It makes sense to do this as part of this issue : the old field_language*() logic is outdated after #2019055: Switch from field-level language fallback to entity-level language fallback, the functions are now mere BC layers that reproduce the old behavior for configurable fields only. This issue makes field_invoke_method() run on all fields, so it shouldn't rely on the config-only language logic anymore - especially since it's useless and slow as hell.
So I explored this a bit, and skimmed the functions down to what is strictly needed now. I removed the $options['display'] param as well - as explained in #170.5, the logic about "only invoke the method on fields that are visible in the EntityDisplay" is already taken care of by the $target_function closures.
This works pretty well and simplifies code a lot, but uncovered some additional fails.Turns out, with the previous patches, ContentEntityFormController::validate() was actually not passing $options['display'] when calling field_invoke_method('flagErrors'), so the function was running in its "old" mode (only on configuration fields), and validation errors were not reported for base fields.
With this new patch, validation errors are reported for every field (base or config) present in the FormDisplay, causing issues with validation errors on empty 'title':
- The NotNull violation is at the ItemList level, i.e not on a specific Item/delta, and the code in WidgetBase::flagErrors() choked with notices.
--> patch fixes that by flagging those errors with no $delta to $element instead of $element[$delta]
- Once this gets fixed, an "empty title" error is reported twice: first as a pure Form API '#required' fail, then by EntityValidation and the NotNull constraint
--> patch fixes that as was discussed in #2070429: Configurable fields aren't validated against the "required" constraint except in forms, by having WidgetBase::flagErrors() *not* report errors found on a field if the form already has errors on that same field. As mentioned in that issue, it would be best to not run validation at all on a field if there were Form API-level errors on it, but IMO that improvement can be left for that other issue to tackle.
- node/Tests/PagePreviewTest was drunk: It tries to preview and submit changes to a node iteratively, but the actual submits, with a partial $edit without a 'title', were being made to node/add instead of node/[nid]/edit. The submissions are actually failing in current HEAD, but nothing checks that.
Comment #173
yched CreditAttribution: yched commentedRelated : opened #2134887: move field_view_field() / field_view_value() to methods
Comment #174
yched CreditAttribution: yched commentedJust a reroll for now.
Comment #176
yched CreditAttribution: yched commentedWorking on this.
Reading through the patch, I stumbled on MetadataGeneratorInterface::generateField(), and posted suggestions at #2144879: Brush up MetadataGeneratorInterface::generate(Field|Entity)(): use FieldItemListInterface + better naming :-)
Comment #177
yched CreditAttribution: yched commentedThis is very close to being RTBCable IMO.
- I opened #2144919: Allow widgets and formatters for base fields to be configured in Field UI for the next step (allow actual configurability instead of the - smart - hack done here for node title)
- I'm still exploring a couple adjustments in a helper issue.
Meanwhile, a couple questions for @effulgentsia:
The changes in FormatterPluginManager/WidgetPluginManager::prepareConfiguration() do not seem to be able to rescue anything: if we're not able to come up with a 'type' of widget/formatter, then the rest of the code will fail anyway ?
Why this ?
Comment #178
effulgentsia CreditAttribution: effulgentsia commentedRe #177.1:
The issue is "extra fields" in entity displays that are the same name as a base field of a type for which there is no formatter/widget. For example, taxonomy term name. That overlap should go away with #2144919: Allow widgets and formatters for base fields to be configured in Field UI, but what to do in the meantime? What "fails anyway"?
Re #177.2:
Because theme_field__node__title() outputs a
span
wrapper in order to house the attributes needed by edit.module.Comment #179
effulgentsia CreditAttribution: effulgentsia commentedStraight reroll to remove the change to an Overlay test now that Overlay module is gone. #2047229: Make use of classes for entity field and data definitions landed, so I'd like to resolve the @todos referencing that, which I'll work on now, unless yched tells me not to :)
Comment #180
effulgentsia CreditAttribution: effulgentsia commentedResolved those todos, and made sure all the remaining ones link to a follow up issue.
Comment #181
effulgentsia CreditAttribution: effulgentsia commentedInterdiff between #179 and #180.
Comment #183
effulgentsia CreditAttribution: effulgentsia commentedComment #185
yched CreditAttribution: yched commentedRegarding the @todos on #2047229: Make use of classes for entity field and data definitions :
I'm not sure we can remove them yet, EntityManager->getFieldDefinitions($entity_type, $bundle) currently contains the $fields, not the $instances :-/. That's for #2114707: Allow per-bundle overrides of field definitions to solve, for now $items->getFieldDefinition() is the only way to get the $instance.
Actually, in my local patch, I kept the @todos but moved the link to 2114707. Sorry, I should have posted that here, but I'm still working on fails on other parts :-/
Comment #186
effulgentsia CreditAttribution: effulgentsia commentedI see. Ok, here's a reversion of that, and a complete interdiff from #174. Looking forward to your next patch once you resolve the stuff you're working on :)
Comment #190
yched CreditAttribution: yched commentedOk, sorry for the confusion.
Here's a patch built on top of @eff's latest #186, with a couple trivial changes:
- add a missing typehint
- update @todo issue URL
- fix a recently added test involving a node title in a form
Still experimenting a couple possible cleanups in a local branch, not included here for now.
Comment #193
yched CreditAttribution: yched commentedShould fix the fail ?
Comment #194
yched CreditAttribution: yched commentedOK, I think I'm done.
- Attached patch clarifies the handling for "fields" and "extra fields" in EntityDisplayBase, trying to keep a "you're one or the other but not both" line, which we very much need for #1875974: Abstract 'component type' specific code out of EntityDisplay.
- This also lets us revert the changes in Widget/FormatterPluginManager (we don't try to run Widget/FormatterPluginManager::prepareConfiguration() on "base field"-style display options)
This is ready to fly as far as I'm concerned - but obviously I can't push the RTBC button myself now...
Comment #195
yched CreditAttribution: yched commentedAdded a patch summary to the OP
Comment #196
yched CreditAttribution: yched commentedComment #197
yched CreditAttribution: yched commentedComment #198
Gábor HojtsyThese are the "choice quotes" from the patch. Basically instead of defining it as a string field and providing our own form / display, we provide it as a field.
Discussed this with @yched and @fago in detail in Vienna. Basically the main problem is we treat title as both an extra field (so it shows up on the list of fields to configure) and as a field on the entity display that we need to alter. This is not very nice, but best we can do with current tools.
#2114707: Allow per-bundle overrides of field definitions and #2144919: Allow widgets and formatters for base fields to be configured in Field UI as well as other issues mentioned in the patch will go a long way to clean this up. Given that this is an infrastructure issue to set us up to get widgets for base fields available, I'd vote to move forward with the current approach instead of jumping around more.
Ps. #2111887: Regression: Only title (of base fields) on nodes is marked as translatable also needs this infrastructure to have widgets for base fields proper to attach translatability information without needing to hack around in the form structure.
Comment #199
effulgentsia CreditAttribution: effulgentsia commentedThanks for the RTBC! I looked this over one more time in depth, and am happy with all of yched's changes as well.
Just trivial cleanup here, so leaving at RTBC.
Comment #200
fagoI agree with others that it's a good idea to move on with this now and work in the remaining issues as follow-ups.
I had a detailed look at the patch, in particular at the entity validation changes - those are good as well, great work!
The only minor thing (besides the commented interim hacks) I found:
Also the function has no @return, what could be ok as this is an override and has an @see on the original function. But if so, @param should go as well -> either add both, or none.
That's nothing what should hold up this important step though, and can be fixed in a separate issue.
Comment #201
effulgentsia CreditAttribution: effulgentsia commentedAdded the type to the @param of the new theme function and also to theme_field() to keep them consistent, but did not add the @return to either due to #2148429: Add @return to theme.inc functions.
Comment #202
effulgentsia CreditAttribution: effulgentsia commentedComment #203
effulgentsia CreditAttribution: effulgentsia commentedComment #204
fagoThanks - improvements are good! Thanks for opening #2148429: Add @return to theme.inc functions - I replied there .
Comment #205
catchOK there's a few @todos in here but they're all related to existing issues and/or need to be resolved when everything has been converted. Committed/pushed to 8.x, thanks!
Comment #206
Wim LeersOMG YAY!!!!! My sincere thanks to go to everybody who helped get this patch to RTBC, especially effulgentsia and yched! :)
Comment #207
Wim LeersNow that this is committed, some follow-up work:
Comment #208
nod_Tag and some refactor of the JS.
$el.is(':has(.field-item)')
? I think not :DComment #209
plachAwesome!
However, don't we need a change notice for this?
Comment #210
yched CreditAttribution: yched commentedI'll let @Wim chime on the JS followup :-)
@plach: the functionality will definitely deserve a change notice, but for now it's still relatively hidden and clunky to leverage. I think it would make more sense to write a change notice after the "right" API and UI have landed in #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title)
Comment #211
plachOk, works for me :)
Comment #212
Wim Leers#208: Let's please do that in #2148797: field_language() always NULL for non-configurable fields (=> in-place editing of node title fails) :)
Comment #213
effulgentsia CreditAttribution: effulgentsia commentedRe #208 / #212: The JS improvement is unrelated to #2148797: field_language() always NULL for non-configurable fields (=> in-place editing of node title fails), so I opened #2149933: Improve Edit module JS to use .find(...) instead of .is(':has(...)') for it.
Comment #215
xjmThe followup issue has now turned into a meta issue, and I just tried to reference the change record from this one there, except there wasn't one. :) So let's create the change record here, since this is a monumental improvement, and then we can add to it as needed.
Comment #216
yched CreditAttribution: yched commented@xjm: This issue here did the internal legwork of adapting the field rendering system to run on base fields too, but the actual API to specify widget and formatter options for your entity type's base fields was added by #2144919: Allow widgets and formatters for base fields to be configured in Field UI.
The change record was thus created for the latter : https://drupal.org/node/2186583
IIRC, this issue here doesn't need a change record of its own.
Comment #217
xjmI've updated https://drupal.org/node/2186583 to reference this issue and #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title). Thanks @yched!