Commit message
Issue #1953528 by swentel, amateescu, plopesc: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable.
The configuration for
"does this view mode shows a separate setup screen in 'Manage Display', or does it use the settings for the 'default' view mode"
is the only thing left in field_bundle_settings(), which is still based on variable_set() / variable_get().
This is configured bundle by bundle, and thus is not a direct property of the view mode.
Leave view modes CMI files untouched, but use the "active" property of the EntityDisplay CMI files
This maps fairly well to the conceptual model of the feature - the EntityDisplay is there, just "disabled".
Drawback: You load two EntityDisplay objects at runtime; "load the display for 'full'; oh, it has 'active = FALSE', then load the display for 'default'"
API changes
- field_bundle_settings() (used to read and write) goes away, there's nothing left in there.
- most reads of those settings are done through entity_get_render_display(), so the storage change will stay encapsulated in that function
Comment | File | Size | Author |
---|---|---|---|
#77 | 1953528-77.patch | 34.95 KB | swentel |
#77 | interdiff.txt | 2.1 KB | swentel |
#74 | 1953528-74.patch | 35.13 KB | swentel |
#74 | interdiff.txt | 6.45 KB | swentel |
#73 | 1953528-73.patch | 37.81 KB | swentel |
Comments
Comment #1
swentel CreditAttribution: swentel commentedTagging
Comment #2
swentel CreditAttribution: swentel commentedThis can move forward now.
Comment #3
swentel CreditAttribution: swentel commentedJust an idea,
Comment #4
yched CreditAttribution: yched commentedThat's not very different from caching :-) - which, as we discussed in Portland, raises the question of cache clears. But, agreed, cleaning on config import should be fine.
That's also one extra db call (or more depending on the granularity of the "cache' entry). But well, whichever place we put those "which ones are enabled, which ones use default" settings (in another config entry, in cache, in state), there will be an extra hit anyway, so...
Comment #5
yched CreditAttribution: yched commentedAnd, right, of those three possible places, state looks the most appropriate.
Comment #6
swentel CreditAttribution: swentel commentedSo yes, indeed, that's another hit indeed into the database, forgot that ... Since it only contains one type of setting anymore (view mode -> state), I guess we can merge them into one entry, instead of 'field_bundle_settings_{entity_type}_{bundle}' ?
Comment #7
yched CreditAttribution: yched commentedSounds good. We have a static cache on state gets ?
Comment #8
swentel CreditAttribution: swentel commentedSo, no code yet, but I started locally, and maybe we don't really need state() at all.
There's already entity_get_view_modes() and we should/could rename field_view_mode_settings() to entity_view_mode_settings() since the latter calls entity_get_view_modes() anyway and all that information is cached ..
Comment #9
swentel CreditAttribution: swentel commentedOk, nevermind that comment ..
Comment #10
swentel CreditAttribution: swentel commentedMaybe we should just keep the same way ... ?
Comment #12
amateescu CreditAttribution: amateescu commentedIs it me or we don't need this config at all anymore since we now always keep the status property in the view_mode config entity?
Comment #13
yched CreditAttribution: yched commentedI don't think so. This setting is specific to bundles separately, and if I'm not mistaken, the view mode config entity is about describing the view mode itself (on an entity type as a whole, no per-bundle settings thete)
Comment #14
amateescu CreditAttribution: amateescu commented/me shakes fist at bundles :)
Comment #15
yched CreditAttribution: yched commentedOf course, this could end up into a "list of bundlrs for which the view mode is active" in the CMI file for the view mode. But AFAIK this is not done yet
Comment #16
amateescu CreditAttribution: amateescu commentedHere's a crazy idea: how about putting a targetEntityBundles property on the config entity? and query it with EQ..
Comment #17
amateescu CreditAttribution: amateescu commentedLOL, we typed the same thing at the same time :)
Comment #18
yched CreditAttribution: yched commentedWell, it's about removing our last variable_set() / variable_get(), so this is critical...
Comment #19
yched CreditAttribution: yched commentedUpdated the issue title & summary
Comment #20
swentel CreditAttribution: swentel commentedYes, this one is the first I'll get back to to get back into the game. Expect updates this week and weekend.
Comment #20.0
swentel CreditAttribution: swentel commentedUpdate summary
Comment #21
alexpottWe need to complete the variable_* from the entity & field systems.
Comment #22
swentel CreditAttribution: swentel commentedLet's see what this does.
Comment #23
swentel CreditAttribution: swentel commentedDid a quick grep still in codebase, field_bundle_settings is called in user_update and upgrade path isn't handled yet.
Comment #24
swentel CreditAttribution: swentel commentedDjeezus, completely wrong patch, one sec.
Comment #25
swentel CreditAttribution: swentel commentedComment #26
aspilicious CreditAttribution: aspilicious commentedI thought we were trying to not access properties directly. Isn't it "better" to use functions that access these properties.
$mode->setTargetBundle($bundle, $status);
OR
$mode->enableBundle($bundle);
$mode->disableBundle($bundle);
OR
...
Comment #28
swentel CreditAttribution: swentel commentedI can live with those setter functions, will do that next, now at least get the 100's of notices away and probably a few upgrade paths too. No interdiff yet.
Comment #30
swentel CreditAttribution: swentel commentedMore getters and setters, failure also seems completely unrelated .. we clearly have no upgrade tests for the status of bundle settings ...
Comment #31
aspilicious CreditAttribution: aspilicious commentedMaybe we should default $status to TRUE. That way you can easily add a $view_mode to an entity display. Could be usefull in contrib.
You forgot this one.
Comment #32
yched CreditAttribution: yched commentedYay for kicking this :-)
EntityDisplayMode config entities are the responsibility of entity system, so this code should be in entity.module's entity_entity_bundle_rename(), this housekeeping is not the responsibility of field.module anymore.
Also, shouldn't there be the same housekeeping for form modes ?
(same for delete)
Why this change, the plural seems correct ?
Similarly, this doesn't belong to field.module anymore, it's just about reading data held in EntityDisplayMode objects.
The only place where this is used (well outside of Field UI that provides a UI for it) is within entity_get_render_display()...
I'd tend to think this function can go away now (or at least be deprecated & moved to field.deprecated.inc), and the consuming code should be able to read directly from the EntityDisplayMode objects.
(Same for field_form_mode_settings())
It seems the method itself could be completely removed and inlined on the one single place that calls it ?
Comment #33
amateescu CreditAttribution: amateescu commentedI did just that in #2044863-10: EntityFromController doesn't assign weights for extra fields that are not yet saved in a EntityFormDisplay :)
Comment #34
swentel CreditAttribution: swentel commentedAnother thing: what do we do with contrib/custom who have declared/created custom view modes ?
So it means we can only upgrade to view modes that core knows of and we simply leave the field_bundle_settings file there and then contrib has to take care of it ? Sounds like we might need upgrade helper functions then no ?
Comment #35
swentel CreditAttribution: swentel commentedAdditionally, we still have hook_entity_view_mode_info_alter (and for form modes) though which allows you to still add view/form modes programmatically. With that, the 'targetBundles' approach in this patch can never work, so we should probably kill that alter hook ?
Comment #36
amateescu CreditAttribution: amateescu commentedDefinitely!
Comment #37
yched CreditAttribution: yched commentedOuch, yes, tough... That's a drawback of approach 1) over approach 2). Approach 2) places the info in the EntityDisplay objects, and all of those are created in field_update_8002().
As mentioned in the OP, I tend to consider option 2) more inline with what the feature really is about: bypass the display object that might exist for a view mode and use 'default' instead. That's a logic/behavior internal to Display objects, encapsulated in entity_get_render_display(), so not coupling it with behavior in another ConfigEntity type is cleaner. When setting those checkboxes in the UI, we're configuring Display objects.
The drawback of option 2) was it meant loading two display objects instead of one (load the display for $mode, see if it's "active", if not load the display for 'default'). I think it can be mitigated though:
- use ConfigFactory::loadMultiple() to load raw CMI data for the two displays. That's one single multiple_cache_get request.
- check the 'status' on the display for $mode to determine the right entity display to use
- use a full entity_load() to load that display entity. This doesn't hit the db since the CMI data is in memory after step 1, and we just create the one Display ConfigEntity we need.
Sorry for the change of gears :-/ I think I see more clearly now why I'd favor option 2 now...
What do you guys think ?
Indeed, if view/form modes live in config, we can't have alter hooks on them. This should have been done as part of "view modes as config entities". Could be a separate issue though.
Comment #38
swentel CreditAttribution: swentel commentedI can live with option two as well. It will give us less pain in the upgrade path too, so I'll recode, no problem there.
Comment #39
swentel CreditAttribution: swentel commentedThat also means we can leave the alter in view modes for now. I guess altering config objects is probably a more general problem, also acknowledged already at some point during an IRC conversation with alex. We can discuss maybe a bit further in Prague.
Comment #40
swentel CreditAttribution: swentel commentedNew patch - only tested in manually, let's see what gives.
I included the patch from #2044863: EntityFromController doesn't assign weights for extra fields that are not yet saved in a EntityFormDisplay as well.
Comment #42
swentel CreditAttribution: swentel commentedComment #44
swentel CreditAttribution: swentel commentedThis should fix some more things. Some funny things in the interdiff, especially the drupal-7.field.database :)
User register still behaves annoying though
Comment #45.0
(not verified) CreditAttribution: commentedAPI changes
Comment #46
swentel CreditAttribution: swentel commentedThis should be green
Comment #48
yched CreditAttribution: yched commented#46: 1953528-46.patch queued for re-testing.
Comment #49
yched CreditAttribution: yched commentedThanks !
Started to review, but I need to run for now, posting what I have.
"the display to use for rendering this entity display" :-)
"We load the default and the view mode..." - Grammar is a bit sloppy.
Suggestion: "Look at the default display and display for the view mode, and fallback to the former if the latter does not exist or is disabled" ?
(also applies to entity_get_render_form_display())
#37 had a suggestion to optimize that ?
+ Maybe using an $ids array with the ids keyed with 'default' & 'view_mode' would help streamline the code (avoid re-concatenating the several times again)
+ In a followup, we need to consider how we can optimize for multiple entity rendering (do one single entity_load_multiple('entity_display') for all the entities). That might mean moving entity_get_render_display() to a method in EntityRenderController.
Surprised that the $status is better off defaulted to FALSE ? It seems most calling code uses TRUE ?
Couldn't we just leave the param out, set the status to TRUE, and let callers set FALSE explicitly if they need ?
Here would be a good place to put a word of explanation about the "fallback to the 'default' display" logic ?
something gone wrong here ;-)
Could be clearer IMO:
?
+ Not for this issue, but it seems ViewModeAccessCheck & FormModeAccessCheck could share most of their code in a base class...
Comment #50
swentel CreditAttribution: swentel commentedI have the same feeling about getDisplayModeSettings() and saveDisplayModeSettings(). I'd been refactoring all of those at once here. I can take a crack at that tomorrow.
Comment #51
yched CreditAttribution: yched commentedDidn't come across getDisplayModeSettings() / saveDisplayModeSettings() in my review yet, so I don't know about those.
The thing about ViewModeAccessCheck() & FormModeAccessCheck() is that I don't think they should be needed in the end.
What the Field UI does currently is still modeled after the D7 architecture:
- define routes for "Manage display [view_mode]" pages,
- use access callbacks to only show those where the view modes uses "custom settings"
- in the page callbacks, show a form collecting various stuff and save them where appropriate on submit.
The D8 situation is that those "Manage display" pages really are "the edit form of an EntityDisplay entity".
So the EntityDisplay object should be loaded by the route as part of parameter upcasting (in a way that the upcast would fail if status = FALSE --> 404), and received as an argument in the form builder (instead of currently: the form builder receives the raw URL parts, and at some point goes "I'll load an EntityDisplay object because I'll do stuff with it").
(and same for form displays of course)
That is way outside the scope of this specific patch, of course. But that's why I'm thinking maybe we don't need to sweat on refactoring ViewModeAccessCheck() & FormModeAccessCheck() here, they might not be needed in the end.
Comment #52
swentel CreditAttribution: swentel commentedRight, good point, didn't see it from that point of view. We'll take step by step. killing the variables first is a giant step already anyway :)
Comment #53
swentel CreditAttribution: swentel commentedAddressed all points I think, except for 3. I left that behavior as is. It's true most calling code in the update script is using TRUE, always when we know the view or form mode is 'default'. But for other modes, we don't know whether they are actually enabled or not. The status of those is updated later then, see
Comment #54
yched CreditAttribution: yched commentedThanks @swentel !
As discussed in IRC, the $status param in _update_8000_entity_get_display() still looks weird IMO - it's the "status that will get assigned to the freshly created display if it doesn't exist in config yet"... better if we there's a possibility we can do without it IMO.
Review for the rest of the patch :
Nitpick, but looks a bit weird, those two things are not really related, they only happened to be stored at the same location.
Maybe just say "Migration of the content of the old 'bundle_settings_*'' variables" here, and then in the code, separate comments for "Migrate display settings for extra fields." & "Migrate view mode settings" ?
rename getDisplayModeSettings() / saveDisplayModeSettings() to getDisplayStatuses() / setDisplayStatuses($statuses) ?
+ As you pointed in a previous comment, yup, those should move to the OverviewBase class (the only difference is the entity type of the display objects to load)
Would be better with a multiple load, and better if we removed the 'default' *before* loading it rather than after.
Maybe a protected helper that collects all ids and removes the one for 'default' ? (getDisplayModeSettings() & saveDisplayModeSettings() both need to do that same dance currently)
The label we want is the human name of the view mode, that info is not available in the $display object (and the calling code actually doesn't make use of this 'label' property populated here). getDisplayModeSettings() should just return an array of view_mode (machine name) => boolean status ?
not strictly needed, as the status will be (re)saved by saveDisplayModeSettings() anyway (which is fine IMO: initialize a new display if needed, then set all statuses - not fully optimized, but keeps the code simpler)
+ we should rename the $display_mode_bundle_settings var in DisplayOverviewBase::submitForm()
It's damn long, and irrelevant too now. $display_statuses ?
Similarly, it should now be a (view_mode_name => bool) array ?
Woah, nice catch, took me a while to understand what happened here.
- This is indeed 'custom_settings' in D7.
- #1043198: Convert view modes to ConfigEntity renamed it to 'status' in D8, but did not provide a corresponding upgrade, and instead renamed the properties to 'status' in the "test D7 database". LOL.
Looks a bit weird ? why is this needed now ?
Comment #55
swentel CreditAttribution: swentel commentedNew patch - lets see how it behaves.
Comment #56
swentel CreditAttribution: swentel commentedAnd here's the interdiff
Comment #57
yched CreditAttribution: yched commentedSide note: created #2094499: Convert "Manage (form) display" forms to be the official entity edit forms for EntityView[Form]Display objects for #51.
Comment #58
andypostConfig entities already have methods to works with status (
enable()
,disable()
,setStatus()
,status()
) seeConfigEntityBase
, just need to define entity_keysAlso should be added to
entity_keys
annotation of display and form display configurables. the property already defined inConfigEntityBase
Config entities have
setStatus()
method for thatUse
status()
method of config entityIs this change related?
status()
Comment #59
swentel CreditAttribution: swentel commentedRe: 2 : no we're in upgrade, let's not call classess
Re: 5: yes that's relevant #54 point 6
I'll see for other changes.
Comment #60
swentel CreditAttribution: swentel commentedComment #62
yched CreditAttribution: yched commentedAs discussed a couple days ago in IRC, we should avoid numeric indexes here. Rather key $ids with 'default' and $view_mode, then check
if ($display = $entity_displays[$ids[$view_mode]] && $display['status']) {...}
Also, note that we are using raw config data here, so we can't use status(), needs to be ['status']
Comment #63
Hydra CreditAttribution: Hydra commentedNot sure about #58 4, but I made the other suggestions from #58 Swentel missed in #60 and what yched pointed in #62
Comment #64
Hydra CreditAttribution: Hydra commentedOkay, yched did a quick review over my shoulder, I hope I managed to get all he told me :) Thank you very much!
This also needs to be done for display mode
This was a misunderstanding, we don't need this here.
We don't have the object here, so we need to use set
I tried fixing those issues in the following patch.
Comment #66
swentel CreditAttribution: swentel commentedAlso, #2086095: Remove remaining references to field_sql_storage will go in, so we need to remove the 'variable' reference (the table install) that are still in the tests - we can look in that test to see where we need to change.
Comment #67
swentel CreditAttribution: swentel commentedShould be green. We need the isset check because if not we get a 1000 notices ...
Comment #69
andypostForum displays need update too!
unnecessary change
this already defined in 'ConfigEntityBase'
D7 has changed?
Comment #70
swentel CreditAttribution: swentel commentedre: 3 no - in D7 it's custom_settings - we just never tested that variable :)
I'll fix the rest today
Comment #71
yched CreditAttribution: yched commented1. needed because the patch adds a new entry in the annotation
3. see #54.6
Comment #72
andypost1. is not needed because nothing is added to form and view modes
Comment #73
swentel CreditAttribution: swentel commented1. Ugh, yeah unrelated change there
2. we'll leave status there so we can actually document the behavior and how the entity displays are loaded and falling back to 'default' and such.
3. change forum entity displays.
Comment #74
swentel CreditAttribution: swentel commentedNew patch after yched's review at the con
Comment #75
yched CreditAttribution: yched commentedThanks ! Looks good if green
Comment #76
alexpottWe should not be reading directly from the config storage here... in ConfigStorageController::buildQuery() we do the following:
We need to get config through the factory to ensure we can use the overrides system.
Comment #77
swentel CreditAttribution: swentel commentedOk, talked this through with alex yesterday, today and with yched, should be fine when coming back green.
A follow up after this is moving entity_get_render_display and entity_get_render_form_display functions inside the entity render controller.
Comment #78
yched CreditAttribution: yched commentedWorks for me if green.
The followup about optimizing entity_get_render_display() / entity_get_render_form_display() is #2090509: Optimize entity_get_render_display() for the case of "multiple entity view"
Comment #79
alexpottCommitted 10b43b9 and pushed to 8.x. Thanks!
Comment #80
alexpottOops - we need a change notification here.
Comment #81
aspilicious CreditAttribution: aspilicious commentedComment #82
catchComment #83
swentel CreditAttribution: swentel commentedStarted change notification over at https://drupal.org/node/2104695
Comment #84
ianthomas_ukExample Drupal 7 and Drupal 8 code for each of the uses mentioned would make that a lot clearer.
Comment #84.0
ianthomas_ukUpdated issue summary.
Comment #85
xjmTagging "Missing change record", as completing the change record updates here blocks a beta: #2083415: [META] Write up all outstanding change notices before release
The patch for this issue was committed on September 27, 2013, meaning that the change record here has been waiting to be finalized for more than four months. Let's add the example code and get the change record reviewed so we can put this issue to bed. :)
Comment #86
yched CreditAttribution: yched commentedAdded code samples, and adjusted the change notice a bit.