With the addition of the new defaults UI, there should no longer be a need for the custom field to be manually added to an entity type. It seems that the field is now created automatically per supported entity type (right now just nodes). We need to a) block the field from being addable via the UI, and b) make sure there's an upgrade path.

CommentFileSizeAuthor
#12 panelizer-n2835590-12.patch1.35 KBDamienMcKenna
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

DamienMcKenna created an issue. See original summary.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Related issues: +#2664682: Implement admin UI for editing Panelizer defaults
EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

No, because the field tracks:

1.) Custom override level data in the situations where we perform that operation
2.) WHICH default this entity is using in the case that bundle allows layout selection.

If layout selection is not enabled AND customization is not enabled, then in that case I don't believe we would even deploy the panelizer field as it currently stands for all the reasons you suggested in the OP.

Eclipse

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

The question then becomes: why do we need the custom field if the custom table can store exactly the same data, even the same columns? Is there any benefit to it that couldn't be moved into the custom table?

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc commented

I'm confused. The reason we implemented it as a field is to get native revision control for free. It'll also make language easier when we get that squared away. What are you proposing we do instead?

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

The reason we implemented it as a field is to get native revision control for free.

That's one of the reasons, but there's more!

  • By being a field, it's stored on whatever entity storage backend is used by the entity type. So, if, for example, a site used MongoDB or just an optimized DB backend, Panelizer would use it too
  • In D8, the only real way to get arbitrary properties on an entity is as field. Of course, we could have a separate API call to lookup Panelizer data for an entity, but this allows us to hang the data right on the entity
  • In D8 with almost everything covered by entities or config, it feels like making custom tables is becoming the way of the past

That said, I'm also caught a little off guard about the field be required now. In my original implementation it was only necessary for per-entity customizations. But what EclipseGC writes #3.2 makes sense, and I don't really see any way around it (without adding the field as soon as there is more than one default, but that seems... less good).

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

I understand the benefits of adding a field, the problem is, though, that it doesn't seem to actually be used?

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Title: Remove the Panelizer field? » Remove/hide the Panelizer field?
Issue summary: View changes
Parent issue: » #2838104: Plan for Panelizer 8.x-3.0 release

The issue may be that the field is not supposed to be exposed to be added, in which case we just need a way of stopping the field from being manually added.

japerry’s picture

japerry
Location KVUO
CreditAttribution: japerry commented
Priority: Normal » Minor

Removing it from the UI would be nice, but I don't think its a priority and also is not a blocker to a release.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Even if it's just like a 5 line hook_form_alter(), I think we should put something in for the 3.0 release.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Issue summary: View changes
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Active » Needs review
Parent issue: #2838104: Plan for Panelizer 8.x-3.0 release » #2834847: Plan for Panelizer 8.x-3.0-beta1
FileSize
panelizer-n2835590-12.patch1.35 KB

This hides the field from field_ui_field_storage_add_form().

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

I just thought of something - do we need an update script to migrate data away from any custom fields that people might have added into the correct tables?

  • DamienMcKenna committed ef6b0f6 on 8.x-3.x 
    Issue #2835590 by DamienMcKenna: Hide the custom Panelizer field, it's...
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Title: Remove/hide the Panelizer field? » Hide the custom Panelizer field
Status: Needs review » Fixed

Committed.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

I created a new issue to further work on removing the field, which can be done later: #2841710: Remove the custom Panelizer field from Field API

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

I created a new issue to deal with any legacy data that might exist: #2841712: Move data over from any custom Panelizer fields to the new entity data structures

Status: Fixed » Closed (fixed)

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