Clean install of HEAD as of Aug 17, 2010, STR:

go to ?q=admin/structure/types/manage/article/display

Expand "Custom display settings"

Select "Search index" and submit.

Navigate to ?q=admin/structure/types/manage/article/display/search_index

All fields are set to be "hidden", including (most critically) the node body - so the body is not added to the search index.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Defaults should probably be the same as when viewing a node in full page view, not all off...

pwolanin’s picture

This is so bad it's funny - the same happens if you enable "Custom display settings" for "Full content". Check that box, and then if you navigate to a node of type article, the body is hidden.

jhodgdon’s picture

So it looks like the problem is this:
The default settings for field display are fine - everything is shown.
But if you set up any "custom display settings", the default configuration is to have everything hidden.

jhodgdon’s picture

Trying to track this down...

Looking at function field_ui_display_overview_form() -- this is going through all the instances for the particular content type (bundle) and doing:

  foreach ($instances as $name => $instance) {
    $display = $instance['display'][$view_mode];

    ...
    $table[$name]['type'] = array(
      '#type' => 'select',
      '#options' => $formatter_options,
      '#default_value' => $display['type'],
...
     );

So apparently $instance['display']['a new view mode']['type'] is set to use the Hidden formatter by default.

That is presumably being set up in http://api.drupal.org/api/function/field_read_instances/7, though I don't see anything about display settings there...

Ah: http://api.drupal.org/api/function/_field_info_prepare_instance/7

   if (!isset($instance['display'][$view_mode])) {
      $instance['display'][$view_mode] = array(
        'type' => 'hidden',
        'label' => 'above',
        'settings' => array(),
        'weight' => 0,
      );
    }

That is probably it. Why that default was chosen, I'm not sure...

hefox’s picture

Looked into "Why that default was chosen"

http://drupal.org/node/553298#comment-2805046

- sets fields to 'hidden' if they have no explicit settings in a given build mode, instead of defaulting to the settings used for the 'full' mode, as per #715826: field display mode should default to hidden . No more surprises on your displayed nodes when enabling a random module that creates a new field.

pwolanin’s picture

Status: Active » Needs review
FileSize
925 bytes
pwolanin’s picture

Title: All node fields are hidden from the search index by default » All node fields are hidden from the search index (and other modes) by default
hefox’s picture

That patch is similiar to pwolanin, however, I don't think either is quite right.

There are two defaults to consider now, the defaults for field, which is what pwolanin's accounts for.

However, the manage field revamp has added this new 'custom_settings'. If a field does not use custom_settings, those values under display are not actually used; instead a different, 'fake'?, view mode ('default')'s settings are used. This is done in field_get_display and field_extra_fields_get_display. the current settings for the content type are ignored.

Attached patch sets unknown to the default, but that is basically equivalent to pwonlanin's atm due to the field settings getting updated a lot, keeping the values set in that foreach.

I think that is what we need to not set the default; only use the view mode settings when set and custom_settings is set, and use default when not. The second patch attempts that; note that due to updating of field info based on defaults set in _field_info_prepare_instance, need to create a new content type or such to test any patch tmk (it's not just cached, the field settings get saved -- perhaps when enabling custom settings!).

Seperate patches, 2 different approaches.

hefox’s picture

Status: Active » Needs review

huhu how did that happen

jhodgdon’s picture

Status: Needs review » Needs work

I like the approach of http://drupal.org/files/issues/886152_drupal_node_fields_hidden.patch -- it is clear that the default for "I don't know what this display mode is" is "use the default view mode". Which will correctly handle the case where you're adding a new view mode.

I don't think it correctly handles the case of adding a new field though (where the consensus appears to be that it should be hidden by default if not specified? So probably it needs some logic saying "If the default view mode is set up for this field instance, use the default view mode. Otherwise, set it to hidden."

yched’s picture

subscribe - short of time tonight, but please don't rush this in.

chx’s picture

Status: Needs work » Needs review
FileSize
1.15 KB

I am not sure whether I can offer anything coherent but I can give some insight on how we use display modes. So if you want to put out an entity somewhere on the screen, you need to use the loaded entity (so that entitycache can fire on it) but must of the time you do not need all the fields dangling off an entity when displaying in a block or something like that and the most clean approach to that is to add a display mode to it. This is way, way simpler than cobbling together field_view_field commands. Indeed the rule of thumb should be, if you need to add more than one field to a single render array in a function, you are better off with a display mode.

I wrote this up to show that display mode are numerous. And this kind of usage, which, I think, will be quite common, calls for display modes to default to hidden.

But this issue is about display modes that want to default to the default formatter of the instance.

The two are orthogonal and someone is going to be unhappy if we enforce either. We need to add a setting to entity info which tells whether to fall back to default or to fall back to hidden. Here is a failing test then a patch w the same test passing.

chx’s picture

FileSize
2.44 KB

The above patch hopefully has 1 fail. This hopefully has 0.

Status: Needs review » Needs work

The last submitted patch, defaults_to_hidden.patch, failed testing.

chx’s picture

Title: All node fields are hidden from the search index (and other modes) by default » All node fields are hidden from the search index (and other view modes) by default
Status: Needs work » Needs review
FileSize
3.84 KB

Turns out we had a default display mode that we have thrown away (?) but this patch reinstates it. Added documentation, noted that view mode is consistent in core ( i thought we also called it display modes? no. it's view mode) and so fixed it in the new test texts. (the previous patch, while fixing the issue has fallen back too much. this is hopefully better.)

sun’s picture

Looks good, but yched asked to not rush this in.

yched’s picture

Title: All node fields are hidden from the search index (and other view modes) by default » All fields hidden on view modes newly configured not to use 'default' display
Status: Needs review » Needs work

First off, the title is overly dramatic, fields are not "hidden in 'search' (and other modes) by default". By default, they are rendered in 'search' and other view modes just like they are on 'full' node page.

They are hidden in a view mode that was just newly configured to use dedicated display settings instead of the common 'default' display settings, until those custom display settings are actually defined.

That was the intended behavior back in #553298: Redesign the 'Manage Display' screen. All fields are hidden unless explicit display settings are provided, either programmatically in the $înstance definition, or through admin decisions in Field UI. The rationale is : do not surprise the site admin and have pieces of content suddenly popup in places he doesn't expect and break his layouts or content display policy.

As chx points, view modes are a highly valuable way to define custom 'display flavors' : either reusable 'node display variants' for the custom needs of your specific site (e.g : "xtra short teaser" for side blocks, "lengthy teaser" for your 'highlighted' area, whatever...), or for the specific display contexts added by a contrib module (e.g : 'print' when enabling http://drupal.org/project/print). View modes will florish in D7 - especially with the fact that, unlike in D6, each view mode has its own field order and grouping (fieldgroup).

- When you enable a module that adds its own fields in hook_install(), its $instance definition can specify display settings for a couple view modes it knows (probably 'full' and 'teaser'), but it most probably won't know about your custom view modes, or the ones added by a random contrib module.
We don't want that field to suddenly popup in all your view modes. Several days can pass before you notice it and configure all your view modes back.
- Similarly, when admins add a new field through Field UI, hiding it everywhere unless specified otherwise is what makes most sense.

The issue here is that, the moment you specialize 'search index' or 'rss' view modes to use their own specific display settings instead of the 'default' settings, your cron immediately starts indexing empty content for your nodes, and your live feeds start advertising empty items, until you actually go and configure the view mode. Agreed that this is critical.

In the case of 'newly specialized view modes', neither of the approaches in the patches above stand. The 'least surpising' behavior would be to initialize the view mode similarly to how it was displayed so far (with the settings for the 'default' mode). No change for end users while you fine tune your display settings for the view mode on its 'Manage display' screen and until you hit 'Submit' there.

Pondering on the best way to do that (views modes can be specialized through Field UI, like pwolanin described in the OP, or programmatically).

For the record, in D8 I want to stop scattering display settings in each field's $instance definition, and instead create $display objects, that hold the collection of display settings for all the components of an entity (fields and non-fields) for a given bundle in a given view mode.

chx’s picture

And why the patch I submitted does not fulfill these goals...? You can specify a view mode to use the default settings or you can specify it to use hidden and it seems this is what your writeup wants too...?

hefox’s picture

I think my second patch, 886152_drupal_node_fields_hidden_use_actual_mode.patch, was aiming at what #17 was talking about; trying to use the 'default' untill the screen is submitted.

Basically, as soon as a setting for a field is added, it's very likely to be saved via other screens, so the instance populating on read will easily get saved.

For example, why I disliked my first patch is that it essiently initalized to same as pwolain ; everything is default formatter for existing view modes cause the user hasn't had a chance to set the 'default' yet before the default is set!. So for example a user sets up their default, then enables customizing a view mode, the new view mode will come out as everything at default formatter, cause it's default were to be set a while ago then a save on a field elsewhere saved them.

Now if a new view mode was added, it'd be set to whatever the current default is (or with chx patch, hidden if view mode is such, but current right then default if set to default). However, user goes reworks the the default, and the new, not-customized-yet view, doesn't get the new settings, instead ends up whatever the /old/ default was (ie. whenever the view mode appeared on the scene).

(New fields get the default formatter/hidden depending on patch)

I think gotta tread likely if any default is set, or else may end up with a lot of 'huh? why did that happen?' behavior.

Note I'm still rather not knowledgeable about d7 so I could likely be wrong.

yched’s picture

So, to summarize the target behavior :

  • a) when a new field instance is added (programmatically or through the UI), it should be hidden in all view modes for which its $instance definition doesn't specify explicit display settings.
    --> OK in HEAD : _field_info_prepare_instance()
  • b) when a new view mode appears (e.g search.module is enabled), it should be displayed like the 'default' mode.
    --> OK in HEAD : view modes can specify whether they want to start as specialized or as 'default'.
    Side remark : starting as specialized is not recommended unless you're a widely-known view mode ('teaser'), for which core or contrib modules or install profiles that ship with predefined fields will likely provide display settings for. If a view mode starts as 'specialized' and no-one is aware of its existence, it displays nothing out of the box.
  • c) when a view mode previously set to 'default' gets specialized to use its own display settings, it should start out with the same display settings it used so far (the settings in 'default' mode). Seamless transition, no disruptive display changes for end users.
    --> not OK in HEAD, that's what this issue is about.

All the patches above are based on making a decision when asked to display a field in a view mode for which we have no specified display settings. They all gain c) but lose a). Bottomline is : when asked to make such a decision, we cannot tell which of a) (hide) or c) (mimic 'default') should apply, because we have no history of what brought us to that state : new field instance, or newly specialized view mode ?

