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

Files: 
CommentFileSizeAuthor
#30 2057973-30.patch3.29 KBWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]
#30 interdiff.txt2.62 KBWim Leers
#29 2057973-29-pass.patch2.91 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 58,820 pass(es).
[ View ]
#27 2057973-27-fail.patch2.29 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 59,020 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#27 2057973-27-pass.patch3.15 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 58,662 pass(es).
[ View ]
#17 edit_ds-2057973-16.patch883 bytesWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,235 pass(es).
[ View ]
#16 edit_ds-2057973-15.patch0 bytesWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,221 pass(es).
[ View ]
#15 edit_ds-2057973-15.patch882 bytesWim Leers
PASSED: [[SimpleTest]]: [MySQL] 58,185 pass(es).
[ View ]
#13 2057973-13.patch882 bytesaspilicious
PASSED: [[SimpleTest]]: [MySQL] 58,167 pass(es).
[ View ]
#8 2057973-8.patch880 bytesaspilicious
PASSED: [[SimpleTest]]: [MySQL] 57,923 pass(es).
[ View ]
edit-check-fields.patch1.04 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 57,968 pass(es).
[ View ]

Comments

Title:Edit attributes are added on any fieldDon'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.

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?

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.

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.

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.

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

StatusFileSize
new880 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,923 pass(es).
[ View ]

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

#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?

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

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.

I'll make a patch :)

StatusFileSize
new882 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,167 pass(es).
[ View ]

Let's try this...

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new882 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,185 pass(es).
[ View ]
  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.

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,221 pass(es).
[ View ]

D'oh :( Forgot to save. Sorry.

StatusFileSize
new883 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,235 pass(es).
[ View ]

Now d.o is messing things up…

Issue tags:+quickfix

.

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.

Can we test js errors?

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.

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

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.

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

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

Awesome — thanks! :)

Status:Needs work» Needs review
StatusFileSize
new3.15 KB
PASSED: [[SimpleTest]]: [MySQL] 58,662 pass(es).
[ View ]
new2.29 KB
FAILED: [[SimpleTest]]: [MySQL] 59,020 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

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.

Status:Needs work» Needs review
StatusFileSize
new2.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,820 pass(es).
[ View ]

Better?

Status:Needs review» Reviewed & tested by the community
Issue tags:+sprint
StatusFileSize
new2.62 KB
new3.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,789 pass(es).
[ View ]

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

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Issue tags:-sprint

.

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