Add 'visible' key to hook_field_extra_fields()

Hm, why not, but right now it would only make sense for 'display' extra elements (as opposed to 'form' elements - Field UI doesn't let you hide elements in forms)

[edited out, I was on crack]

This looks reasonable.

Back to CNR for the testbot.

In the course of reviewing this, I found the patch no longer applied, probably due to other subsequent changes. Re-rolled.

Confirming #8 worked for me. Makes extra fields much more viable, as now they won't insert themselves on every node type they appear in without you setting them to visible.

Adding backport patch for 7.x

Resubmitting the patch with a d7 prefix. Thanks! I was wondering how testing was going to happen, given that this is an 8.x issue.

The field.info.inc code is nearly identical in both cases, so I see no reason why it wouldn't be backportable. If that can't be done in this issue, I'll open a feature request against 7.x and cross-reference this issue. I will admit that my primary motivation for working on this is to use it now, in our D7 sites, so I'm more interested in the backport.

So #13 is mistaken; this does not work, nor has it ever been the case that it worked. Please see http://drupal.org/node/1319154 for a description of how the backporting process works.

I am reattaching the d8 patch so that tests get run again against d8.

Also see http://drupal.org/node/332678 for more on automated patch testing.

Tagging contributed project blocker because i18n is unable to hide node language fields by default: #1350638: Language display should default to hidden when enabling "Multilingual content"

Backport would be possible since it's an API addition, which is allowed.

@iamEAP I'm not sure that this is a project blocker, just a huge pain in the ...! I can post the code we used to update one of our sites for two new extra fields if you are interested.

+1 for getting this in AND backporting, the update scripts to handle this would need to modify field_bundle_settings variable, which would mean that it is likely that contrib is likely to kill the display settings for all fields at least once or twice until this gets in...

As per #9.

+++ b/core/modules/field/field.api.phpundefined
@@ -31,6 +31,8 @@
+ *   - visible: Optional, Defaults to TRUE. The default visibility of the
+ *     display component. Not used for form components.

I think this should read

 - visible: (optional) The default visibility of the display component, defaults to TRUE. Not used for form components.

Obviously still needs to be wrapped at 80.

This is much needed. Currently if a new field gets added to a large site, it gets displayed everywhere and there's no sane way to disable it.

Speaking of #24, an almost-sane way to hide them for those who desperately need it now: hook_field_extra_fields_display_alter().

This looks fine but can we add tests for the new feature?

Yes this will need a testing module, it may well be possible to re-use the existing field_test module (or whatever it's called) and just add the hook implementation to it.

Looks great, thanks Niklas

Actually, two minor points.

-class ExtraFieldsTest extends WebTestBase {
+class FieldExtraFieldsTestCase extends FieldTestCase {

-      'name' => 'Extra fields tests',
+      'name' => 'Field extra fields tests',

Re-rolled patches against D7 for testing.

The tests missed the third option, that this doesn't negatively impact the existing API. This patch (against 7.x atm, but will reroll to 8.x) checks that if the visibility is not specified, then the field will show.

#29 is RTBC. Let's leave it that way, until committed.

K. I was going to quickly run some D7 tests then re-roll for D8 with minor text mods / additional tests. Accidentally deleted setUp() and it took a while to figure out what was going wrong. Out of time to re-roll to D8 :(

The D7 one adds all 3 use-cases, visibility specified as true, false, unspecified, and adds a test on another content type to ensure that the view modes only effect the specified bundle (there are no tests for anything related to extra fields afaict). Appending to assist backport to D7.

Well, if the D7 one has more assertions, they should be added to D8 before this goes in.

Only one additional assertion. I was testing the field_test module with 3 of the additional ones ;)

So, by default, all extra fields which do not not have the visible key will be displayed, right ? Isn't the intent of the issue (which also causes a lot of frustration for site builders) that they should be hidden by default if there's no specification ? I'd personally vote for hiding them by default to be honest, unless I'm reading the code and tests completely wrong of course (it's late here).

I have to agree with Niklas, anything else would be a change in the API and be unlikely to accepted. We should do our best to no hold this issue up (this 2 liner change is nearly a year old).

I am sure that contrib will quickly update their additional field definitions.

Summary, fix to this issue is a 2 liner.

The rest are tests, for this issue and fields via hook_node_view() successfully adding content to nodes that are displayed via the default visibility configuration.

I tried to reroll the patch from #37. This feature/regression was fixed in #1757504: Regression: language field is not visible on manage display. But there are no test for this feature.

This patch would be a very welcome addition.

Wouldn't it be sensible to also add label visibility as an option as well if we have visibility?

[EDIT] Sorry, just realised that extra fields don't have a label, but that's a whole separate issue.

#45: hook_field_extra_fields-visibility-test-1256368-45.patch queued for re-testing.

Rerolled patch from #45

Is this a feature or bug? Also there's a related issue #1954188: Revaluate the concept of 'extra fields'

The re-roll is missing the test.

Could use this as well in a 7.x project. I guess for 8.x it's a task to write tests for it as it seems to have been fixed already.

Coming here from #1614108: Document hook_field_extra_fields()'s "edit" and "delete" properties... I would just like to point out that back in June of 2012, fields *did* support visibility.

The function _field_info_prepare_extra_fields():
http://drupalcode.org/project/drupal.git/blob/e039aaaa32c5053a441330b403...
supported a 'visible' element for the 'display' mode, and this was used in field_ui_display_overview_form():
http://drupalcode.org/project/drupal.git/blob/e2bcdbcedb605325e4f0889428...

So if this is not present in the current code, it may be a regression? I could also be mistaken about whether the previous functionality is what is being requested here... I didn't read all 50 comments, was just responding to David Rothstein's comment here on that other issue:
http://drupal.org/node/1614108#comment-7299436

@jhodgdon
Needs work for tests. The core functionality was committed accidentally in another commit.

Duh. It has been lost.

class FieldInfo {
  public function prepareExtraFields($extra_fields, $entity_type, $bundle) {
    ....

    // Extra fields in displayed entities.
    $data = $extra_fields['display'];
    foreach ($extra_fields['display'] as $name => $field_data) {
      $settings = isset($bundle_settings['extra_fields']['display'][$name]) ? $bundle_settings['extra_fields']['display'][$name] : array();
      $view_modes = array_merge(array('default'), array_keys($entity_type_info['view modes']));
      foreach ($view_modes as $view_mode) {
        if (isset($settings[$view_mode])) {
          $field_data['display'][$view_mode] = $settings[$view_mode];
        }
        else {
          $field_data['display'][$view_mode] = array(
            'weight' => $field_data['weight'],
            'visible' => TRUE,
          );
        }
      }
      unset($field_data['weight']);
      $result['display'][$name] = $field_data;
    }

    return $result;
  }
}

[edit] I think, completely new to this class

@jhodgdon, @Alan D., I don't understand. Those code examples show that the default 'visible' setting is hardcoded to TRUE, don't they? But the point of this issue is to allow it to be provided by hook_field_extra_fields() (so that people defining new "extra fields" can start them off as 'visible' = FALSE if they want to).

That change was committed to Drupal 8, but I don't see any evidence that it was ever in Drupal 7.

Quick look at the D7 git history didn't show anything.

AFAICT, this never got to that point to backport, as this was never finalized for D8 due to lack of tests. And this lack of tests allowed the committed code to be reverted if I am reading the latest 8.x branch code correctly. So now neither branches appear to have this. :(

RE #55... See #52 (the links are to what was committed about a year ago in May). It looks to me as though if you pass in 'visible' => TRUE or 'visible' => FALSE in the version of _field_info_prepare_extra_fields() as of that date, then it is used/return, but if you don't set up a display mode, 'visible' is set to TRUE. Right? And that function is called from the collate function
http://drupalcode.org/project/drupal.git/blob/e039aaaa32c5053a441330b403...
which invokes hook_field_extra_fields() to get the information to pass to _field_info_prepare_extra_fields(). So I think as of about a year ago, this was possible to do with the hook. Right?

@Alan D., you might be right, I'm not seeing any evidence of it in Drupal 8 either right now. I guess the tests can prove that, one way or another...

@jhodgdon, what I'm seeing in that code from May 2012 (leaving out the surrounding code) is:

  foreach ($extra_fields['display'] as $name => $field_data) {
...
    if (isset($settings[$view_mode])) {
      $field_data['display'][$view_mode] = $settings[$view_mode];
    }
    else {
      $field_data['display'][$view_mode] = array(
        'weight' => $field_data['weight'],
        'visible' => TRUE,
      );
    }

$extra_fields contains the default settings passed in from the hook, but $settings comes from somewhere else (I believe that comes from the database, i.e. for existing fields it respects whatever was saved in the admin UI most recently, but for new fields it won't be there). And no matter what, the display settings on the $field_data variable are getting overwritten.

In the case of 'weight', they can be overridden based on the value that was provided in $extra_fields, but in the case of 'visible', they can't?

Duh, you are correct. I read that code several times and glossed over that.

So we should go back to that docs issue and remove 'visible' from the hook docs for both 7 and 8 currently then?

Tracked this down to #1852966: Rework entity display settings around EntityDisplay config entity.

Posted there to ask for some feedback here (without reopening, so hopefully someone will trace this here)

@jhodgdon: Hah, well, I've certainly been in that position before too :)

So I decided to check if it works in Drupal 8, and although I can't figure out where in the code is making it work, it does actually seem to. For example, node_field_extra_fields() sets 'visible' => FALSE on the Language form element, and it correctly shows up as hidden on the Manage Display screen. And if you remove that line of code and clear caches, it goes back to being un-hidden.

So I think for Drupal 8 the issue here just needs tests after all (only Drupal 7 needs the functionality)... I'll go back and change the docs issue accordingly also.

...coming from #2024011: Expose a display setting to hide new EVA fields by default

Posting a new D7 patch in case anyone needs it.

fire up the bot

Please do not post Drupal 7 patches on Drupal 8 issues until the 8.x issue is resolved.

I think it's OK to post a Drupal 7 patch but it's best to follow the instructions for how to do so that appear on the file upload field:

For patches that should not be tested by the testbot (for example, patches that do not work with the current API version in the issue), add "do-not-test" before the .patch. For example "drupal.do_nothing_555555-do-not-test.patch"

Although in this case the issue was already "needs work" so the testbot wasn't supposed to run on it anyway :)

Posting the Drupal 7 patch somewhere on drupal.org is also a requirement if anyone is going to use it in a Drupal distribution before it's committed (see https://drupal.org/node/1475972) and while it could be posted in another issue to reduce noise, I think it's better to keep it in one place?

apologies :)

Re-rolled for 7.23.

7.23 is passed and patch wasn't included. Do we have to open new issue for d7 and keep this for d8 so patch can be committed ?

Can someone please update the issue summary so we know what needs to be done for both Drupal 7 and Drupal 8?

If the patches are substantially similar for both, our policy is normally that bugs have to be fixed in 8 before fixes can be committed to 7.

If the patches/code is substantially different in 7, we might consider branching off to a separate issue. I'm just not sure what the plan is for 8 and 7 and the issue summary doesn't illuminate the issue at all.

Patch in #70 looks good to me for D7.

Comparing the following two (correct file for D8?)

• D8: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
• D7: http://drupalcode.org/project/drupal.git/blob/refs/heads/7.x:/modules/fi...

I'd say the changes in D8 are vast enough for this to be separated into a D7 and a D8 issue. Judging from #62 this now works for D8 but needs only testing.

Can we get the patch from #70 into D7 please? :)

OK, feel free to (a) create a separate D7 issue (or a separate D8 issue?) and (b) update the issue summary for both and (c) make sure the issues are linked to each other.

Split off the D8/D7 tests into their own issue.

OK, so this is now just a D7 issue, but there is still no real issue summary. And we do not commit patches to D7 or D8 without tests -- the proposed D7 patch has not even been run though the test bot, and it has no test for the proposed change.

It is fine to make a separate issue for D8 but it is not fine to say that making the test for new behavior is a separate issue. We need tests for reviewers to verify the new code is OK.

I understand your point about the necessity of tests. It's a bit difficult in this case because the functionality is, according to #62, already in D8 without having tests written for it.

Seeing as we will not backport the D8 functionality (because it's a complete rewrite?), I figured we could already get this into D7 while others work on the tests for both versions. Of course this is silly, because it could cause regressions.

I just cloned a copy of D8 to do some grep magic on and perhaps find a test or comment left or right that may point us in the right direction.

In all fairness: I did update the issue summary, just nothing ground-breaking. How would you like to see it updated?

...hiding all patches except the last one.

Found it in D8! :)

The extra fields now use EntityDisplay objects along with regular fields: https://drupal.org/node/1875952

So I did some digging and found the D8 functionality in Drupal\field_ui\DisplayOverviewBase::buildExtraFieldRow(). See the code (line 447).

So it could very well be that there are tests already for D8, but that they now are more generic tests for EntityDisplay objects.

(Cross-posting in related issue)

I guess the issue summary is good enough. We do have a template. :) This issue went through a lot of iterations about whether it was a bug, a docs bug, a feature request, etc. but the current summary does seem to reflect what we want to happen.

Anyway, we do need a test if we want to get this into D7.

And at this point, since it's not a purely documentation issue, I'm going to stop following it and let the maintainers of this sub-system and/or the D7 maintainers weigh in on whether they would accept such a feature request this late in the D7 game.

This works fine for me on 7.22, tests or no tests.

That conditional made me go cross-eyed, so I rewrote it a bit.

I might have time this week to look at a test...

I'm going to update a version of this that uses FALSE as the default just to see if we have any coverage at all.

So we do have lots of implicit test coverage.
We just need explicit tests that setting it to FALSE does work.

Besides the missing test I think this is RTBC :).

Subscribing, this is an important issue.

Ordermind - you don't need to post to subscribe to topics, you can just click the 'follow' (or subscribe?) button at the top of the thread, underneath the issue information.

Rerolled.

Aren't tests always missing?

Tests with FALSE by default drupal-n1256368-91-FALSE.patch will always fail since in their tests, modules assume extra field is displayed by default.

Re #98 I meant, see #85. We need PASSED tests when the default value is FALSE.

+++ b/modules/field/field.api.php
@@ -35,6 +35,8 @@
+ *   - visible: (optional, defaults to TRUE) The default visibility of the

Just a small note: this comment should obviously say FALSE if the default value is FALSE

I think Tim means that test will need to be added to the actual patch to ensure that this is actually covered, rather than generating a patch that fails :)

The code and reviews are rtbc, the doco is correct, this is just missing some test to check that:

a) The fields still show that are already define
b) Adding a new field with visibility set to FALSE does not show

While imho, FALSE default makes sense, that would be an massive API change in this area, so this needs to be TRUE by default. Which the second patch does (drupal-n1256368-91.patch).

#101.b is what I meant.
Leave the default as is, but we need a test showing that a declaration of FALSE does what we expect.

Yes, as a new feature this really should have tests, especially since it looks like they are already in Drupal 8 (at least to some extent); see discussion in #2165879: Write tests for 'visibility' of fields exposed with hook_entity_extra_field_info(). That could be the basis for some Drupal 7 tests. Or the basis could be many of the earlier patches in this issue, many of which look like they included tests in the patch, but then those got lost in later patches...?

Oops.

Until this patch is finished, I just wanted to leave this here for anyone who comes looking for an answer on how to hide an extra field by default. Below is a D7 snippet to hide newly created extra fields when they're defined for the first time. With it, you won't need to wait on this patch to be committed or patch core yourself.

<?php
/**
 * Utility function to hide any newly created extra_fields.
 *
 * @param $entity_type string
 *   Entity type on which the new extra field will reside.
 *
 * @param $bundle string
 *   Bundle on which the new extra field will reside.
 *
 * @param $field_name string
 *   Machine name of extra field.
 */
function mymodule_hide_new_extra_field($entity_type, $bundle, $field_name) {
  $settings = field_bundle_settings($entity_type, $bundle);
  $settings_updated = FALSE;
  $entity_info = entity_get_info($entity_type, $bundle);
  $view_modes = array_keys($entity_info['view modes']);
  if (!in_array('default', $view_modes)) $view_modes[] = 'default';
  foreach ($view_modes as $view_mode) {
    if (!isset($settings['extra_fields']['display'][$field_name][$view_mode])) {
      $settings_updated = TRUE;
      $settings['extra_fields']['display'][$field_name][$view_mode] = array(
        'weight' => 100,
        'visible' => FALSE,
      );
    }
  }
  if ($settings_updated) {
    field_bundle_settings($entity_type, $bundle, $settings);
  }
}
?>