hook_field_extra_fields()
allows a module to define a default weight, that can be overridden by the user, but not a default visibility:
function hook_field_extra_fields() {
$extra['node']['page']['display']['my_display'] = array(
'label' => t('My special display'),
'description' => t('Displays some blah blah ...'),
'weight' => -4,
'visible' => FALSE, // In D8, yet not in D7
);
return $extra;
}
Why not allow a default visibility setting, that can be overridden by the user? As of now all extra fields are displayed by default.
Comment | File | Size | Author |
---|---|---|---|
#91 | drupal-n1256368-91-FALSE.patch | 1.29 KB | DamienMcKenna |
#91 | drupal-n1256368-91.patch | 1.29 KB | DamienMcKenna |
Comments
Comment #1
yched CreditAttribution: yched commentedHm, why not, but right now it would only make sense for 'display' extra elements (as opposed to 'form' elements - Field UI doesn't let you hide elements in forms)
Comment #2
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRight, I didn't notice I was pasting the part with the form elements.
Comment #3
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedPlease review.
Comment #4
yched CreditAttribution: yched commented[edited out, I was on crack]
This looks reasonable.
Comment #5
yched CreditAttribution: yched commentedBack to CNR for the testbot.
Comment #6
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThanks for the review yched. I think you missed something there. If it is not set we want it to be TRUE. However !empty will be FALSE in that case.You were faster editing then I was replying ;)
Comment #7
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedComment #8
timcosgrove CreditAttribution: timcosgrove commentedIn the course of reviewing this, I found the patch no longer applied, probably due to other subsequent changes. Re-rolled.
Comment #9
wbobeirne CreditAttribution: wbobeirne commentedConfirming #8 worked for me. Makes extra fields much more viable, as now they won't insert themselves on every node type they appear in without you setting them to visible.
Comment #10
timcosgrove CreditAttribution: timcosgrove commentedComment #11
timcosgrove CreditAttribution: timcosgrove commentedAdding backport patch for 7.x
Comment #13
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedThank you, timcosgrove. The patch filename should contain
d7
in order to test it against Drupal 7 while the issue is on Drupal 8.We should ask webchick to see if this is backportable, but it certainly would make a nice API addition.
Moving back to RTBC as of #9.
Comment #14
timcosgrove CreditAttribution: timcosgrove commentedResubmitting the patch with a d7 prefix. Thanks! I was wondering how testing was going to happen, given that this is an 8.x issue.
The field.info.inc code is nearly identical in both cases, so I see no reason why it wouldn't be backportable. If that can't be done in this issue, I'll open a feature request against 7.x and cross-reference this issue. I will admit that my primary motivation for working on this is to use it now, in our D7 sites, so I'm more interested in the backport.
Comment #16
timcosgrove CreditAttribution: timcosgrove commentedSo #13 is mistaken; this does not work, nor has it ever been the case that it worked. Please see http://drupal.org/node/1319154 for a description of how the backporting process works.
I am reattaching the d8 patch so that tests get run again against d8.
Comment #17
timcosgrove CreditAttribution: timcosgrove commentedAlso see http://drupal.org/node/332678 for more on automated patch testing.
Comment #18
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedIndeed, sorry. I thought that had been added, but only "do-not-test" support is there, as of now.
Comment #19
iamEAP CreditAttribution: iamEAP commentedTagging contributed project blocker because i18n is unable to hide node language fields by default: #1350638: Language display should default to hidden when enabling "Multilingual content"
Backport would be possible since it's an API addition, which is allowed.
Comment #20
Alan D. CreditAttribution: Alan D. commented@iamEAP I'm not sure that this is a project blocker, just a huge pain in the ...! I can post the code we used to update one of our sites for two new extra fields if you are interested.
+1 for getting this in AND backporting, the update scripts to handle this would need to modify
field_bundle_settings
variable, which would mean that it is likely that contrib is likely to kill the display settings for all fields at least once or twice until this gets in...Comment #21
Alan D. CreditAttribution: Alan D. commentedAs per #9.
Comment #22
tim.plunkettI think this should read
Obviously still needs to be wrapped at 80.
Comment #23
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedRerolled with that.
Comment #24
dalinThis is much needed. Currently if a new field gets added to a large site, it gets displayed everywhere and there's no sane way to disable it.
Comment #25
iamEAP CreditAttribution: iamEAP commentedSpeaking of #24, an almost-sane way to hide them for those who desperately need it now: hook_field_extra_fields_display_alter().
Comment #26
catchThis looks fine but can we add tests for the new feature?
Comment #27
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedNot without a testing module that provides an extra field that is hidden by default. Should we add one? We could use the module we need for #1593888: Tests for indentation in Field UI, or a different one.
Comment #28
catchYes this will need a testing module, it may well be possible to re-use the existing field_test module (or whatever it's called) and just add the hook implementation to it.
Comment #29
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedOk, thanks. Added tests.
Comment #30
Alan D. CreditAttribution: Alan D. commentedLooks great, thanks Niklas
Comment #31
Alan D. CreditAttribution: Alan D. commentedActually, two minor points.
Re-rolled patches against D7 for testing.
Comment #32
Alan D. CreditAttribution: Alan D. commentedThe tests missed the third option, that this doesn't negatively impact the existing API. This patch (against 7.x atm, but will reroll to 8.x) checks that if the visibility is not specified, then the field will show.
Comment #34
tim.plunkett#29 is RTBC. Let's leave it that way, until committed.
Comment #35
Alan D. CreditAttribution: Alan D. commentedK. I was going to quickly run some D7 tests then re-roll for D8 with minor text mods / additional tests. Accidentally deleted setUp() and it took a while to figure out what was going wrong. Out of time to re-roll to D8 :(
The D7 one adds all 3 use-cases, visibility specified as true, false, unspecified, and adds a test on another content type to ensure that the view modes only effect the specified bundle (there are no tests for anything related to extra fields afaict). Appending to assist backport to D7.
Comment #36
tim.plunkettWell, if the D7 one has more assertions, they should be added to D8 before this goes in.
Comment #37
Alan D. CreditAttribution: Alan D. commentedOnly one additional assertion. I was testing the field_test module with 3 of the additional ones ;)
Comment #39
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedTest failure looks unrelated to me.
#37: drupal-1256368-extra-fields-visibility-37-d8.patch queued for re-testing.
Comment #40
swentel CreditAttribution: swentel commentedSo, by default, all extra fields which do not not have the visible key will be displayed, right ? Isn't the intent of the issue (which also causes a lot of frustration for site builders) that they should be hidden by default if there's no specification ? I'd personally vote for hiding them by default to be honest, unless I'm reading the code and tests completely wrong of course (it's late here).
Comment #41
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI'd suggest accepting this as is, because it's the only way this is backportable. Once this has been committed to 8.x, I'll open an issue to reconsider the visibility settings of the extra fields available in D8.
Comment #42
Alan D. CreditAttribution: Alan D. commentedI have to agree with Niklas, anything else would be a change in the API and be unlikely to accepted. We should do our best to no hold this issue up (this 2 liner change is nearly a year old).
I am sure that contrib will quickly update their additional field definitions.
Summary, fix to this issue is a 2 liner.
The rest are tests, for this issue and fields via hook_node_view() successfully adding content to nodes that are displayed via the default visibility configuration.
Comment #43
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLet's see if this still applies. #37: drupal-1256368-extra-fields-visibility-37-d8.patch queued for re-testing.
Edit: Nope, it doesn't. Needs a reroll.
Comment #45
webflo CreditAttribution: webflo commentedI tried to reroll the patch from #37. This feature/regression was fixed in #1757504: Regression: language field is not visible on manage display. But there are no test for this feature.
Comment #46
rooby CreditAttribution: rooby commentedThis patch would be a very welcome addition.
Wouldn't it be sensible to also add label visibility as an option as well if we have visibility?
[EDIT] Sorry, just realised that extra fields don't have a label, but that's a whole separate issue.
Comment #47
Chi CreditAttribution: Chi commented#45: hook_field_extra_fields-visibility-test-1256368-45.patch queued for re-testing.
Comment #49
Chi CreditAttribution: Chi commentedRerolled patch from #45
Comment #50
andypostIs this a feature or bug? Also there's a related issue #1954188: Revaluate the concept of 'extra fields'
Comment #51
BerdirThe re-roll is missing the test.
Could use this as well in a 7.x project. I guess for 8.x it's a task to write tests for it as it seems to have been fixed already.
Comment #52
jhodgdonComing here from #1614108: Document hook_field_extra_fields()'s "edit" and "delete" properties... I would just like to point out that back in June of 2012, fields *did* support visibility.
The function _field_info_prepare_extra_fields():
http://drupalcode.org/project/drupal.git/blob/e039aaaa32c5053a441330b403...
supported a 'visible' element for the 'display' mode, and this was used in field_ui_display_overview_form():
http://drupalcode.org/project/drupal.git/blob/e2bcdbcedb605325e4f0889428...
So if this is not present in the current code, it may be a regression? I could also be mistaken about whether the previous functionality is what is being requested here... I didn't read all 50 comments, was just responding to David Rothstein's comment here on that other issue:
http://drupal.org/node/1614108#comment-7299436
Comment #53
Alan D. CreditAttribution: Alan D. commented@jhodgdon
Needs work for tests. The core functionality was committed accidentally in another commit.
Comment #54
Alan D. CreditAttribution: Alan D. commentedDuh. It has been lost.
[edit] I think, completely new to this class
Comment #55
David_Rothstein CreditAttribution: David_Rothstein commented@jhodgdon, @Alan D., I don't understand. Those code examples show that the default 'visible' setting is hardcoded to TRUE, don't they? But the point of this issue is to allow it to be provided by hook_field_extra_fields() (so that people defining new "extra fields" can start them off as 'visible' = FALSE if they want to).
That change was committed to Drupal 8, but I don't see any evidence that it was ever in Drupal 7.
Comment #56
Alan D. CreditAttribution: Alan D. commentedQuick look at the D7 git history didn't show anything.
AFAICT, this never got to that point to backport, as this was never finalized for D8 due to lack of tests. And this lack of tests allowed the committed code to be reverted if I am reading the latest 8.x branch code correctly. So now neither branches appear to have this. :(
Comment #57
jhodgdonRE #55... See #52 (the links are to what was committed about a year ago in May). It looks to me as though if you pass in 'visible' => TRUE or 'visible' => FALSE in the version of _field_info_prepare_extra_fields() as of that date, then it is used/return, but if you don't set up a display mode, 'visible' is set to TRUE. Right? And that function is called from the collate function
http://drupalcode.org/project/drupal.git/blob/e039aaaa32c5053a441330b403...
which invokes hook_field_extra_fields() to get the information to pass to _field_info_prepare_extra_fields(). So I think as of about a year ago, this was possible to do with the hook. Right?
Comment #58
David_Rothstein CreditAttribution: David_Rothstein commented@Alan D., you might be right, I'm not seeing any evidence of it in Drupal 8 either right now. I guess the tests can prove that, one way or another...
Comment #59
David_Rothstein CreditAttribution: David_Rothstein commented@jhodgdon, what I'm seeing in that code from May 2012 (leaving out the surrounding code) is:
$extra_fields contains the default settings passed in from the hook, but $settings comes from somewhere else (I believe that comes from the database, i.e. for existing fields it respects whatever was saved in the admin UI most recently, but for new fields it won't be there). And no matter what, the display settings on the $field_data variable are getting overwritten.
In the case of 'weight', they can be overridden based on the value that was provided in $extra_fields, but in the case of 'visible', they can't?
Comment #60
jhodgdonDuh, you are correct. I read that code several times and glossed over that.
So we should go back to that docs issue and remove 'visible' from the hook docs for both 7 and 8 currently then?
Comment #61
Alan D. CreditAttribution: Alan D. commentedTracked this down to #1852966: Rework entity display settings around EntityDisplay config entity.
Posted there to ask for some feedback here (without reopening, so hopefully someone will trace this here)
Comment #62
David_Rothstein CreditAttribution: David_Rothstein commented@jhodgdon: Hah, well, I've certainly been in that position before too :)
So I decided to check if it works in Drupal 8, and although I can't figure out where in the code is making it work, it does actually seem to. For example, node_field_extra_fields() sets 'visible' => FALSE on the Language form element, and it correctly shows up as hidden on the Manage Display screen. And if you remove that line of code and clear caches, it goes back to being un-hidden.
So I think for Drupal 8 the issue here just needs tests after all (only Drupal 7 needs the functionality)... I'll go back and change the docs issue accordingly also.
Comment #63
klonos...coming from #2024011: Expose a display setting to hide new EVA fields by default
Comment #64
dabblela CreditAttribution: dabblela commentedPosting a new D7 patch in case anyone needs it.
Comment #65
jpstrikesback CreditAttribution: jpstrikesback commentedfire up the bot
Comment #67
jhodgdonPlease do not post Drupal 7 patches on Drupal 8 issues until the 8.x issue is resolved.
Comment #68
David_Rothstein CreditAttribution: David_Rothstein commentedI think it's OK to post a Drupal 7 patch but it's best to follow the instructions for how to do so that appear on the file upload field:
Although in this case the issue was already "needs work" so the testbot wasn't supposed to run on it anyway :)
Posting the Drupal 7 patch somewhere on drupal.org is also a requirement if anyone is going to use it in a Drupal distribution before it's committed (see https://drupal.org/node/1475972) and while it could be posted in another issue to reduce noise, I think it's better to keep it in one place?
Comment #69
jpstrikesback CreditAttribution: jpstrikesback commentedapologies :)
Comment #70
theunraveler CreditAttribution: theunraveler commentedRe-rolled for 7.23.
Comment #71
GoZ CreditAttribution: GoZ commented7.23 is passed and patch wasn't included. Do we have to open new issue for d7 and keep this for d8 so patch can be committed ?
Comment #72
jhodgdonCan someone please update the issue summary so we know what needs to be done for both Drupal 7 and Drupal 8?
If the patches are substantially similar for both, our policy is normally that bugs have to be fixed in 8 before fixes can be committed to 7.
If the patches/code is substantially different in 7, we might consider branching off to a separate issue. I'm just not sure what the plan is for 8 and 7 and the issue summary doesn't illuminate the issue at all.
Comment #73
kristiaanvandeneyndePatch in #70 looks good to me for D7.
Comment #74
kristiaanvandeneyndeComparing the following two (correct file for D8?)
I'd say the changes in D8 are vast enough for this to be separated into a D7 and a D8 issue. Judging from #62 this now works for D8 but needs only testing.
Can we get the patch from #70 into D7 please? :)
Comment #75
jhodgdonOK, feel free to (a) create a separate D7 issue (or a separate D8 issue?) and (b) update the issue summary for both and (c) make sure the issues are linked to each other.
Comment #76
kristiaanvandeneyndeSplit off the D8/D7 tests into their own issue.
Comment #77
jhodgdonOK, so this is now just a D7 issue, but there is still no real issue summary. And we do not commit patches to D7 or D8 without tests -- the proposed D7 patch has not even been run though the test bot, and it has no test for the proposed change.
It is fine to make a separate issue for D8 but it is not fine to say that making the test for new behavior is a separate issue. We need tests for reviewers to verify the new code is OK.
Comment #78
kristiaanvandeneyndeI understand your point about the necessity of tests. It's a bit difficult in this case because the functionality is, according to #62, already in D8 without having tests written for it.
Seeing as we will not backport the D8 functionality (because it's a complete rewrite?), I figured we could already get this into D7 while others work on the tests for both versions. Of course this is silly, because it could cause regressions.
I just cloned a copy of D8 to do some grep magic on and perhaps find a test or comment left or right that may point us in the right direction.
In all fairness: I did update the issue summary, just nothing ground-breaking. How would you like to see it updated?
Comment #79
klonos...hiding all patches except the last one.
Comment #80
kristiaanvandeneyndeFound it in D8! :)
The extra fields now use EntityDisplay objects along with regular fields: https://drupal.org/node/1875952
So I did some digging and found the D8 functionality in Drupal\field_ui\DisplayOverviewBase::buildExtraFieldRow(). See the code (line 447).
So it could very well be that there are tests already for D8, but that they now are more generic tests for EntityDisplay objects.
(Cross-posting in related issue)
Comment #81
jhodgdonI guess the issue summary is good enough. We do have a template. :) This issue went through a lot of iterations about whether it was a bug, a docs bug, a feature request, etc. but the current summary does seem to reflect what we want to happen.
Anyway, we do need a test if we want to get this into D7.
And at this point, since it's not a purely documentation issue, I'm going to stop following it and let the maintainers of this sub-system and/or the D7 maintainers weigh in on whether they would accept such a feature request this late in the D7 game.
Comment #82
NancyDruThis works fine for me on 7.22, tests or no tests.
Comment #83
tim.plunkettThat conditional made me go cross-eyed, so I rewrote it a bit.
I might have time this week to look at a test...
I'm going to update a version of this that uses FALSE as the default just to see if we have any coverage at all.
Comment #85
tim.plunkettSo we do have lots of implicit test coverage.
We just need explicit tests that setting it to FALSE does work.
Comment #86
Fabianx CreditAttribution: Fabianx commentedBesides the missing test I think this is RTBC :).
Comment #88
ordermind CreditAttribution: ordermind commentedSubscribing, this is an important issue.
Comment #90
Jaypan CreditAttribution: Jaypan commentedOrdermind - you don't need to post to subscribe to topics, you can just click the 'follow' (or subscribe?) button at the top of the thread, underneath the issue information.
Comment #91
DamienMcKennaRerolled.
Comment #95
Fabianx CreditAttribution: Fabianx for Drupal Association commentedComment #97
jcisio CreditAttribution: jcisio commentedAren't tests always missing?
Comment #98
GoZ CreditAttribution: GoZ at Centarro commentedTests with FALSE by default drupal-n1256368-91-FALSE.patch will always fail since in their tests, modules assume extra field is displayed by default.
Comment #99
jcisio CreditAttribution: jcisio commentedRe #98 I meant, see #85. We need PASSED tests when the default value is FALSE.
Comment #100
pjcdawkins CreditAttribution: pjcdawkins commentedJust a small note: this comment should obviously say FALSE if the default value is FALSE
Comment #101
Alan D. CreditAttribution: Alan D. commentedI think Tim means that test will need to be added to the actual patch to ensure that this is actually covered, rather than generating a patch that fails :)
The code and reviews are rtbc, the doco is correct, this is just missing some test to check that:
a) The fields still show that are already define
b) Adding a new field with visibility set to FALSE does not show
While imho, FALSE default makes sense, that would be an massive API change in this area, so this needs to be TRUE by default. Which the second patch does (drupal-n1256368-91.patch).
Comment #102
tim.plunkett#101.b is what I meant.
Leave the default as is, but we need a test showing that a declaration of FALSE does what we expect.
Comment #103
David_Rothstein CreditAttribution: David_Rothstein commentedYes, as a new feature this really should have tests, especially since it looks like they are already in Drupal 8 (at least to some extent); see discussion in #2165879: Write tests for 'visibility' of fields exposed with hook_entity_extra_field_info(). That could be the basis for some Drupal 7 tests. Or the basis could be many of the earlier patches in this issue, many of which look like they included tests in the patch, but then those got lost in later patches...?
Comment #104
David_Rothstein CreditAttribution: David_Rothstein commentedOops.
Comment #105
gabesulliceUntil this patch is finished, I just wanted to leave this here for anyone who comes looking for an answer on how to hide an extra field by default. Below is a D7 snippet to hide newly created extra fields when they're defined for the first time. With it, you won't need to wait on this patch to be committed or patch core yourself.