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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

swentel’s picture

Issue tags: +Field API

Tagging

swentel’s picture

Assigned: Unassigned » swentel
Status: Postponed » Active

This can move forward now.

swentel’s picture

Just an idea,

  • Use status from the config entity, just like views does
  • store the actual settings (enabled or disabled) into state() - that way we can avoid the potential double loading of an config object. The only thing we need to figure out is whether we can clear the state on config import - I bet we can
yched’s picture

That'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...

yched’s picture

And, right, of those three possible places, state looks the most appropriate.

swentel’s picture

So 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}' ?

yched’s picture

Sounds good. We have a static cache on state gets ?

swentel’s picture

So, 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 ..

swentel’s picture

Ok, nevermind that comment ..

swentel’s picture

Status: Active » Needs review
FileSize
16.33 KB

Maybe we should just keep the same way ... ?

Status: Needs review » Needs work

The last submitted patch, view-modes-settings-1953528-10.patch, failed testing.

amateescu’s picture

Is 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?

yched’s picture

I 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)

amateescu’s picture

/me shakes fist at bundles :)

yched’s picture

Of 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

amateescu’s picture

Here's a crazy idea: how about putting a targetEntityBundles property on the config entity? and query it with EQ..

amateescu’s picture

LOL, we typed the same thing at the same time :)

yched’s picture

Priority: Normal » Critical

Well, it's about removing our last variable_set() / variable_get(), so this is critical...

yched’s picture

Title: Move 'view modes' part out of field_bundle_settings() » Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable

Updated the issue title & summary

swentel’s picture

Yes, this one is the first I'll get back to to get back into the game. Expect updates this week and weekend.

swentel’s picture

Issue summary: View changes

Update summary

alexpott’s picture

We need to complete the variable_* from the entity & field systems.

swentel’s picture

Status: Needs work » Needs review
FileSize
5.03 KB

Let's see what this does.

swentel’s picture

Did a quick grep still in codebase, field_bundle_settings is called in user_update and upgrade path isn't handled yet.

swentel’s picture

Djeezus, completely wrong patch, one sec.

swentel’s picture

FileSize
8.67 KB
aspilicious’s picture

I 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

...

Status: Needs review » Needs work

The last submitted patch, 1953528-25.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
11.03 KB

I 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.

Status: Needs review » Needs work

The last submitted patch, 1953528-28.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
14.03 KB

More getters and setters, failure also seems completely unrelated .. we clearly have no upgrade tests for the status of bundle settings ...

