Problem/Motivation

I'm considering changing something in Panelizer:

  • Panelizer saves a record for each panelized view mode for each entity of a panelized bundle. On some sites this can result in tens or hundreds of thousands of records.
  • Panelizer does not care if the values are the defaults, it just saves them.
  • Panelizer will automatically assign the default values if they are not already present in the entity object.
  • Panelizer provides a structure whereby a site can change the selected default for a bundle's view mode; if this is done, existing entities are not updated unless the site builder changes the value via a checkbox, or manually updates the database.

When this came up before I decided to save all records for all entities so that it was easier to ensure data was being saved correctly for revisions, but now I'm not so sure.

For comparison, Metatag does not save default values, those entities have their records generated during hook_entity_view().

Proposed resolution

Update hook_entity_save to not save default values for the entity bundle / view_mode combination.

Remaining tasks

Discuss it, write a patch to update the logic.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna’s picture

I think we should do this for v3.2.

DamienMcKenna’s picture

Title: META: Consider not saving records for default values » Don't save records for default values

The solution would be simple: in hook_entity_save and hook_entity_update, check if the to-be-saved $panelizer object matches the current default, if it does then don't bother saving.

nghai’s picture

Hey DamienMcKenna! I tried to follow your approach but while modifying the hook_entity_update I have encountered there are many cases for which the $panelizer->display_is_modified flag is not set. Would it be good to fetch the default object based on the current panelizer name and compare its all properties if any difference is there or not ?

DamienMcKenna’s picture

Status: Active » Needs review
FileSize
1.34 KB

Initial patch that changes hook_entity_insert and hook_entity_update to skip saving the records if they are the same as the default for the current view mode.

DamienMcKenna’s picture

Status: Needs review » Needs work

This needs an update script to remove existing records.

DamienMcKenna’s picture

To avoid timeouts, I think the update script should use a sandbox and in each loop delete records for one entity type, because on some sites there can be 10,000's of records.

maximpodorov’s picture

I'm tired to fight with Panelizer bugs and slowness. So the new project is arisen: Entity Panels. It it simple and quick, but its purpose is to panelize the whole view mode of entity bundle, not single entities. I hope it will help those who want a simple and working solution.

DamienMcKenna’s picture

@maximpodorov: Does Entity Panels provide anything that you don't get normally using Panels, i.e. can you customize each entity's displays individually?

maximpodorov’s picture

It panelizes the result of entity_view(). Every view mode is panelized separately. Each "page" (I don't know how to call it properly) can contain several variants with selection rules, so the particular entity can be panelized in it's own variant using entity id access rule.

ciss’s picture

Has this patch been tested? There are two calls to the function get_default_display_default_name(), which cause a fatal error on node_save(), since it should actually read $this->get_default_display_default_name().

DamienMcKenna’s picture

@ciss: The answer would be 'no' :)

ciss’s picture

@DamienMcKenna: I hope that didn't sound rude. I highly appreciate the work you're doing. :)
Referencing test coverage issue, as this seems to be a good candidate.

joegraduate’s picture

Status: Needs work » Needs review
FileSize
3.54 KB

The attached patch addresses the problems described in #9 and includes a largely untested implementation of the update function described in #6 (please don't test it on production websites).

The update function may be more complicated than it needs to be, particularly the code that determines which panelizer_entity records to delete. I hope this is useful.

joegraduate’s picture

Here is better version of the patch in #13 (this one actually works).

joegraduate’s picture

Here is a re-roll of #14 with a renamed update function that should apply cleanly to the latest dev.

DamienMcKenna’s picture

Status: Needs review » Needs work

Needs to be rerolled as update 7121 was added for another issue. Also, the update needs some watchdog() calls to log what's going on.

joegraduate’s picture

Status: Needs work » Needs review
FileSize
4.4 KB
2.09 KB

Ok, here is an updated patch per #16.

SocialNicheGuru’s picture

This fixed my duplicate entry issue:https://www.drupal.org/node/2337005

tobiberlin’s picture

#17 works when I do use "return;" instead of "continue;" -> here is no loop and you get errors when using "continue;" without any loop context.

joegraduate’s picture

Status: Needs review » Needs work
joegraduate’s picture

Status: Needs work » Needs review

@tobiberlin, I'm not seeing any problems like what you're describing with patch #17 applied. Also, I don't think we want to change those "continue;" lines into "return;" because those are, in fact, happening inside of foreach loops...

tobiberlin’s picture

I did not apply the patch - I just added these lines by hand into the current dev version from 25th April 2015 without the update function. May be this was the problem but I got an error claiming that this "continue" is used in a wrong way. Strange... Whatever - hopefully this commit will find it's way into the next stable version!?

fox_01’s picture

After applying #17 i have not to save all panelized entites after modifying a panelizer display. Great!

thePanz’s picture

After applying this patch, users are no more able to customize the "default" display for a specific node.
Edits are not saved.
Tested with 3.2-beta1 + this patch, should I use the latest dev+patch combination?

rcodina’s picture

