Store view modes in an entity_view_mode table (compatible with Display Suite?)

I think using the same table can be quiet a problem with installation/desinstallion/update. But using a new table with the same structure is a first step. DS can after add this module as a dependency.

As great as it would be, there are huge red flags with depending on another module's schema without actually declaring yourself as a dependency, so I'm not ready to take that risk yet.

i agree with a {entity_view_mode} table

Re-titling for better visibility.

If we used a table we possibly couldn't use ctools exportables as we possibly cannot rely on things that need database schemas in hook_entity_info(). We'd have to manually maintain that code for the UI and export/import handling, which is not ideal at all. Unless it can be proven that it would be ok to use ctools exportables and schema in hook_entity_info() for Drupal 7, I'm very tenative on this solution.

Is there a specific reason why using variables is not acceptable? Most sites would only need a few variables.

Put another way, could DS be updated to use the same variables?

Two comments:

• Shouldn't the view modes be stored per entity bundle?
• Shouldn't the separator be two underlines?

Patch failed because we need to delete the old record when changing machine_name.

I wasn't able to apply the patch, git reported fatal: unrecognized input

However, just looking at the patch, it doesn't add a .install file to update existing view modes to use the db, it doesn't add a schema to add the new table. So I don't think it does what it is supposed to do.

Sry about that. Try this one. As for the unrecognized input likely an issue with line endings. Should be fixed.

I can't use the patch:

patch -p1 < 1702530-17.entity_view_mode.table-storage.patch
patch unexpectedly ends in middle of line
patch: **** Only garbage was found in the patch input.

LOL I have such a hard time with windows providing good patches.... Here is an updated. It applied cleanly on my linux box. I should have known with the difference in filesize.

I can confirm that the patch applies cleanly.

The last failing test is due to a test that checks if the functionality still works when a manual variable_set() is used to update entity_view_mode. I replaced it with a direct db update test. Not sure if we want this or not but let see if it works!

Hmm. This is my last attempt at blindly swinging before I need to setup a testing environment on this box.

Maybe simpletest doesn't like drupal_write_record? Give this a quick try.

I am not sure what is failing here. I will have to check into it when I get in a testing environment.

It looks like the variable_set call and the db_update call are not the same. You can see there is 'taxonomy_term' being set with an empty array in the variable set. I am not sure what that is doing, but it might be why these are not working.

This is what I get for commenting, before I read the testbot fail message. The labels don't match.

Value array ( 'label' => 'Hook-defined view mode #3', 'custom settings' => false, ) is identical to value array ( 'label' => 'DB-altered view mode', 'custom settings' => true, ).

The patch doesn't label things consistently.

Before I start changing these test, I want to make sure this is working. So I will begin some manual testing to confirm the test is wrong.

I was just about to make the mistake of assuming that the test was wrong, so just change the test.

I think I found it.

There is a problem somewhere with the custom settings. It likely has to do with "custom_settings" vs "custom settings"

What I have noticed is the custom settings being shown don't reflect what they are for custom db view modes.

So I worked on this a bit, it still needs more work.

The module is no longer updating the field settings when the view mode is updated and visa versa. So more work to be done.

I have attached an interdiff so you can see what I changed. We have enough patches that we should start to do interdiffs.

Updated the summary.

Hmm. I don't believe #39 is correct. I believe entity_get_info() returns "custom settings" not "custom_settings" however we have to store the data in the database with the underline. I think your patch may actually cause issues on the admin page. As you are setting "custom_settings" to a value that doesn't exist.

@dobe #39 was meant to address the admin pages being broken with the patch from #32.

The patch mentioned in #42 should fix the test issues, However, the admin will still be broken with the previous patch. This is because the module used to store everything with no underscore (this is how core field api needs it). When saving the module would change custom_settings into "custom settings"

My patch isn't right. that is why the issue is still marked as needs work. We need to figure out what we are going to do to keep everything in sync. I am thinking we need to leave the admin ui clean with custom_settings as it is in the DB and add some mapping from custom_settings into "custom settings" in the field_entity_view_mode_* functions. Keep the core specific conversions in the core portion and leave everything else as it is. Maybe we should just add a couple to/from mapping functions.

function _entity_view_mode_reformat_to_core_format($entity_view_mode) {
  // do conversion
  return $entity_view_mode;
}

function _entity_view_mode_reformat_from_core_format($entity_view_mode) {
  // do conversion
  return $entity_view_mode;
}

This patch should pass tests. I removed the test to check the hooks after variable_set since we are not using it anymore and I don't really believe we need to test if the functionality works by modifying the database directly (not even sure simpletest supports this type of test).

This patch may also address https://www.drupal.org/node/2508313 by setting "custom settings" when we load from the db. I feel doing it here may be better as it makes sure that if someone uses those functions they shouldn't' have to do the conversion in there code. We also call one of the load functions in entity_view_mode_entity_info_alter().

This also solves the issue on the admin pages without modifying the admin file.

The test that was set for testing after a variable set was testing view modes that where created in the ui. Otherwise, all that is being tested is the hook_implemented view modes. We need to still test the view modes that are created with the ui. I don't see how this could address #2508313: hook_entity_view_mode_info() 'custom_settings' property is not being respected.

Have you done any manual tests to make sure this patch works?

The test that was set for testing after a variable set was testing view modes that where created in the ui. Otherwise, all that is being tested is the hook_implemented view modes. We need to still test the view modes that are created with the ui.

The test that was removed has NOTHING to do with view modes created through the UI. Those tests are passing, they all happen in: function testEntityViewModes() which I did not have to modify (the first 58 tests, test the UI). I modified testInfoHooks() which most of the tests are still there. I only removed the one test that was used to test that the info hooks still worked after variable_set().

Have you done any manual tests to make sure this patch works?

I have done manual tests. Everything seems functional. But that is also where the community comes in.

I don't see how this could address #2508313: hook_entity_view_mode_info() 'custom_settings' property is not being respected.

The current patch does what #2508313 does in entity_view_mode_load() and entity_view_mode_load_all() now instead of entity_view_mode_entity_info_alter().

testing with new patch from #44

The admin UI is broken.

From admin/config/system/entity-view-modes the user cannot set the view mode custom settings.

Also, the admin table column for showing if the Display mode has custom settings doesn't reflect the settings that are in the DB.

These are the things that I fixed in #39. I will not be submitting another patch any time soon, but I can still review your patches if you fix these bugs.

I updated the issue summary with things that are done and need to be done.

Hmm. For some reason the unset in the save function showed back up which the first patch in this que removed... Not sure how that happened.

This fix the admin UI

I queued a new test of #56. For some reason the last patch tested was an old one I wrote in #39.

I installed #56 locally and tested it adding a custom view mode, and looking at the presence of it in the database. I verified the view modes functionality within the content type structure interface, and deleted it.

Is this going to be included in a next release?