The TableField module has had a bunch of updates since Panopoly last updated it, including a new 3.x branch. The 3.x branch changes the back-end storage and includes hooks to manage the updates. This might be too much for Panopoly to take on right now, but we should at least update to release 2.5. The 2.5 update adds some accessibility and CSS fixes - including the ability to add a table caption.

[EDIT]: see comments #6 and #7, after successful preliminary testing it looks like updating to tablefield-7.x-3.1 is a reasonable proposition.

CommentFileSizeAuthor
#24 interdiff-2919401-24.txt807 bytesdsnopek
#24 panopoly_widgets-update-tablefield-2919401-24.patch4.68 KBdsnopek
#17 panopoly_test-update-tablefield-7.x-3.1-2919401-17.patch2.82 KBcboyden
#17 panopoly_widgets-update-tablefield-7.x-3.1-2919401-17.patch4.68 KBcboyden
#15 panopoly_widgets-update-tablefield-7.x-3.1-2919401-15.patch4.22 KBcboyden
#14 panopoly_widgets-update-tablefield-7.x-3.1-2919401-14.patch458 bytescboyden
#11 panopoly_widgets-update-tablefield-7.x-3.1-2919401-11.patch4.33 KBbyronveale
#8 panopoly_widgets-update-tablefield-7.x-3.1-2919401-8.patch4.32 KBcboyden
#5 panopoly_widgets-update_to_tablefield-7.x-3.1_for_testing-2919401-5.patch361 bytesbyronveale
#3 panopoly_widgets-update-tablefield-2919401-3.patch2.45 KBcboyden
#2 panopoly_widgets-update-tablefield-2919401-2.patch361 bytescboyden
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cboyden created an issue. See original summary.

cboyden’s picture

cboyden CreditAttribution: cboyden commented
Status: Active » Needs review
FileSize
panopoly_widgets-update-tablefield-2919401-2.patch361 bytes

Patch is attached.

cboyden’s picture

cboyden CreditAttribution: cboyden commented
FileSize
panopoly_widgets-update-tablefield-2919401-3.patch2.45 KB

The 2.5 version of Tablefield provides a new bit of configuration - the ability to hide headers. This is causing a PHP notice on the add widget screen. See #2796515: Undefined index: hide_headers in tablefield_field_widget_form() for a description of the error message. Just going to the field settings page and saving fixes the error, but it can also be fixed programmatically by updating the field base config.

The updated module also provides two config settings on the field formatter: trim_trailing_cols and trim_trailing_rows. In the current Panopoly version, table rows and columns that are completely empty are still rendered in the output. Leaving these config settings at "false" replicates the current behavior. It would be better to turn them on, as it will rationalize the tables a bit more and avoid the visual and accessibility issues that can be caused by empty rows/columns. So this patch updates those settings to turn on trimming. But if that's going too far we could rip that part out.

dsnopek’s picture

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

Here's a Travis build of the latest patch:

https://travis-ci.org/panopoly/panopoly/builds/323068629

byronveale’s picture

byronveale CreditAttribution: byronveale at Princeton University commented
FileSize
panopoly_widgets-update_to_tablefield-7.x-3.1_for_testing-2919401-5.patch361 bytes

Morbid (okay, not really) curiosity, and the red "Not supported!" message in the module updates list leads me to create this patch…

(Hiding so as not to confuse the issue, but wanted to be able to test this patch.)

byronveale’s picture

byronveale CreditAttribution: byronveale at Princeton University commented

FWIW, saw this on TableField's project page:

You are invited to try the D7 newest version. It is stable and provides an automated upgrade when coming from the 7.x-2.x version.

So 7.x-3.1 is working with rudimentary testing so far. If someone can point me in the direction of more targeted testing I could be doing, I'll be more than happy to do so…

byronveale’s picture

byronveale CreditAttribution: byronveale at Princeton University commented
Title: Update TableField to 2.5 at least » Update TableField to 7.x-3.1
Issue summary: View changes

I have successfully tested TableField 7.x-3.1 for both new and existing sites, so making the leap and "suggesting" we go all the way with this update. Changing title of this issue from "2.5 at least", un-hiding the latest patch.

cboyden’s picture

cboyden CreditAttribution: cboyden commented
FileSize
panopoly_widgets-update-tablefield-7.x-3.1-2919401-8.patch4.32 KB

The updates to tablefield involve adding permissions and moving configuration from field base to field instance, and from field settings to display settings. So the feature is going to be overridden and not fully configured unless panopoly_widgets also updates its own configuration. I've attached a patch that accounts for this, and also added an accessibility patch.

cboyden’s picture

cboyden CreditAttribution: cboyden commented
Assigned: cboyden » Unassigned
byronveale’s picture

byronveale CreditAttribution: byronveale at Princeton University commented

Super, thanks cboyden, will test…

byronveale’s picture

byronveale CreditAttribution: byronveale at Princeton University commented
FileSize
panopoly_widgets-update-tablefield-7.x-3.1-2919401-11.patch4.33 KB

Missing "=" in line #134; updated patch attached…

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
CreditAttribution: bkosborne at Princeton University commented
Status: Needs review » Reviewed & tested by the community

We've had the latest patch in production for a few weeks w/o issue.

dsnopek’s picture

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

I haven't tested this yet, but here some quick patch review:

  1. +++ b/panopoly_widgets.install
