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.
Comment | File | Size | Author |
---|---|---|---|
#39 | Auswahl_003.png | 40.57 KB | zuernBernhard |
#34 | panelizer-n2404999-34.interdiff.txt | 433 bytes | DamienMcKenna |
#34 | panelizer-n2404999-34.patch | 6.05 KB | DamienMcKenna |
| |||
#15 | panelizer-no-records-default-values-2404999-15.patch | 3.7 KB | joegraduate |
#14 | interdiff.txt | 635 bytes | joegraduate |
Comments
Comment #1
DamienMcKennaI think we should do this for v3.2.
Comment #2
DamienMcKennaThe 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.
Comment #3
nghai CreditAttribution: nghai commentedHey 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 ?
Comment #4
DamienMcKennaInitial 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.
Comment #5
DamienMcKennaThis needs an update script to remove existing records.
Comment #6
DamienMcKennaTo 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.
Comment #7
maximpodorov CreditAttribution: maximpodorov commentedI'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.
Comment #8
DamienMcKenna@maximpodorov: Does Entity Panels provide anything that you don't get normally using Panels, i.e. can you customize each entity's displays individually?
Comment #9
maximpodorov CreditAttribution: maximpodorov commentedIt 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.
Comment #10
ciss CreditAttribution: ciss commentedHas 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().
Comment #11
DamienMcKenna@ciss: The answer would be 'no' :)
Comment #12
ciss CreditAttribution: ciss commented@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.
Comment #13
joegraduateThe 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.
Comment #14
joegraduateHere is better version of the patch in #13 (this one actually works).
Comment #15
joegraduateHere is a re-roll of #14 with a renamed update function that should apply cleanly to the latest dev.
Comment #16
DamienMcKennaNeeds to be rerolled as update 7121 was added for another issue. Also, the update needs some watchdog() calls to log what's going on.
Comment #17
joegraduateOk, here is an updated patch per #16.
Comment #18
SocialNicheGuru CreditAttribution: SocialNicheGuru commentedThis fixed my duplicate entry issue:https://www.drupal.org/node/2337005
Comment #19
tobiberlin#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.
Comment #20
joegraduateComment #21
joegraduate@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...
Comment #22
tobiberlinI 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!?
Comment #23
fox_01 CreditAttribution: fox_01 commentedAfter applying #17 i have not to save all panelized entites after modifying a panelizer display. Great!
Comment #24
thePanz CreditAttribution: thePanz at Liip for FREITAG lab. AG commentedAfter 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?
Comment #25
rcodina CreditAttribution: rcodina commentedPatch 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.
Comment #26
zipymonkey CreditAttribution: zipymonkey commentedPatch in #17 prevents me from overriding the defaults using 'Save as custom'. Using panelizer version "7.x-3.2-beta1+56-dev".
Comment #27
GarChris CreditAttribution: GarChris commentedWhenever "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
Comment #28
japerry+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.
Comment #29
DamienMcKennaComment #30
DamienMcKennaReroll.
Comment #32
DamienMcKennaFound a bug in the current logic - it isn't letting you override the display, any changes are not saved.
Comment #33
DamienMcKennaThe tests pass!
Comment #34
DamienMcKennaBumping the update script to 7300.
Comment #36
DamienMcKennaCommitted.
Comment #37
zuernBernhard CreditAttribution: zuernBernhard as a volunteer and at UEBERBIT GmbH commentedThats 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 ?
Comment #38
DamienMcKenna@zuernBernard: The contexts are overridden per node?
Comment #39
zuernBernhard CreditAttribution: zuernBernhard at UEBERBIT GmbH commentedYes we have custom contexts for the panelized nodes (like node 852 in the example)
These records have no Display-ID (did=0)
Comment #40
FeyP CreditAttribution: FeyP at werk21 commentedUnfortunately, 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
inPanelizerEntityDefault::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.Comment #41
DamienMcKenna@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.
Comment #42
DamienMcKennaIf anyone else experiences problems with this new architecture please open a new issue and I'll work on it.
Comment #43
DamienMcKenna@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.