Additionally, regarding chx's patch #15 : there's no case for letting view modes opt out of behavior c). c) is critical for 'search_index' or 'rss', because (from my #17) "[in current HEAD, ] the moment you specialize 'search index' or 'rss' view modes, your cron immediately starts indexing empty content for your nodes, and your live feeds start advertising empty items, until you actually go and configure the view mode". But really, this is a problem for any view mode on a live site. After one innocent click to specialize a view mode, it displays empty content, i.e : site is broken. No need to add a head-scratching dilemma for modules exposing their own view modes, when one of the options is wrong anyway.

That's why IMO the solution is more something like : when a view mode gets specialized, initialize it with the current 'default' settings - meaning, take actual action on all affected instances :

foreach (field_info_instances($entity_type, $bundle) as $instance) {
  $instance['display'][$view_mode] = $instance['display']['default']; 
  field_update_instance($instance);
}

+ do the same for 'extra fields' display settings, which have their own storage.
After that, we have a consistent db state where every unspecified display setting can unambiguously translate to 'hidden'.

Will try to come with a patch soon.

yched’s picture

Status: Needs work » Needs review
FileSize
2.21 KB

Here's a preliminary patch. Initializes newly specialized view mode with the current 'default' settings.

Obvious tradeoff is :
1- Switch a view mode back from 'default' to ''custom settings'
2- The settings for the view mode are the same than for 'default', and you can adjust them to your needs
3- Switch the view mode back to 'default', and then back again to 'custom settings'
4- The display settings for the view mode are back to the same than for 'default', the adjustments done in step 2 are lost.
I think we can live with that.

Still a couple @todos :
- For now I can't really decide whether we want to hardcode this behavior in the field_bundle_settings() API function (like the patch currently does), or only trigger it from Field UI, and let direct API callers handle the situation (or not) the way they intend. Still pondering.
- Still needs tests - depending on how we answer the 1st question, those tests will end up in either field.test or field_ui.test, and will be API-based or form-based (quite similar to the ones chx wrote in #15).
- We need a hook so that contrib modules (read : fieldgroup) can do the same initialisation for their own elements.

Status: Needs review » Needs work

The last submitted patch, field_view_mode_custom-886152-21.patch, failed testing.

pwolanin’s picture

Status: Needs work » Needs review
FileSize
2.26 KB

Added isset() check to remove the notices.

chx’s picture

Assigned: Unassigned » chx

I will add a hoook, document it and add a test.

chx’s picture

FileSize
4.88 KB

Did just that

yched’s picture

Thx folks !

The hook doesn't have to be an 'alter' hook. field_bundle_settings() only stores display settings for 'extra fields', exposed through hook_field_extra_fields(), and we already take care of initializing those.

Contrib modules adding other components to the entities (that are not fields nor extra_fields - again, this is mainly targeted at fieldgroup, tricky animals with nesting...) will take care of storing their display settings themselves. They won't have any businessa altering what we store in field_bundle_settings(). We just need a notification hook ("foobar view mode just got specialized") so that they can massage them just like we do for fields and extra_fields.

So, no alter needed, and at this point in the workflow (just before saving), I'm not sure we want to provide a random alter hook that we don't really need nor have a clear vision for.

chx’s picture

FileSize
4.9 KB

Like this?

pwolanin’s picture

Looks good, but waiting for yched to set rtbc.

rszrama’s picture

fwiw, I tested the functionality and it works as expected; hook change seems reasonable, but almost every other hook I could find that allows modules to operate when something happens has the verb associated with the operation - hook_block_save(), hook_node_load(), etc. I'm wondering if it wouldn't be better as hook_field_bundle_settings_save(). : ?

omar’s picture

Status: Needs review » Reviewed & tested by the community

I applied this patch and tested. On the ?q=admin/structure/types/manage/article/display/search_index page:
* the format of the image is still "hidden"
* the format of the body is set to "default"
* the format of the tags is set to "link"

... so AFAICT, this works and is RTBC.

Omar

chx’s picture

Status: Reviewed & tested by the community » Needs review

Let's wait for yched.

drunken monkey’s picture

Do we still have to wait for yched? It's been basically RTBC for five days now.
Could someone at least ping him on this?

yched’s picture

I'm aware of this one, but have not been able to dedicate time since then (not in sync with CPH folks, I know, sorry).
Will try hard to post an updated patch tonight.

pfrenssen’s picture

the patch in #27 fixes the issue for me, both with the search index and the full content.

yched’s picture

So, sorry for the delay, I spent a couple days mulling over this (+ been busy moving #616240: Make Field UI screens extensible from contrib - part II forward).

I've more or less convinced myself that hardcoding the 'view mode initialization' at the API level is the way to go. Having API and UI produce different effects would be confusing.

The thing is, in current HEAD, display settings specified in a programmatic field_create_instance() are possibly not applied at once, depending on how the admin configured its view modes : If 'rss' view mode is currently configured to use 'default' settings, creating an $instance with $instance['display']['rss'] = whatever has no actual effect.
In current HEAD, we store the setting, and it is kept for if/when the 'rss' mode gets specialized to use its own set of display settings. That way modules creating programmatically adding fields when they're installed can provide suggestions on how they should be displayed in a given view mode. Depending on the current config, these suggestions apply or not.

This thread has identified an issue with that behavior : in most cases, a newly enabled view mode has no stored settings ready and thus starts as empty - and that is really bad for 'search_index' if content indexing happens to be running at that time or shortly after.

Options :
a) Like has been discussed above, initialize a view mode with the current state of 'default' any time it gets specialized, so that the live displayed entities are not affected while the admin adjusts the new display settings
This means that the $instance['display']['rss'] settings in the example above will never be used (will be overwritten before they get a chance to), and there's no point in storing them, field_create_instance() should just silently discard them.
Similarly, when a view mode gets configured back to 'default', there's no point in keeping stale display settings for this view mode, since they will be overridden next time the view mode is specialized. Just clutters the db and confuses things when looking at an actual $instance array.
That's a slight API change for field_create_instance() / field_update_instance(), and more code than I wish this late in the cycle.

b) Only copy 'default' settings for $instances that currently have no stored display settings for the view mode (and would thus result as hidden). Programmatically created instances can come with suggestions, that will be kept for later if the view mode is currently set to 'default'.
When a view mode gets specialized, this still means an instant change on live displayed entities, though, even if it affects less fields. Might be good enough, even though not ideal for the special critical case of 'search_index' view mode.

c) Provide a hook to let modules react and initialize a view mode the way they want. By default, we do b), but node.module can implement a) for the special case of 'search_index'.

