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...

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Wim Leers’s picture

Title: Edit attributes are added on any field » Don't add Edit module's data- attributes on pseudo fields (as used e.g. by Display Suite)
Category: bug » task
Issue tags: +sprint, +Spark

Pinging some folks to chime in on this.

effulgentsia’s picture

What'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?

aspilicious’s picture

With 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.

aspilicious’s picture

And 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.

effulgentsia’s picture

Thanks. #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.

Wim Leers’s picture

aspilicious: are you sure that Entity::getPropertyDefinitions() won't help you? IIRC it lists node title.

aspilicious’s picture

FileSize
880 bytes

Ok I tried your suggestion.
It works but I have some concerns as this list also contains properties as "sticky", "changed", ...

Wim Leers’s picture

#8: true, but if you prefix all DS fields with something like ds_ or ds:, then there is no danger, is there?

effulgentsia, what do you think?

aspilicious’s picture

Thats true, it's no problem for display suite, just want to deliver a good patch and not some semi good solution.

effulgentsia’s picture

I 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 check empty($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.

aspilicious’s picture

I'll make a patch :)

aspilicious’s picture

FileSize
882 bytes

Let's try this...

swentel’s picture

Status: Needs review » Needs work
@@ -164,7 +164,11 @@ function edit_field_formatter_info_alter(&$info) {
+  $defintion = $entity->getPropertyDefinition($element['#field_name']);

This should be 'definition'.

@@ -164,7 +164,11 @@ function edit_field_formatter_info_alter(&$info) {
+  if ($defintion && empty($definition['computed'])) {

First one is wrong too.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
882 bytes
  1. This follows effulgentsia's guidance in #11 to the letter.
  2. In-place editing still works — confirmed through manual testing :)
  3. The change is tiny and trivial.

RTBC.

This reroll only fixes the typo in #14, which shouldn't preclude me from RTBC'ing.

Wim Leers’s picture

FileSize
0 bytes

D'oh :( Forgot to save. Sorry.

Wim Leers’s picture

FileSize
883 bytes

Now d.o is messing things up…

Wim Leers’s picture

Issue tags: +quickfix

.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Hm. 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.

aspilicious’s picture

Can we test js errors?

effulgentsia’s picture

We 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.

aspilicious’s picture

I 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().

Wim Leers’s picture

Issue tags: -sprint

Since 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.

Wim Leers’s picture

aspilicious, any idea when you'll have time to write test coverage for this?

aspilicious’s picture

Assigned: Unassigned » aspilicious

Tomorrow at the sprint?

I'll assign it to me... I think there will be enough people helping me if I need it :p

Wim Leers’s picture

Awesome — thanks! :)

aspilicious’s picture

Status: Needs work » Needs review
FileSize
3.15 KB
2.29 KB

I'm not that experienced with tests so I hope I this is ok? :)

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests

Thanks! Almost there! :)

  1. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -22,7 +22,7 @@ class EditLoadingTest extends WebTestBase {
    -  public static $modules = array('contextual', 'edit', 'filter', 'node');
    +  public static $modules = array('contextual', 'edit', 'filter', 'node', 'edit_test');
    

    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.

  2. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -291,4 +291,16 @@ function testUserWithPermission() {
    +   * Tests that edit module doesn't add metadata for pseudo fields.
    

    Tests that Edit doesn't set a data- attribute on pseudo fields or computed fields.

  3. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -291,4 +291,16 @@ function testUserWithPermission() {
    +  function testEditPseudoFields() {
    

    I'd remove the "Edit" here.

  4. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -291,4 +291,16 @@ function testUserWithPermission() {
    +    // Login author
    

    Missing trailing period, but comment is unnecessary.

  5. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -291,4 +291,16 @@ function testUserWithPermission() {
    +    // Check that the metadata is not added
    

    Missing trailing period. data- attribute, not metadata.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
2.91 KB

Better?

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +sprint
FileSize
2.62 KB
3.29 KB

There were still quite a few small problems, I've now fixed them for you. That's easier & faster than pointing them out.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Wim Leers’s picture

Issue tags: -sprint

.

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

yched’s picture

Issue summary: View changes
Issue tags: -

Ping 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 :-)

yched’s picture

Opened #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 :-)