Patch on #17 doesn't work for me, neither with changing "continue" with "return" as suggested in #19. It stills shows me this error:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'node-13226-35798-page_manager' for key 'PRIMARY': INSERT INTO {panelizer_entity} (entity_type, entity_id, revision_id, name, did, view_mode) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5); Array ( [:db_insert_placeholder_0] => node [:db_insert_placeholder_1] => 13226 [:db_insert_placeholder_2] => 35798 [:db_insert_placeholder_3] => node:basic_subpages:nivell_3 [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => page_manager ) in drupal_write_record() (line 7316 of /var/www/vhosts/coac.drautadev.com/httpdocs/includes/common.inc).

I'm back to 7.x-3.1 version.

zipymonkey’s picture

Patch in #17 prevents me from overriding the defaults using 'Save as custom'. Using panelizer version "7.x-3.2-beta1+56-dev".

GarChris’s picture

Whenever "Allow per-record display choice" is selected and I attempt to create a node I get the following error upon node save:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'node-2604-2640-default' for key 'PRIMARY': INSERT INTO {panelizer_entity} (entity_type, entity_id, revision_id, name, no_blocks, css_id, css, pipeline, contexts, relationships, did, view_mode, css_class, title_element, link_to_entity, extra) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7, :db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15); Array ( [:db_insert_placeholder_0] => node [:db_insert_placeholder_1] => 2604 [:db_insert_placeholder_2] => 2640 [:db_insert_placeholder_3] => node:product:default:default [:db_insert_placeholder_4] => 0 [:db_insert_placeholder_5] => [:db_insert_placeholder_6] => [:db_insert_placeholder_7] => standard [:db_insert_placeholder_8] => a:0:{} [:db_insert_placeholder_9] => a:0:{} [:db_insert_placeholder_10] => 0 [:db_insert_placeholder_11] => default [:db_insert_placeholder_12] => [:db_insert_placeholder_13] => H2 [:db_insert_placeholder_14] => 1 [:db_insert_placeholder_15] => a:0:{} ) in drupal_write_record() (line 7316 of /path/to/drupal/folder/includes/common.inc)

Using the latest dev module (7.x-3.2-beta1+56-dev)

Other modules enabled include: Domain access, entityreference, Commerce, pathauto, Hierarchical Select Taxonomy

japerry’s picture

Issue tags: +affects drupal.org

+1 on this -- I noticed that as we roll out our 12k documentation pages and endless project pages, it will make for a large panelizer table.

DamienMcKenna’s picture

Issue tags: +Needs tests
DamienMcKenna’s picture

Status: Needs review » Needs work

The last submitted patch, 30: panelizer-n2404999-30.patch, failed testing.

DamienMcKenna’s picture

Found a bug in the current logic - it isn't letting you override the display, any changes are not saved.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
8.82 KB
4.66 KB

The tests pass!

DamienMcKenna’s picture

  • DamienMcKenna committed 314e177 on 7.x-3.x
    Issue #2404999 by DamienMcKenna, joegraduate: Don't save {...
DamienMcKenna’s picture

Status: Needs review » Fixed

Committed.

zuernBernhard’s picture

Thats a problem for me right now. We use panelizer context to assign custom images to panelized nodes. So we have rows in the panelizer_entity Table which have context but did=0. With the current update all these rows are gone. Perhaps its possible to have a look at the context when the code decides wether a panelizer_entity record is "default" or not ?

DamienMcKenna’s picture

@zuernBernard: The contexts are overridden per node?

zuernBernhard’s picture

FileSize
40.57 KB

Yes we have custom contexts for the panelized nodes (like node 852 in the example)

Screenshot

These records have no Display-ID (did=0)

FeyP’s picture

Unfortunately, I also got a problem, that seems to be related to this change.

I've got a node bundle with "Full page override" set to panelized. Substitute view mode is set to "Ignore this option". "Provide initial display" and "Allow panel choice" are active. In the panel list, I've got the default panel and then an additional panel. The "Default panel" is set to the default panel from the panel list. If I edit a node that is currently using the additional panel and set the panel choice back to the default panel, the change is not saved.

I tried to debug the issue and it seems that the problem is in PanelizerEntityDefault::hook_entity_update(). The new display fails the check in line 1711, since $panelizer->display_is_modified is not set and the name of my display is of course equal to the default display's name. Thus, my view mode is skipped and nothing is saved to the database. Out of curiosity, I added $entity->panelizer[$view_mode]->display_is_modified = TRUE in PanelizerEntityDefault::hook_field_attach_submit(), but then the node is marked as overridden and uses the original display. However, I can then use the reset link on the "Customize display" tab to get back to the default display.

DamienMcKenna’s picture

@zuernBernhard: Any time you make any changes to any Panelizer configuration for an individual entity it should create a new panels_display record, so what you've been seeing is actually a bug I'll tackle in #2758561: Ensure any per-entity changes create a new panels_display. Out of interest, what version of Panelizer were you using before you updated?

@FeyP: Ugh :-\ I've opened #2758569: Changing an entity's default display may not be working correctly and will fix that before 3.2 is released.

DamienMcKenna’s picture

If anyone else experiences problems with this new architecture please open a new issue and I'll work on it.

DamienMcKenna’s picture

@zuernBernhard: Ok, I reviewed your screenshot again, and the record for node 852 is completely baffling. There's no default configuration selected, it didn't save a new display, but somehow it still overrode the context? This update script wouldn't delete records like that, but there may be an issue where it's not loading those {panelizer_entity} records because I never thought what you're seeing would be possible.

Status: Fixed » Closed (fixed)

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