aspilicious’s picture

  1. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayModeBase.php
    @@ -75,4 +82,31 @@ public function getTargetType() {
    +  public function setTargetBundle($bundle, $status) {
    

    Maybe we should default $status to TRUE. That way you can easily add a $view_mode to an entity display. Could be usefull in contrib.

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php
    @@ -147,9 +147,11 @@ protected function getDisplayModeSettings() {
    +      $entity_view_mode->targetBundles[$this->bundle] = $status;
    

    You forgot this one.

yched’s picture

Yay for kicking this :-)

+++ b/core/modules/field/field.moduleundefined
@@ -320,9 +320,13 @@ function field_entity_bundle_rename($entity_type, $bundle_old, $bundle_new) {
-  $settings = variable_get('field_bundle_settings_' . $entity_type . '__' . $bundle_old, array());
-  variable_set('field_bundle_settings_' . $entity_type . '__' . $bundle_new, $settings);
-  variable_del('field_bundle_settings_' . $entity_type . '__' . $bundle_old);
+  $view_modes = entity_load_multiple('view_mode');
+  foreach ($view_modes as $view_mode_info) {
+    if ($view_mode_info->getTargetType() == $entity_type) {
+      $view_mode_info->renameTargetBundle($bundle_old, $bundle_new);
+      $view_mode_info->save();
+    }

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)

+++ b/core/modules/field/field.moduleundefined
@@ -546,7 +510,7 @@ function field_form_mode_settings($entity_type, $bundle) {
- *   An array keyed by view mode, with the following key/value pairs:
+ *   An array keyed by view mode, with the following key/value pair:

Why this change, the plural seems correct ?

+++ b/core/modules/field/field.moduleundefined
@@ -555,16 +519,18 @@ function field_view_mode_settings($entity_type, $bundle) {

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())

+++ b/core/modules/field/lib/Drupal/field/FieldInfo.phpundefined
@@ -607,24 +607,8 @@ public function prepareInstance($instance, $field_type) {
   public function prepareExtraFields($extra_fields, $entity_type, $bundle) {

It seems the method itself could be completely removed and inlined on the one single place that calls it ?

amateescu’s picture

It seems the method itself could be completely removed and inlined on the one single place that calls it ?

I did just that in #2044863-10: EntityFromController doesn't assign weights for extra fields that are not yet saved in a EntityFormDisplay :)

swentel’s picture

Another thing: what do we do with contrib/custom who have declared/created custom view modes ?

  • Programmatically with entity_info_alter(): the bundle_settings file will exist, but no view mode definition exists after upgrade.
  • Display suite or 'view modes' module aren't upgraded either

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 ?

swentel’s picture

Additionally, 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 ?

amateescu’s picture

Definitely!

yched’s picture

what do we do with contrib/custom who have declared/created custom view modes?

Ouch, 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 ?

we still have hook_entity_view_mode_info_alter (and for form modes) ... we should probably kill that alter hook ?

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.

swentel’s picture

I 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.

swentel’s picture

That 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.

swentel’s picture

FileSize
26.68 KB

New 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.

Status: Needs review » Needs work

The last submitted patch, 1953528-40.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
27.58 KB

Status: Needs review » Needs work

The last submitted patch, 1953528-42.patch, failed testing.

swentel’s picture

Status: Needs work » Needs review
FileSize
6.04 KB
30.23 KB

This should fix some more things. Some funny things in the interdiff, especially the drupal-7.field.database :)

User register still behaves annoying though

Status: Needs review » Needs work

The last submitted patch, 1953528-44.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

API changes

swentel’s picture

Status: Needs work » Needs review
FileSize
1.98 KB
32.04 KB

This should be green

Status: Needs review » Needs work
Issue tags: -API change, -Field API, -Approved API change

The last submitted patch, 1953528-46.patch, failed testing.

yched’s picture

Status: Needs work » Needs review
Issue tags: +API change, +Field API, +Approved API change

#46: 1953528-46.patch queued for re-testing.

yched’s picture

Thanks !

Started to review, but I need to run for now, posting what I have.

  1. +++ b/core/includes/entity.inc
    @@ -727,11 +728,18 @@ function entity_get_render_display(EntityInterface $entity, $view_mode) {
    -  // Determine the display to use for rendering this entity. Depending on the
    -  // configuration of the view mode for this bundle, this will be either the
    -  // display associated to the view mode, or the 'default' display.
    +  // Determine the display to use for rendering this entity display. We load
    +  // the default and the view mode that is passed in. Depending on the status
    +  // of the view mode entity display, we fall back on default.
    

    "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())

  2. +++ b/core/includes/entity.inc
    @@ -727,11 +728,18 @@ function entity_get_render_display(EntityInterface $entity, $view_mode) {
    +  $render_view_mode = 'default';
    +  $load = array(
    +    $entity->entityType() . '.' . $entity->bundle() . '.default',
    +    $entity->entityType() . '.' . $entity->bundle() . '.' . $view_mode,
    +  );
    +  $entity_displays = entity_load_multiple('entity_display', $load);
    +  if (isset($entity_displays[$entity->entityType() . '.' . $entity->bundle() . '.' . $view_mode]) && $entity_displays[$entity->entityType() . '.' . $entity->bundle() . '.' . $view_mode]->status) {
    +    $render_view_mode = $view_mode;
    +  }
    

    #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.

  3. +++ b/core/modules/entity/entity.install
    @@ -19,11 +19,13 @@
    -function _update_8000_entity_get_display($entity_type, $bundle, $view_mode) {
    +function _update_8000_entity_get_display($entity_type, $bundle, $view_mode, $status = FALSE) {
    

    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 ?

  4. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -51,6 +51,13 @@
       /**
    +   * Whether this display is enabled or not.
    +   *
    +   * @var boolean
    

    Here would be a good place to put a word of explanation about the "fallback to the 'default' display" logic ?

  5. +++ b/core/modules/field/field.install
    @@ -254,23 +253,18 @@ function field_update_8002() {
    +              $display_id =  $entity_type . '.' . $bundle . '.' . $view_mode;            if (!isset($displays[$display_id])) {
    

    something gone wrong here ;-)

  6. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php
    @@ -31,8 +31,14 @@ public function access(Route $route, Request $request) {
    +      $visibility = ($form_mode == 'default') ? TRUE : FALSE;
    +      if ($form_mode != 'default') {
    +        $entity_form_display = entity_load('entity_form_display', $entity_type . '.' . $bundle . '.' . $form_mode);
    +        if ($entity_form_display) {
    +          $visibility = $entity_form_display->status;
    +        }
    +      }
    

    Could be clearer IMO:

    if ($form_mode == 'default') {
      $visibility = TRUE;
    }
    elseif ($display = entity_load(...)) {
      $visibility = $display->status;
    }
    else {
      $visibility = FALSE;
    }
    

    ?

    + Not for this issue, but it seems ViewModeAccessCheck & FormModeAccessCheck could share most of their code in a base class...

swentel’s picture

+ Not for this issue, but it seems ViewModeAccessCheck & FormModeAccessCheck could share most of their code in a base class...

I 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.

yched’s picture

Didn'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.

swentel’s picture

Right, 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 :)

swentel’s picture

FileSize
8.09 KB
31.3 KB

Addressed 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

      if (isset($variable_value['view_modes'])) {
        foreach ($variable_value['view_modes'] as $view_mode => $view_mode_settings) {
          // Determine name and create initial entry in the $displays array
          // if it does not exist yet.
          if (!isset($displays[$display_id])) {
            $displays[$display_id] = _update_8000_entity_get_display($entity_type, $bundle, $view_mode);
          }
          $displays[$display_id]->set('status', $view_mode_settings['custom_settings']);
        }
      }
yched’s picture

Thanks @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 :

  1. +++ b/core/modules/field/field.install
    @@ -238,13 +238,12 @@ function field_update_8002() {
    -  // Migration of 'extra_fields' display settings. Avoid calling
    -  // entity_get_info() by fetching the relevant variables directly in the
    -  // variable table.
    +  // Migration of 'extra_fields' and view mode 'custom_settings' settings.
    

    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" ?

  2. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php
    @@ -140,16 +140,39 @@ protected function getDisplayModes() {
       protected function getDisplayModeSettings() {
    

    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)

  3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php
    @@ -140,16 +140,39 @@ protected function getDisplayModes() {
    +    foreach ($ids as $id) {
    +      $display = entity_load('entity_display', str_replace($config_prefix . '.', '', $id));
    +      if ($display->get('mode') == 'default') {
    +        continue;
    +      }
    

    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)

  4. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverview.php
    @@ -140,16 +140,39 @@ protected function getDisplayModes() {
    +      $display_settings[$display->get('mode')] = array(
    +        'label' => $display->label(),
    +        'status' => $display->get('status')
    +      );
    

    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 ?

  5. +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php
    @@ -519,6 +519,7 @@ public function submitForm(array &$form, array &$form_state) {
    +            $display->setStatus(TRUE);
    

    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 ?

  6. +++ b/core/modules/system/tests/upgrade/drupal-7.field.database.php
    @@ -12,19 +12,19 @@
    -      'status' => 1,
    +      'custom_settings' => 1,
    

    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.

  7. +++ b/core/modules/user/lib/Drupal/user/Tests/UserRegistrationTest.php
    @@ -213,6 +213,8 @@ function testRegistrationWithUserFields() {
    +    entity_get_form_display('user', 'user', 'register')
    +      ->save();
     
    

    Looks a bit weird ? why is this needed now ?

swentel’s picture

FileSize
32.05 KB

New patch - lets see how it behaves.

swentel’s picture

FileSize
15.44 KB

And here's the interdiff

yched’s picture

andypost’s picture

Config entities already have methods to works with status (enable(), disable(), setStatus(), status()) see ConfigEntityBase, just need to define entity_keys

  1. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -51,6 +51,14 @@
    +  public $status;
    
    @@ -129,6 +137,7 @@ public function getExportProperties() {
    +      'status',
    

    Also should be added to entity_keys annotation of display and form display configurables. the property already defined in ConfigEntityBase

  2. +++ b/core/modules/field/field.install
    @@ -287,15 +284,22 @@ function field_update_8002() {
    +          $displays[$display_id]->set('status', $view_mode_settings['custom_settings']);
    

    Config entities have setStatus() method for that

  3. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/FormModeAccessCheck.php
    @@ -31,8 +31,16 @@ public function access(Route $route, Request $request) {
    +        $visibility = $entity_form_display->status;
    
    +++ b/core/modules/field_ui/lib/Drupal/field_ui/Access/ViewModeAccessCheck.php
    @@ -31,8 +31,16 @@ public function access(Route $route, Request $request) {
    +        $visibility = $entity_display->status;
    
    +++ b/core/modules/field_ui/lib/Drupal/field_ui/DisplayOverviewBase.php
    @@ -725,6 +709,49 @@ protected function getExtraFieldVisibilityOptions() {
    +      $display_statuses[$display->get('mode')] = $display->get('status');
    

    Use status() method of config entity

  4. +++ b/core/modules/system/tests/upgrade/drupal-7.field.database.php
    @@ -12,19 +12,19 @@
    -      'status' => 1,
    +      'custom_settings' => 1,
    ...
    -      'status' => 0,
    +      'custom_settings' => 0,
    ...
    -      'status' => 0,
    +      'custom_settings' => 0,
    

    Is this change related?

  5. +++ b/core/includes/entity.inc
    @@ -819,12 +829,20 @@ function entity_get_form_display($entity_type, $bundle, $form_mode) {
    +    if (isset($entity_form_displays[$ids[1]]) && $entity_form_displays[$ids[1]]['status']) {
    

    status()

swentel’s picture

Re: 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.

swentel’s picture

FileSize
4.24 KB
33.06 KB

Status: Needs review » Needs work

The last submitted patch, 1953528-60.patch, failed testing.

yched’s picture

+++ b/core/includes/entity.inc
@@ -750,12 +751,20 @@ function entity_get_display($entity_type, $bundle, $view_mode) {
+    if (isset($entity_displays[$ids[1]]) && $entity_displays[$ids[1]]->status()) {

As 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']

Hydra’s picture

Status: Needs work » Needs review
FileSize
2.97 KB
34.22 KB

Not sure about #58 4, but I made the other suggestions from #58 Swentel missed in #60 and what yched pointed in #62

Hydra’s picture

FileSize
34.2 KB
2.81 KB

Okay, yched did a quick review over my shoulder, I hope I managed to get all he told me :) Thank you very much!

  1. +++ b/core/includes/entity.inc
    @@ -857,11 +857,11 @@ function entity_get_render_form_display(EntityInterface $entity, $form_mode) {
    -      'entity.form_display.' . $entity->entityType() . '.' . $entity->bundle() . '.default',
    -      'entity.form_display.' . $entity->entityType() . '.' . $entity->bundle() . '.' . $form_mode,
    +      'default' => 'entity.form_display.' . $entity->entityType() . '.' . $entity->bundle() . '.default',
    +      $form_mode => 'entity.form_display.' . $entity->entityType() . '.' . $entity->bundle() . '.' . $form_mode,
         );
         $entity_form_displays = \Drupal::service('config.storage')->readMultiple($ids);
    -    if (isset($entity_form_displays[$ids[1]]) && $entity_form_displays[$ids[1]]->status()) {
    +    if ($form_display = $entity_form_displays[$ids[$form_mode]] && $form_display['status']) {
    

    This also needs to be done for display mode

  2. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormMode.php
    @@ -47,7 +47,8 @@
    + *     "status" = "status"
    
    +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewMode.php
    @@ -48,7 +48,8 @@
    + *     "status" = "status"
    

    This was a misunderstanding, we don't need this here.

  3. +++ b/core/modules/field/field.install
    @@ -294,7 +294,7 @@ function field_update_8002() {
    -          $displays[$display_id]->set('status', $view_mode_settings['custom_settings']);
    +          $displays[$display_id]->setStatus($view_mode_settings['custom_settings']);
    

    We don't have the object here, so we need to use set

I tried fixing those issues in the following patch.

Status: Needs review » Needs work

The last submitted patch, 1953528-64.patch, failed testing.

swentel’s picture

Also, #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.

swentel’s picture

Status: Needs work » Needs review
FileSize
2.24 KB
35.25 KB

Should be green. We need the isset check because if not we get a 1000 notices ...

Status: Needs review » Needs work

The last submitted patch, 1953528-67.patch, failed testing.

andypost’s picture

Issue tags: +Configuration system

Forum displays need update too!

  1. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormMode.php
    @@ -47,7 +47,7 @@
    - *     "uuid" = "uuid"
    + *     "uuid" = "uuid",
    
    +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityViewMode.php
    @@ -48,7 +48,7 @@
    - *     "uuid" = "uuid"
    + *     "uuid" = "uuid",
    

    unnecessary change

  2. +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
    @@ -51,6 +51,14 @@
    +   * Whether this display is enabled or not. If the entity (form) display
    +   * is disabled, we'll fall back to the 'default' display.
    ...
    +  public $status;
    

    this already defined in 'ConfigEntityBase'

  3. +++ b/core/modules/system/tests/upgrade/drupal-7.field.database.php
    @@ -12,19 +12,19 @@
       'view_modes' => array(
    ...
    -      'status' => 1,
    +      'custom_settings' => 1,
    ...
    -      'status' => 0,
    +      'custom_settings' => 0,
    

    D7 has changed?

swentel’s picture

re: 3 no - in D7 it's custom_settings - we just never tested that variable :)

I'll fix the rest today

yched’s picture

1. needed because the patch adds a new entry in the annotation
3. see #54.6

andypost’s picture

1. is not needed because nothing is added to form and view modes

swentel’s picture

Status: Needs work » Needs review
FileSize
2.8 KB
37.81 KB

1. 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.

swentel’s picture

FileSize
6.45 KB
35.13 KB

New patch after yched's review at the con

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks ! Looks good if green

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/entity.inc
@@ -750,12 +751,20 @@ function entity_get_display($entity_type, $bundle, $view_mode) {
+    $entity_displays = \Drupal::service('config.storage')->readMultiple($ids);

@@ -841,12 +851,20 @@ function entity_get_form_display($entity_type, $bundle, $form_mode) {
+    $entity_form_displays = \Drupal::service('config.storage')->readMultiple($ids);

We should not be reading directly from the config storage here... in ConfigStorageController::buildQuery() we do the following:

    foreach ($this->configFactory->loadMultiple($names) as $config) {
      $result[$config->get($this->idKey)] = new $config_class($config->get(), $this->entityType);
    }

We need to get config through the factory to ensure we can use the overrides system.

swentel’s picture

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

Ok, 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.

yched’s picture

Works 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"

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 10b43b9 and pushed to 8.x. Thanks!

alexpott’s picture

Title: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable » Change notice: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable
Issue tags: +Needs change record

Oops - we need a change notification here.

aspilicious’s picture

Status: Fixed » Active
catch’s picture

Priority: Critical » Major
swentel’s picture

Status: Active » Needs review

Started change notification over at https://drupal.org/node/2104695

ianthomas_uk’s picture

Example Drupal 7 and Drupal 8 code for each of the uses mentioned would make that a lot clearer.

ianthomas_uk’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

Issue summary: View changes
Issue tags: +Missing change record

Tagging "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. :)

yched’s picture

Title: Change notice: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable » Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable
Status: Needs review » Fixed
Issue tags: -Needs change record, -Missing change record

Added code samples, and adjusted the change notice a bit.

Status: Fixed » Closed (fixed)

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