catch’s picture

I like (b) - I've added instance settings, realized the view mode didn't have custom settings, then switched it - having my instance settings wiped for that view mode would be pretty frustrating.

yched’s picture

Here's a patch that does c), i.e. b) w/ special case for 'search_index' (full reinitialization)
Comes with a full suite of tests for the behaviors mentioned in that issue (see beginning of #20). This also forms the starting point for a test suite for the 'Manage display' screens, that are not tested at all currently.

Required a couple cleanups :
- Field UI's various forms were inconsistent on how they update instance definitions.
Some merged incoming form values into the 'raw', unmassaged field_read_instance() data before calling field_update_instance() - correct behavior.
Some merged into the 'prepared' field_info_instance() data. The massaging work done in _field_info_prepare_*(), including "put non specified fields to hidden", thus got written back to the actual db definition of the instance. No way to detect "the instance currently has no stored display settings for the view mode" anymore.
--> Moved all form submits to :

$instance = field_read_instance($entity_type, $field_name, $bundle);
$instance['foo'] = $form_state['values']['foo'];
field_update_instance($instance);

- "set non specified fields to hidden"part in _field_info_prepare_instance() :
do it only for view modes that use custom settings. No need to massage data for a view mode that currently uses 'default'.

yched’s picture

side note: actual patch size is ~13kb without the field_ui.test hunks.

yched’s picture

yched’s picture

This most probably needs a reroll after #616240: Make Field UI screens extensible from contrib - part II. I'm away from my coding environment for a couple weeks, and cannot take care of this myself. Any taker ?

Status: Needs review » Needs work

The last submitted patch, field_initialize_view_mode-886152-37.patch, failed testing.

rszrama’s picture

Status: Needs work » Needs review
FileSize
32.05 KB

Ok, I attempted a re-roll. I tried to correct some regressions the patch would've introduced in the Field tests that got moved around. Also, I noticed this patch still includes API comments and an invocation of hook_field_bundle_settings_alter(), but in comment 26 yched said to remove it and comment 27 has chx's change from an alter hook to a save hook. It seems yched reintroduced the hook in comment 37 but I couldn't gather from his comments if this was intentional.

(Other differences between my patch and yched's are due to Git and CVS placing changes in one hunk a little differently.)

Status: Needs review » Needs work

The last submitted patch, field_initialize_view_mode-886152-42.patch, failed testing.

rszrama’s picture

Status: Needs work » Needs review
FileSize
32.06 KB

Fixed some other changes to form field names. Still need clarification on questions in comment 42.

yched’s picture

@rszrama: Thks for this. Yes, basically, further considerations in #35 revealed that the 'alter hook' approach I dismissed in chx's #25 was what we needed after all.

Reroll looks good on visual inspection. RTBC anyone ?

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

code looks good. we add lots of tests and all is green

sun’s picture

moshe weitzman’s picture

We've been RTBC for 2 weeks. Please please provide feedback or commit. Pretty please.

webchick’s picture

I keep staring at it, but this sucker is huge.

I will see chx this weekend and I'm hoping he can explain it to me. :)

chx’s picture

uh oh :)

sun’s picture

Status: Reviewed & tested by the community » Needs review
+++ modules/field/field.api.php	13 Sep 2010 16:17:32 -0000
@@ -2177,6 +2177,44 @@ function hook_field_extra_fields_display
+ * @param $settings
+ *   Settings that are about to be saved.
+ *   @see field_bundle_settings for the structure of this array.

You cannot use @see within another Doxygen directive. Also, function names need trailing (). Either use "See function_name() for..." within the @param or move the @see with proper function name and without trailing sentence at the end of the phpDoc.

+++ modules/field/field.module	13 Sep 2010 16:17:32 -0000
@@ -386,9 +386,36 @@ function field_bundle_settings($entity_t
+    // Let other modules react.
+    drupal_alter('field_bundle_settings', $settings, $entity_type, $bundle);

Are these alter hook arguments in line with other alter hooks? AFAIK, all other Field API alter hooks are using $primary_alterable + $context, whereas $context contains 'entity_type', 'bundle', etc. They should be kept consistent.

Powered by Dreditor.

chx’s picture

sun, thank you very much for your extreme attention to detail. It's really nice of you.

Dave Reid’s picture

Status: Needs review » Reviewed & tested by the community

This is a super A+ patch and looks great to me. Want the testbot to double-confirm an RTBC but I'll go ahead and mark it anyway.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, field_initialize_view_mode-886152-52.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
31.32 KB

That was a copypaste error.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/field/field.api.php	2010-10-02 02:52:52 +0000
@@ -2177,6 +2177,46 @@ function hook_field_extra_fields_display
+ *   Settings that are about to be saved. Ssee field_bundle_settings() for the

Typo in "Ssee".

+++ modules/field/field.module	2010-10-02 02:53:36 +0000
@@ -386,9 +386,41 @@ function field_bundle_settings($entity_t
+    // When a view mode gets specialized to use custom display settings,
+    // initialize fields without explicit display settings for this mode with
+    // the current settings for the 'default' mode.

I've read this comment like 5 times on different days in the meantime, and each time I had to read it at least twice to understand what it tries to tell me.

It starts with the doubled meaning of "specialized custom settings"... What is a "specialized custom thing"? Why not "customized settings"?

Then it goes on and uses "explicit settings" instead of "specialized settings", trying to explain the relationship to "current settings" of the "default mode".

That said, "default mode" settings are used for some "this mode", while "this mode" can remotely mean anything within the context of "specialized custom settings" and "explicit settings" or "current settings".

+++ modules/field/field.module	2010-10-02 02:53:36 +0000
@@ -386,9 +386,41 @@ function field_bundle_settings($entity_t
+    // Let other modules react.
+    // Let modules alter the display settings.

Rarely seen review issue: Duplicate comment.

+++ modules/field_ui/field_ui.admin.inc	2010-10-02 02:52:52 +0000
@@ -1241,19 +1241,23 @@ function field_ui_display_overview_form_
-    $instance = field_info_instance($entity_type, $field_name, $bundle);
+    // Merge incoming values in the stored instance.
+    $instance = field_read_instance($entity_type, $field_name, $bundle);

@@ -1550,9 +1561,12 @@ function field_ui_widget_type_form($form
+  // Merge incoming values in the stored instance.
+  $instance = field_read_instance($entity_type, $field_name, $bundle);

@@ -1887,8 +1902,8 @@ function field_ui_field_edit_form_submit
-  // Update the instance settings.
-  $instance_source = field_info_instance($instance['entity_type'], $instance['field_name'], $instance['bundle']);
+  // Merge the incoming values in the stored instance.
+  $instance_source = field_read_instance($instance['entity_type'], $instance['field_name'], $instance['bundle']);

Not sure what this comment is trying to tell me. I don't see any merge happening in the directly following code lines.

Powered by Dreditor.

chx’s picture

Assigned: chx » Unassigned
catch’s picture

Don't want to derail this, but I think the automatic setting of defaults should be implemented by field_ui module using hook_field_bundle_settings_alter() - if you're changing this via the API then you can change all the field instance settings however you like at the same time, so won't run into this issue anyway.

sun’s picture

@catch: Can you clarify and elaborate a bit more on that? What about the node_field_bundle_settings_alter() that's in this patch already?

catch’s picture

So the current behaviour is that when you set a view mode to custom, all fields are automatically hidden. This was something requested in #715826: field display mode should default to hidden - the issue being that before you would usually end up having to manually specify a large number of fields as hidden, for a view mode that might only display a couple of them.

I think when view modes are changed via the API, it'd be good to keep this behaviour, otherwise if you're making changes in code, you're going to have to figure out what all the instance settings are going to be from the default then set them all again.

So current workflow:

1.Change view mode to custom.
2. field_update_instance() to set body and image field to be displayed.

With this patch:
Change view mode to custom.
2. field_update_instance() to set body and image field to be displayed, and also to set any fields displayed by default to hidden.

Thinking about it, what'd be ideal was if this hunk I think was done via a field ui form submit rather than a hook at all. However if it was in field_ui_field_bundle_settings_alter() then sites that don't have field_ui.module installed at all wouldn't get the behaviour.

+    // When a view mode gets specialized to use custom display settings,
+    // initialize fields without explicit display settings for this mode with
+    // the current settings for the 'default' mode.
+    $view_mode_settings = field_view_mode_settings($entity_type, $bundle);
+
+    foreach (array_keys($settings['view_modes']) as $view_mode) {
+      if (!empty($settings['view_modes'][$view_mode]['custom_settings']) && empty($view_mode_settings[$view_mode]['custom_settings'])) {
+        // Update display settings for field instances.
+        $instances = field_read_instances(array('entity_type' => $entity_type, 'bundle' => $bundle));
+        foreach ($instances as $instance) {
+          if (!isset($instance['display'][$view_mode])) {
+            $instance['display'][$view_mode] = $instance['display']['default'];
+            field_update_instance($instance);
+          }
+        }
+
+        // Update display settings for 'extra fields'.
+        foreach (array_keys($settings['extra_fields']['display']) as $name) {
+          if (!isset($settings['extra_fields']['display'][$name][$view_mode])) {
+            $settings['extra_fields']['display'][$name][$view_mode] = $settings['extra_fields']['display'][$name]['default'];
+          }
+        }
+      }
+    }

Also, node_field_bundle_settings_alter() looks extremely similar to that hunk. I don't have time to verify this morning, but is that actually doing anything that hasn't already been done?

ksenzee’s picture

Assigned: Unassigned » ksenzee

I am going to spend the rest of the day on this and see if I can address catch's comments.

ksenzee’s picture

Assigned: ksenzee » Unassigned
Status: Needs work » Needs review
FileSize
8.72 KB
32.83 KB

I have gone back and forth on this, and in the end I agree with catch that when people are creating instances programmatically, we want to babysit them as iittle as possible. There may be a little confusion over why the UI and the API behave differently (see yched's #35), but I think that's the least of all possible WTFs. It's going to be a lot more confusing to have to go through the workflow catch outlines in #60 ("field_update_instance() to set body and image field to be displayed, and also to set any fields displayed by default to hidden").

So I've moved the magic "copy the default view mode" behavior into field_ui_display_overview_form_submit(), as catch suggested in #60. I've also addressed sun's comments in #56. The new helper function introduced here is not going on my resume, but it's just a helper function. The tests all still pass (locally, anyway), so the UI is behaving the way we expect. And in my opinion, the API was already behaving pretty much the way its users would expect.

I had a hard time figuring out why we still need the alter hook, especially after catch pointed out in #60 that the node.module implementation doesn't seem at first blush to do much of anything. So I ripped it out to see what would happen. What happened was that one of the new tests failed (yay!), which helped me figure out what's going on. Without the alter hook, you only get the default display settings copied over to search_index the *first time* you set the search index to use custom display settings. If you uncheck that "custom settings" box to set search_index back to the default, play around with your default settings, then check the box again, your search index is out of sync with the default view mode. To me that's a good enough reason to keep the alter hook.

Anonymous’s picture

subscribe, will review.

catch’s picture

I'm not sure about keeping the node module hook implementation in. It was my understanding that we'd only want to copy the defaults over if settings for the bundle weren't specified, and not if they are - if we're leaving the custom settings intact when setting bundles back to non-customized, I dunno if we want to override them again when they're switched again. However I won't argue to strongly either way on this since otherwise I'm happy with the patch as is.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Marking this RTBC, I'm not overly keen on leaving the hook implementation in, but there are two equally unlikely edge cases that it either fixes or causes, it's impossible to account for both, and neither should hold the critical bug up.

Pasting irc conversation with ksenzee:

<catch> ksenzee: so I just re-read your comment, seems like there's two possibilities - 1. you customize, un-customize, change defaults, customize again and end up with weird config 2. you customize, change the search settings, uncustomize, recustomize then your changes were wiped.
<ksenzee> parsing
<ksenzee> Yes, 2 is a possibility.
<catch> the first one is broken without having the hook, the second is broken with it.
<ksenzee> Right.
<ksenzee> On balance I think 2 is less likely.
<ksenzee> So I left the hook and implementation in.
<catch> I think they're both relatively unlikely, but in the case of #1 you're left with a reference of what to change.
<catch> whereas if you actually ran into #2 you'd have nothing to go on.
<ksenzee> yes, but you're also left with an inexplicably weird search index.
<ksenzee> well.
<ksenzee> I think either way we're in the realm of seriously corner cases.
<catch> ksenzee: doesn't that also mean that changing this via code will cause the hook to fire?
<ksenzee> catch: yes :/
<catch> what's left is people who keep switching their field settings from customized to uncustomized and back again, and whether to keep copying the defaults across.
<ksenzee> catch: What we need is some JS that tells them "quit switching back and forth between customized and uncustomized you git"
<catch> and whether node module should do that for programmatic changes just for the search index.
<catch> ksenzee: heh.
<catch> ksenzee: so, to be honest, I don't really care much, except from the point of view that there would be less lines in the patch with +
<ksenzee> catch: Heh. I don't care much either, except that I didn't want to undo yched's work unless I had a really super good reason.
<ksenzee> catch: I guess ideally we'd have Dries or webchick make the call.
<ksenzee> But if we asked
<ksenzee> they would be like "you're wasting your time on *that*??"
<catch> ksenzee: I will give up and mark RTBC.
* ksenzee feels bad now :P
<catch> ksenzee: can I paste the conversation from here?
<ksenzee> catch: certainly
yched’s picture

Status: Reviewed & tested by the community » Needs review

Sorry for not returning here yet. Plz hold this just a bit, will look at this later tonight.

yched’s picture

Sorry for being away, many thanks catch, ksenzee and sun for moving this forward.

Moving the behavior to UI only is fine by me. As as said above, I really had a hard time deciding between API or UI-only, so if you guys have clearer use cases, it works for me.

About special casing the 'search_index' view mode, the reasoning was : search_index is special, its' the only view mode that, when changed, has a persistent effect : data in the search index. For all others (we know of), changes are reversible : change display settings, undo the changes, your site is back to step 1 (well, modulo the page cache...). Not true for search_index, if cron runs in between.

So any unintended, behind-the-scene change in display settings used for 'search_index' is not welcome. Such as :
1) search_index uses 'default',
2) I set it to custom. From then on, search index displays differently, in a way that's hard to tell - empty content, or random settings depending on the history of search index (never customized, or customized/tweaked/un-customized/...)
3) I go to the newly enabled settings form for search_index, adjust settings to what I want, submit
If cron runs between 2) and 3), some content gets indexed in a way I did not control - and I'm very probably not even aware of it.
That's the main reason I agreed with the 'critical' status of this thread, BTW.

So I went with : for 'search_index', always initialize with the current display settings for 'default' no matter what (instead of, for other modes : copy 'default' only for fields that don't specify anything for the view mode) - meaning : no change with how search_index displays during 1), 2) and until I submit the actual settings I want in 3).

What bugs me in the current patch, is the mix of API-level / UI-level. node_field_bundle_settings_alter() runs even for the API.
So view modes get no initialization in the API, only in Field UI, except for search_index, that is initialized (slightly differently) in both API and UI. Sounds confusing. If the conclusions about "API or UI-only" (i.e "no, UI only") stand, they also stand for search_index view mode : "If I'm using the API, I can do whatever I want in a single pass".

Well, it's possible that I'm overthinking this search_index special case, sorry for being a pain.

Proposals :
1) let search_index case be processed in a field_ui hook triggered at form submit - inactive for API code
2) call me a hairsplitter and drop the search_index special case completely
2bis) drop the search_index special case completely, and solve this through the UI (possibly in a separate patch) :
implement node_form_alter() so that when "Manage display : search_index" is submitted, the admin gets the option to set all the node type for reindexing.
Would solve the fact that whenever I later change 'search_index' display settings (hide a field, show a field...), my search index is not to date...

