Edit module assumes that everything processed in hook_process_field is a real field. That assumption is incorrect. Display Suite for example adds pseudo fields that can't be inline edited and those fields aren't real fields either.
To prevent problems with display suite and other contrib modules that play with pseudo fields edit module could insert some extra checks.
My patch does this, but I'm not sure it's 100% correct as the title of the node can't be found in Field::fieldInfo()->getFieldMap();
That method only checks for config fields. And apparently their isn't any way to get the other valid fields...
Comment | File | Size | Author |
---|---|---|---|
#30 | 2057973-30.patch | 3.29 KB | Wim Leers |
#30 | interdiff.txt | 2.62 KB | Wim Leers |
#29 | 2057973-29-pass.patch | 2.91 KB | aspilicious |
#27 | 2057973-27-fail.patch | 2.29 KB | aspilicious |
#27 | 2057973-27-pass.patch | 3.15 KB | aspilicious |
Comments
Comment #1
swentel CreditAttribution: swentel commentedRelated, and probably more relevant in the end: #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title
Comment #2
Wim LeersPinging some folks to chime in on this.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedWhat's an example of a DS pseudo field, why is it themed via theme('field'), and what error is caused by it having the data attribute?
Comment #4
aspilicious CreditAttribution: aspilicious commentedWith ds you can insert any kind of content written in code as a field and it will behave as a field and will be themed as a field.
To make this possible we set the #field_name to the name of the plugin that generates the code for the fake field.
That field_name will be used to build the data attribute. When loading the page edit module (if I'm correct) gathers all kinda meta data. It uses the field name specified in the data attribute to do this. As DS fields are no real fields, the metadata can't be fetched for that field.
When that happens, a js error occurs. The quick edit contextual link won't be added to the node. That way you can't quick edit the "real" fields of the node for no good reason. (and a js error is bad anyway)
Clear?
Swentel correct me if I'm saying something stupid.
Comment #5
aspilicious CreditAttribution: aspilicious commentedAnd as those fields (most of them) are stored in code and not in the db it's impossible to (inline) edit them in the UI so it's normal they won't have the inline edit tools. But the presence of such a field shouldn't destroy the inline edit capabilities of others fields on the screen.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedThanks. #4 helps. Also, we want to support computed fields, which similarly, can't be edited. So I agree we want some kind of check, not entirely sure what exactly we want to check though. Maybe as simple as that it has an entity field definition and the 'computed' key isn't TRUE.
Comment #7
Wim Leersaspilicious: are you sure that
Entity::getPropertyDefinitions()
won't help you? IIRC it lists node title.Comment #8
aspilicious CreditAttribution: aspilicious commentedOk I tried your suggestion.
It works but I have some concerns as this list also contains properties as "sticky", "changed", ...
Comment #9
Wim Leers#8: true, but if you prefix all DS fields with something like
ds_
ords:
, then there is no danger, is there?effulgentsia, what do you think?
Comment #10
aspilicious CreditAttribution: aspilicious commentedThats true, it's no problem for display suite, just want to deliver a good patch and not some semi good solution.
Comment #11
effulgentsia CreditAttribution: effulgentsia commentedI think #8 makes sense. If "sticky" and "changed" are ever rendered to the page via theme('field'), then it makes sense for them to be in-place editable as well (or if it doesn't, then we'll need to figure out why not and encode that info somehow in the definition or the formatter/widget setup). Only improvements I can think of are:
- You can call getPropertyDefinition($field_name) instead of the plural. It returns FALSE when passed a $field_name that doesn't correspond to a defined field/property.
- Per #6, we may also want to have the
if
statement checkempty($definition['computed'])
in order to exclude computed fields from in-place editing. Unless any of you think that needs more discussion, in which case we could punt that to a separate issue.Comment #12
aspilicious CreditAttribution: aspilicious commentedI'll make a patch :)
Comment #13
aspilicious CreditAttribution: aspilicious commentedLet's try this...
Comment #14
swentel CreditAttribution: swentel commentedThis should be 'definition'.
First one is wrong too.
Comment #15
Wim LeersRTBC.
This reroll only fixes the typo in #14, which shouldn't preclude me from RTBC'ing.
Comment #16
Wim LeersD'oh :( Forgot to save. Sorry.
Comment #17
Wim LeersNow d.o is messing things up…
Comment #18
Wim Leers.
Comment #19
webchickHm. Seems like we could use both tests to codify this behaviour, as well as a quick comment to explain what that code is checking for. I would never have gotten "pseudofield" out of it at a glance.
Comment #20
aspilicious CreditAttribution: aspilicious commentedCan we test js errors?
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedWe don't have to. We can create a test module that invokes theme('field') on something that's not a field, and then test the markup that's returned.
Comment #22
aspilicious CreditAttribution: aspilicious commentedI don't have the time to write a test for this soon but if you want to mimic ds, you can add the theme('field) to hook_entity_view_alter().
Comment #23
Wim LeersSince this is blocked on tests (and hence blocked on aspilicious), removing "sprint" tag.
This definitely will get done and needs to get done, but is not a high priority.
Comment #24
Wim Leersaspilicious, any idea when you'll have time to write test coverage for this?
Comment #25
aspilicious CreditAttribution: aspilicious commentedTomorrow at the sprint?
I'll assign it to me... I think there will be enough people helping me if I need it :p
Comment #26
Wim LeersAwesome — thanks! :)
Comment #27
aspilicious CreditAttribution: aspilicious commentedI'm not that experienced with tests so I hope I this is ok? :)
Comment #28
Wim LeersThanks! Almost there! :)
edit_test should only be enabled for the one specific test in which it is necessary; we don't want it to potentially interfere with other tests. Not now, not in the future.
Tests that Edit doesn't set a data- attribute on pseudo fields or computed fields.
I'd remove the "Edit" here.
Missing trailing period, but comment is unnecessary.
Missing trailing period. data- attribute, not metadata.
Comment #29
aspilicious CreditAttribution: aspilicious commentedBetter?
Comment #30
Wim LeersThere were still quite a few small problems, I've now fixed them for you. That's easier & faster than pointing them out.
Comment #31
webchickCommitted and pushed to 8.x. Thanks!
Comment #32
Wim Leers.
Comment #34
yched CreditAttribution: yched commentedPing from #2214241-327: Field default markup - removing the divitis :
The quickedit_test_entity_view_alter() that was added here adds a "fake" field render array, which breaks when template_preprocess_field() needs to access the field definition.
Feedback welcome over there :-)
Comment #35
yched CreditAttribution: yched commentedOpened #2550225: quickedit_test_entity_view_alter() creates a non compliant field render array.
#2214241: Field default markup - removing the divitis can probably be unblocked with a temporary workaround check in template_preprocess_field(), but it seems unreasonable to expect any custom field preprocess / template that needs to access the field definition to do the same :-)