Split off from #1536944: Allow panes to be translated. There is code in those patches to convert the 'path' property to a field so that it can be translated with entity_translation. It made the base patch harder to review since it had not-pretty upgrade path and more complex code.

Comments

Dave Reid’s picture

Status:Postponed» Active

#1536944: Allow panes to be translated has been committed, so un-postponing this.

mrfelton’s picture

Status:Active» Needs review
StatusFileSize
new4.18 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1860486-fieldable_panels_pane-path-field.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Initial patch that pulls the code out from the original patches in #1536944: Allow panes to be translated in order to handle conversion of the path property to a field, which can then be translated properly.

mrfelton’s picture

Status:Needs review» Needs work
StatusFileSize
new4.4 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1860486.3-fieldable_panels_pane-path-field.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch updated to include (crappy) support for the title field from #1860484: Add support for Title module field. This needs work, but does the job for my current use case.

mrfelton’s picture

Status:Needs work» Needs review
StatusFileSize
new8.24 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1860486.4-fieldable_panels_pane-path-field.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Seems none of the previous patches had the update or install hooks for some reason. Patch updated.

DamienMcKenna’s picture

Assigned:Unassigned» Dave Reid
Issue summary:View changes

@Dave Reid: Given your experience with Pathauto and other path-related modules, do you think this is the correct approach?

DamienMcKenna’s picture

DamienMcKenna’s picture

Status:Needs review» Needs work
DamienMcKenna’s picture

Status:Needs work» Needs review

Testbot, do your thing.

The last submitted patch, 2: 1860486-fieldable_panels_pane-path-field.patch, failed testing.

The last submitted patch, 3: 1860486.3-fieldable_panels_pane-path-field.patch, failed testing.

dsnopek’s picture

Status:Needs review» Needs work

Looking at the patch, the hook_install() is creating the field and instance, but that only happens initially when the module is first installed. The hook_update_N() is moving the data from the column on 'fieldable_panels_panes' to the new field, but it isn't creating the field and instance!

Also, when users add a new FPP type via the UI, it doesn't appear to be creating the new instance (unless I'm missing it somewhere).

Marking as "Needs work"!

The last submitted patch, 4: 1860486.4-fieldable_panels_pane-path-field.patch, failed testing.

DamienMcKenna’s picture

Removing this from the schedule for 1.6.

t0xicCode’s picture

Priority:Normal» Major

The target of the link being saved as a property named path has an unintended consequence when #936222: Merge in pathauto_persist module functionality to prevent losing manual aliases with node_save() calls is also applied: fieldable_panels_panes cannot load the value of the path property, as pathauto transforms it to an array and adds its own keys to it.

This is because pathauto expects the path property of an instance to be the path to the instance, as defined in the core path module. However, FPP uses that property to signify the target of the link. Moving this to a field would prevent pathauto (with that patch) from clobbering the field on entity load.

t0xicCode’s picture

Status:Needs work» Needs review
StatusFileSize
new28.71 KB
PASSED: [[SimpleTest]]: [MySQL] 0 pass(es).
[ View ]

I believe this patch properly sets up and converts the path property to a path field.

Dave Reid’s picture

The patch in #15 would be a lot easier to review without all the unnecessary whitespace and alignment changes.