catch’s picture

We explicitly documented somewhere that admnistrative changes won't force a search re-index, typing this from phone so can't look it up. As above I'd be happy to remove the special case for search index since the ui code covers the main case here.

effulgentsia’s picture

For what it's worth, I agree with dropping hook_field_bundle_settings_alter() entirely (at least until a real use-case is identified for modules needing to alter settings that come from API usage). Seems like everything we need can be done within the UI submit handlers.

I don't think this issue needs to worry about the edge case of customize search_index, then set it to default, then set it to specialize, and having it undesirably use the prior customizations that still remain in the db, until the new customizations get saved. I do wonder why we retain customizations in the db when making a view mode use default again. Seems like perhaps we should clean up after ourselves? But anyway, can be discussed in a separate, non-critical issue.

But, I do think we need to handle:
1) Set search_index to specialized the first time
2) Admin takes 30 minutes to customize it
3) cron runs in between 1 and 2

But from what I can tell, #63 does handle that, and would continue to do so even with the removal of the hook.

yched’s picture

I'll reroll when #940668: "Manage display" : Formatter change not reflected on settings gets in (it contains the field_ui.test restructure that also included here)

effulgentsia’s picture

Title: All fields are hidden after the administrator newly configures a view mode to not use 'default' display » All fields hidden on view modes newly configured by administrator to not use 'default' display