@@ -442,3 +446,14 @@ function panopoly_widgets_update_7020() {
+  $roles = user_roles(TRUE, 'configure tablefield');
+  foreach ($roles as $rid => $role) {
+    user_role_grant_permissions($rid, array('always use additional datasources'));
+  }

    This grants a new permission in the update hook, but will the permission get granted to the right roles on a fresh install? This patch doesn't appear to change the permissions in the feature 

  2. +++ b/panopoly_widgets.install
@@ -442,3 +446,14 @@ function panopoly_widgets_update_7020() {
+  features_revert(array('panopoly_widgets' => array('field_base', 'field_instance')));

    I'm not sure this is a good idea. This'll revert any customizations to any field that came from panopoly_widgets.

    Is maybe tablefield_update_7006() enough to convert the settings so that they match the feature? If not, some more targeted updates would be better.

Here's a new Travis build of the current patch:

https://travis-ci.org/panopoly/panopoly/builds/353063921

cboyden’s picture

cboyden CreditAttribution: cboyden commented
Status: Reviewed & tested by the community » Needs review
FileSize
panopoly_widgets-update-tablefield-7.x-3.1-2919401-14.patch458 bytes

The new permission is granted on install. (This feature doesn't include any permissions anyway.)

I've attached a new patch that removes the field reverts. The tablefield update seems to be enough to get the feature into its default state.

cboyden’s picture

cboyden CreditAttribution: cboyden commented
FileSize
panopoly_widgets-update-tablefield-7.x-3.1-2919401-15.patch4.22 KB

Sorry, wrong patch in the previous comment!

cboyden’s picture

cboyden CreditAttribution: cboyden commented
Status: Needs review » Needs work

After more testing, there are overrides in panopoly_widgets after updating tablefield:

$ drush fd panopoly_widgets
Legend:
Code: <      drush features-revert will remove the overrides.
Overrides: > drush features-update will update the exported feature with the displayed overrides

Component type: field_instance
          'settings' => array(
            'header_orientation' => 'Horizontal',
<           'sticky_header' => 1,
<           'striping' => 1,
<           'trim_trailing_cols' => 1,
<           'trim_trailing_rows' => 1,
---
>           'sticky_header' => TRUE,
>           'striping' => TRUE,
          ),
          'type' => 'tablefield_default',

The features export from Panopoly doesn't match the results of how Tablefield is updating its settings. And there are some missing. We'll need a targeted update hook to change the field's display settings.

Also, the table widget test needs updating.

cboyden’s picture

cboyden CreditAttribution: cboyden commented
Status: Needs work » Needs review
FileSize
panopoly_widgets-update-tablefield-7.x-3.1-2919401-17.patch4.68 KB
panopoly_test-update-tablefield-7.x-3.1-2919401-17.patch2.82 KB

Here's an updated patch that changes the field settings individually, and a patch to panopoly_test to update the table widget scenarios.

dsnopek’s picture

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

The new permission is granted on install. (This feature doesn't include any permissions anyway.)

Where is that granted on install? I don't see a tablefield_install().

This feature does have permissions! See panopoly_widgets.features.defaultconfig.inc

I'm actually a little confused about the update hook: it's looking for all roles with the 'configure tablefield' permission, and then giving the new permission to those roles - but we aren't explicitly granting 'configure tablefield' in Panopoly. Maybe this doesn't need to be done upstream in Panopoly?

dsnopek’s picture

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

Here's a new Travis build:

https://travis-ci.org/panopoly/panopoly/builds/353475377

cboyden’s picture

cboyden CreditAttribution: cboyden commented

The permissions are available to the administrator role on install. Not sure where they're coming from, but when I do a fresh install of Panopoly, there are three Tablefield permissions (four, after this patch) and they're all granted to administrators.

dsnopek’s picture

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

Ah, that could just be the fact that the admin role gets all permissions? That should happen even without needing to specifically grant them in an update hook too. So maybe that bit can be removed?

cboyden’s picture

cboyden CreditAttribution: cboyden commented

If the explicit grant isn't included in the update hook, the permission is not granted to the admin role. This will result in different permissions between a clean install of the new code and an updated site.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Needs work

If the explicit grant isn't included in the update hook, the permission is not granted to the admin role. This will result in different permissions between a clean install of the new code and an updated site.

Ok, I can confirm this in my own testing, but I think the fix is wrong. The only reason this permission is granted at all, is because all permissions get granted to the admin role. The current patch is giving the permission to every role with another permission, which could be a different list than just the admin role.

The reason we need to do something at all, is due to a core bug that new permissions aren't granted to the admin role when a code update to module adds new permissions, only when a new module is installed. So, I think the correct fix, is just to trigger the code that runs on module install in our update hook.

I'll post a patch that does this in a bit.

dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs work » Needs review
FileSize
panopoly_widgets-update-tablefield-2919401-24.patch4.68 KB
interdiff-2919401-24.txt807 bytes

Here's a new patch with the update hook changes. Otherwise everything seems fine and works in my limited manual testing. I'll commit it in a moment!

  • dsnopek committed fc266ef on 7.x-1.x 
    Update Panopoly Widgets and Test for Issue #2919401 by cboyden,...
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Fixed

Committed!

Status: Fixed » Closed (fixed)

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