Now that #1822458: Move dynamic parts (view modes, bundles) out of entity_info() landed in Drupal 8, I wonder if it would makes sense for us to "backport" the separate hooks for entity view modes, so that they could be re-used for features exports, and also be forward-compatible with Drupal 8.

CommentFileSizeAuthor
#14 1900676-hook-entity-view-mode-info.patch9.01 KBdave reid
#13 1900676-hook-entity-view-mode-info.patch4.99 KBdave reid
#12 1900676-hook-entity-view-mode-info.patch4.68 KBdave reid
#6 1900676-hook-entity-view-mode-info-6.patch5.28 KBAnonymous (not verified)
#5 1900676-hook-entity-view-mode-info-5.patch5.3 KBAnonymous (not verified)
#3 1900676-hook-entity-view-mode-info.patch4.66 KBdave reid
#1 1900668-add-hook-entity-view-mode-info.patch2.37 KBdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 1900668-add-hook-entity-view-mode-info.patch2.37 KB
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Active » Needs review
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 1900676-hook-entity-view-mode-info.patch4.66 KB

Revised patch adding an api.php, removing unwanted code from features support, and refining comments in entity_view_mode_entity_info_alter() about precedence of view modes.

Anonymous’s picture

Anonymous (not verified) commented 
+++ b/entity_view_mode.moduleundefined
@@ -15,9 +31,21 @@ function entity_view_mode_permission() {
+  // Add in the variable entity view modes which override hook-provided.
+  $view_mode_info = variable_get('entity_view_modes', array()) + $view_mode_info;

This is definitely problematic. If you have view modes for nodes defined in the new hooks and in the entity_view_modes variable, the ones defined in hooks are clobbered entirely and only the ones from the variable are included.

I was able to get better results with array_merge_recursive, but when I deliberately set up some wrong things -- a view mode defined in a hook and in the variable -- other weirdness showed up.

I can fix this. I'll post a patch shortly.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 1900676-hook-entity-view-mode-info-5.patch5.3 KB

Here's a patch which uses hook_entity_view_mode_info_alter() to populate the view mode array with the user-defined view modes from the system variable.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 1900676-hook-entity-view-mode-info-6.patch5.28 KB

Here's one without debug code.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

This is definitely problematic. If you have view modes for nodes defined in the new hooks and in the entity_view_modes variable, the ones defined in hooks are clobbered entirely and only the ones from the variable are included.

Did you actually test this though? It does a merge by using the +. It works exactly like features would expect, view modes defined in the variable override and and merged into the existing view modes defined in the hooks.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Before we get too far as well, it would just be good to figure out if these new hooks make sense and if they're a good idea to begin with. Note that I had to add a notice on the documentation for the hooks that these in no way can cause anything that would invoke hook_entity_info(). Maybe it would be good to add recursion protection against entity_view_mode_entity_info_alter() to prevent it from happening?

Anonymous’s picture

Anonymous (not verified) commented

Did you actually test this though? It does a merge by using the +. It works exactly like features would expect, view modes defined in the variable override and and merged into the existing view modes defined in the hooks.

I did test it, and I found that my resulting view modes were empty.

It was trying to merge a complete view mode definition array (like what the info hook would return) with this which was stored in my entity_view_modes variable.

array(
  'node' => array(),
  'taxonomy_term' => array(),
);
dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Ok good catch and confirmed. I'm still a little tentative about entity_view_mode implementing its own alter hook. I guess I wouldn't expect the variable view modes to be defined yet in my own module's hook_entity_view_mode_info_alter(). I guess I would liken it to CTools exportables: you have exportables which have their own hooks and alter hooks, and then you have the database 'overrides' which are separate. A module that provides CTools exportables doesn't implement hook_mymodule_default_items_alter() and pull in all the database entries.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

I think this can be solved using drupal_array_merge_deep()...

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 1900676-hook-entity-view-mode-info.patch4.68 KB

Let's try this one.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 1900676-hook-entity-view-mode-info.patch4.99 KB

Adding protection against entity_get_info() recursion.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
StatusFileSize
new 1900676-hook-entity-view-mode-info.patch9.01 KB

Test with patches

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Fixed

Committed #14 to 7.x-1.x. http://drupalcode.org/project/entity_view_mode.git/commit/4264c39

Status: Fixed » Closed (fixed)

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