Don't add Edit module's data- attributes on pseudo fields (as used e.g. by Display Suite)

Related, and probably more relevant in the end: #1988612: Apply formatters and widgets to rendered entity base fields, starting with node.title

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?

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.

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

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.

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

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.

D'oh :( Forgot to save. Sorry.

Now d.o is messing things up…

.

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.

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.

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?

Awesome — thanks! :)

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.

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

Committed and pushed to 8.x. Thanks!

.

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

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