Thanks, yched. The other issue just landed, so setting this to "needs work".

In summary, I think the consensus is:

1) When a module uses an API function to newly configure a view mode to not use 'default' display, then what is in HEAD (all fields hidden unless explicitly set otherwise) is what is desired, as per #60.

2) When an administrator uses the UI to newly configure a view mode to not use 'default' display, then what is in HEAD (all fields hidden until explicitly set otherwise) is not desired, because it will take the administrator time to fill out the form deciding on what should and shouldn't be displayed, and during this time, it doesn't make sense to hide content from the website, and it especially doesn't make sense to hide it from the search index, as per OD, and why this is critical.

Although it introduces some inconsistency, this seems like the right solution to me, because of the different needs and limitations of a module and an administrator (a module doesn't suffer from time passing between configuring a view mode to not use default display and then setting what needs to be displayed, and a module can easily copy the default view mode's settings if it wants to, whereas, it's annoying for an administrator to manually re-configure all the fields to be just like the default except for whatever minor customization is wanted).

effulgentsia’s picture

Title: All fields hidden on view modes newly configured not to use 'default' display » All fields are hidden after the administrator newly configures a view mode to not use 'default' display
Status: Needs review » Needs work

tweaking title.

yched’s picture

Title: All fields hidden on view modes newly configured by administrator to not use 'default' display » All fields are hidden after the administrator newly configures a view mode to not use 'default' display
Status: Needs work » Needs review
FileSize
17.48 KB

Rerolled, and removed the 'search_index' special case, along with hook_field_bundle_settings_alter()

In the earlier iterations of the patch, the hook was also the place where fieldgroup module, or other modules that add their own components (non field, non 'extra_field') to displayed entities, could perform their own initialization when a view mode is switched from 'default' to 'custom'.
Now that the initialisation is not done in the API but only in the UI, I guess they will need to use form_alter() to perform in their own initialization. Anyway, that issue is long enough now, let's deal with this separately.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Looks great, bot is happy, RTBC.

claar’s picture

Nitpicks:

+++ modules/field/field.info.inc
@@ -301,16 +303,21 @@ function _field_info_prepare_instance($instance, $field) {
+  $view_modes = array_merge(array('default'), array_keys($entity_info['view modes']));;

Extra semicolon.

+++ modules/field_ui/field_ui.test
@@ -470,4 +470,151 @@ class FieldUIManageDisplayTestCase extends FieldUITestCase {
+   *   Message to display

Missing period (3 instances of this).

Reroll & interdiff attached.

claar’s picture

FileSize
574 bytes

Appears d.o doesn't like hash marks in attachment names; re-uploading interdiff.

effulgentsia’s picture

For Dries or webchick. As per #49, this patch is huge and involves navigating some esoteric internals of the Field API, though it's gotten smaller since that comment. Most of it is the code cleanup described in #37, and approved by many people, including the glowing review in #53. The cleanup was to address various internal inconsistencies, and some auxiliary bugs found when writing the excellent tests that are added to this patch for the "Manage Display" pages. Since the scope of the issue changed in #60 / #72, some of those cleanups might no longer be strictly necessary, but then again, why not benefit from the excellent work that went into it!

The real meat of the patch in terms of addressing the primary scope of this issue is:

+++ modules/field_ui/field_ui.admin.inc	22 Oct 2010 18:49:15 -0000
@@ -1311,10 +1315,14 @@ function field_ui_display_overview_form_
+        // Initialize the newly customized view mode with the display settings
+        // from the default view mode.
+        _field_ui_add_default_view_mode_settings($entity_type, $bundle, $view_mode_name, $bundle_settings);

and the implementation of that helper function. That's what implements #72.2 to solve the OD.

yched’s picture

"Since the scope of the issue changed in #60 / #72, some of those cleanups might no longer be strictly necessary"
No, they still are. The reason for them, explained in #37, still stand for the current patch.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, after spending another half hour with this patch, and re-reading all the comments again, I think this is good to go. It's much less scary since the last time I reviewed it, and has sign-off from all the right people.

Committed #76 to HEAD. Thanks!

webchick’s picture

Status: Fixed » Needs work

Haha. That will teach me to commit patches without re-testing them. :P

This broke testbot:

Manage display (FieldUIManageDisplayTestCase) [Field UI]
Message	Group	Filename	Line	Function	Status
The field is hidden in view modes that use custom settings.	Other	field_ui.test	501	FieldUIManageDisplayTestCase->testViewModeCustom()	
The field is hidden in 'rss' mode.	Other	field_ui.test	524	FieldUIManageDisplayTestCase->testViewModeCustom()	
The previous settings are kept when 'rss' mode is specialized again.	Other	field_ui.test	540	FieldUIManageDisplayTestCase->testViewModeCustom()

I've rolled it back for now.

catch’s picture

Status: Needs work » Needs review

I can't reproduce the test failures locally, let's give the test bot a crack at it.

catch’s picture

Status: Needs review » Needs work

The last submitted patch, 886152-74-76-inter.diff, failed testing.

webchick’s picture

Oh, testbot...

Re-uploading #76.

webchick’s picture

Status: Needs work » Needs review
catch’s picture

Status: Needs review » Reviewed & tested by the community

This passes, and I couldn't reproduce the head failure locally, so try again?

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok. Committed to HEAD.

*duck* :P

sun’s picture

Since this patch landed, I'm seeing arbitrary random test failures of other patches. Don't have any test results to link to, because I sent the patches for re-testing, but I suspect the usual hiccup with $this->randomString() in combination with asserting that string in the output without running it through check_plain() first.

effulgentsia’s picture

Priority: Critical » Normal
Status: Fixed » Needs work

Yep. Just ran into #89 on #319483-25: FAPI checkboxes and radios need strengthening for XSS.

So code like this...

$value = rand(1, 100);
...
$this->assertNodeViewNoText($node, 'rss', $value, t("The field is hidden in 'rss' mode."));

Seems likely to yield false positives. If $value is 1, seems like "1" is likely to show up in a rendered node somewhere.

webchick’s picture

Priority: Normal » Critical

Promoting back to critical since it's causing testbot to fail randomly, wasting dozens of hours of core contributor time, and those are the very worst kind of failures..

drunken monkey’s picture

Status: Needs work » Needs review
FileSize
1.14 KB

This should fix it. (Is probably even safer than necessary – $value is now between 10,000 and 100,000 and is additionally checked for being contained in $body.)

effulgentsia’s picture

FileSize
1006 bytes

5.5 hours to be running the test for #92 seems outrageous. Not sure if it's caught in an infinite loop.

Here's an alternate fix. After all, why do we want this particular value to be random?

yched’s picture

Status: Needs review » Reviewed & tested by the community

#93 should be fine indeed. Sorry for the broken test.

drunken monkey’s picture

Huh, no idea what went wrong with my patch. I triple-checked, the logic is OK as it is. (I regularly suffer from those forgetting "!"s …)
Well, your approach is probably better anyways, should be safe enough and not as insanely paranoid as mine. So thanks for the fix.

Hm, test bot still running – does someone know, what we should do in such a case, who we can tell to restart the test server or such? Don't want to permanently disable one test server with a broken test …

JirkaRybka’s picture

Just a sidenote: I'm not sure what exactly happened, but this seems weird about #92: PHP manual for strpos() says:

If needle is not a string, it is converted to an integer and applied as the ordinal value of a character.

We have integer $value here, actually searching for a single character with a code of 10 to 100 thousands - which obviously doesn't exist, but I really don't know how PHP might react to invalid character code in such case. Maybe even searching for an empty string, leading to the loop in question? Dunno...

drunken monkey’s picture

Oh, wow, can't remember ever knowing that. You live and you learn …
Locally this works fine, but this is probably the best clue to why this doesn't work. Or it's just a coincidence. *shrug*

I reported it in the d.o infrastructure queue, btw: #953600: Test bot stuck in infinite loop?
But back to the issue at hand, let's just commit #93 and stop the random fails.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

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