Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#93 | field_ui_test.93.patch | 1006 bytes | effulgentsia |
#92 | field_ui_test.patch | 1.14 KB | drunken monkey |
#85 | field_initialize_view_mode-886152-76.patch | 17.87 KB | webchick |
#77 | 886152-74-76-inter.diff | 574 bytes | claar |
#76 | 886152-#74-#76-inter.diff | 574 bytes | claar |
Comments
Comment #1
jhodgdonDefaults should probably be the same as when viewing a node in full page view, not all off...
Comment #2
pwolanin CreditAttribution: pwolanin commentedThis 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.
Comment #3
jhodgdonSo 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.
Comment #4
jhodgdonTrying 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:
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
That is probably it. Why that default was chosen, I'm not sure...
Comment #5
hefox CreditAttribution: hefox commentedLooked into "Why that default was chosen"
http://drupal.org/node/553298#comment-2805046
Comment #6
pwolanin CreditAttribution: pwolanin commentedComment #7
pwolanin CreditAttribution: pwolanin commentedComment #8
hefox CreditAttribution: hefox commentedThat 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.
Comment #9
hefox CreditAttribution: hefox commentedhuhu how did that happen
Comment #10
jhodgdonI 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."
Comment #11
yched CreditAttribution: yched commentedsubscribe - short of time tonight, but please don't rush this in.
Comment #12
chx CreditAttribution: chx commentedI 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.
Comment #13
chx CreditAttribution: chx commentedThe above patch hopefully has 1 fail. This hopefully has 0.
Comment #15
chx CreditAttribution: chx commentedTurns 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.)
Comment #16
sunLooks good, but yched asked to not rush this in.
Comment #17
yched CreditAttribution: yched commentedFirst 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.
Comment #18
chx CreditAttribution: chx commentedAnd 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...?
Comment #19
hefox CreditAttribution: hefox commentedI 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.
Comment #20
yched CreditAttribution: yched commentedSo, to summarize the target behavior :
--> OK in HEAD : _field_info_prepare_instance()
--> 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.
--> 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 :
+ 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.
Comment #21
yched CreditAttribution: yched commentedHere'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.
Comment #23
pwolanin CreditAttribution: pwolanin commentedAdded isset() check to remove the notices.
Comment #24
chx CreditAttribution: chx commentedI will add a hoook, document it and add a test.
Comment #25
chx CreditAttribution: chx commentedDid just that
Comment #26
yched CreditAttribution: yched commentedThx 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.
Comment #27
chx CreditAttribution: chx commentedLike this?
Comment #28
pwolanin CreditAttribution: pwolanin commentedLooks good, but waiting for yched to set rtbc.
Comment #29
rszrama CreditAttribution: rszrama commentedfwiw, 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(). : ?
Comment #30
omar CreditAttribution: omar commentedI 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
Comment #31
chx CreditAttribution: chx commentedLet's wait for yched.
Comment #32
drunken monkeyDo we still have to wait for yched? It's been basically RTBC for five days now.
Could someone at least ping him on this?
Comment #33
yched CreditAttribution: yched commentedI'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.
Comment #34
pfrenssenthe patch in #27 fixes the issue for me, both with the search index and the full content.
Comment #35
yched CreditAttribution: yched commentedSo, 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'.
Comment #36
catchI 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.
Comment #37
yched CreditAttribution: yched commentedHere'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 :
- "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'.
Comment #38
yched CreditAttribution: yched commentedside note: actual patch size is ~13kb without the field_ui.test hunks.
Comment #39
yched CreditAttribution: yched commented#37: field_initialize_view_mode-886152-37.patch queued for re-testing.
Comment #40
yched CreditAttribution: yched commentedThis 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 ?
Comment #42
rszrama CreditAttribution: rszrama commentedOk, 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.)
Comment #44
rszrama CreditAttribution: rszrama commentedFixed some other changes to form field names. Still need clarification on questions in comment 42.
Comment #45
yched CreditAttribution: yched commented@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 ?
Comment #46
moshe weitzman CreditAttribution: moshe weitzman commentedcode looks good. we add lots of tests and all is green
Comment #47
sun#44: field_initialize_view_mode-886152-44.patch queued for re-testing.
Comment #48
moshe weitzman CreditAttribution: moshe weitzman commentedWe've been RTBC for 2 weeks. Please please provide feedback or commit. Pretty please.
Comment #49
webchickI 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. :)
Comment #50
chx CreditAttribution: chx commenteduh oh :)
Comment #51
sunYou 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.
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.
Comment #52
chx CreditAttribution: chx commentedsun, thank you very much for your extreme attention to detail. It's really nice of you.
Comment #53
Dave ReidThis 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.
Comment #55
chx CreditAttribution: chx commentedThat was a copypaste error.
Comment #56
sunTypo in "Ssee".
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".
Rarely seen review issue: Duplicate comment.
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.
Comment #57
chx CreditAttribution: chx commentedComment #58
catchDon'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.
Comment #59
sun@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?
Comment #60
catchSo 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.
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?
Comment #62
ksenzeeI am going to spend the rest of the day on this and see if I can address catch's comments.
Comment #63
ksenzeeI 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.
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedsubscribe, will review.
Comment #65
catchI'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.
Comment #66
catchMarking 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:
Comment #67
yched CreditAttribution: yched commentedSorry for not returning here yet. Plz hold this just a bit, will look at this later tonight.
Comment #68
yched CreditAttribution: yched commentedSorry 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...
Comment #69
catchWe 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.
Comment #70
effulgentsia CreditAttribution: effulgentsia commentedFor 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.
Comment #71
yched CreditAttribution: yched commentedI'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)
Comment #72
effulgentsia CreditAttribution: effulgentsia commentedThanks, 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).
Comment #73
effulgentsia CreditAttribution: effulgentsia commentedtweaking title.
Comment #74
yched CreditAttribution: yched commentedRerolled, 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.
Comment #75
catchLooks great, bot is happy, RTBC.
Comment #76
claar CreditAttribution: claar commentedNitpicks:
Extra semicolon.
Missing period (3 instances of this).
Reroll & interdiff attached.
Comment #77
claar CreditAttribution: claar commentedAppears d.o doesn't like hash marks in attachment names; re-uploading interdiff.
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedFor 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:
and the implementation of that helper function. That's what implements #72.2 to solve the OD.
Comment #79
yched CreditAttribution: yched commented"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.
Comment #80
webchickOk, 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!
Comment #81
webchickHaha. That will teach me to commit patches without re-testing them. :P
This broke testbot:
I've rolled it back for now.
Comment #82
catchI can't reproduce the test failures locally, let's give the test bot a crack at it.
Comment #83
catch#74: field_initialize_view_mode-886152-74.patch queued for re-testing.
Comment #85
webchickOh, testbot...
Re-uploading #76.
Comment #86
webchickComment #87
catchThis passes, and I couldn't reproduce the head failure locally, so try again?
Comment #88
webchickOk. Committed to HEAD.
*duck* :P
Comment #89
sunSince 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.
Comment #90
effulgentsia CreditAttribution: effulgentsia commentedYep. Just ran into #89 on #319483-25: FAPI checkboxes and radios need strengthening for XSS.
So code like this...
Seems likely to yield false positives. If $value is 1, seems like "1" is likely to show up in a rendered node somewhere.
Comment #91
webchickPromoting 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..
Comment #92
drunken monkeyThis 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.)
Comment #93
effulgentsia CreditAttribution: effulgentsia commented5.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?
Comment #94
yched CreditAttribution: yched commented#93 should be fine indeed. Sorry for the broken test.
Comment #95
drunken monkeyHuh, 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 …
Comment #96
JirkaRybka CreditAttribution: JirkaRybka commentedJust a sidenote: I'm not sure what exactly happened, but this seems weird about #92: PHP manual for strpos() says:
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...
Comment #97
drunken monkeyOh, 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.
Comment #98
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.