Support from Acquia helps fund testing for Drupal Acquia logo

Comments

helior’s picture

Status: Needs work » Active
FileSize
25.4 KB
cangeceiro’s picture

Status: Active » Needs work

I like the idea, but it looks like this patch breaks the menu hierarchy that previously exists. We also don't want to remove the ability for developers to create bundles in code, nor do we want to remove that from the documentation.

helior’s picture

Status: Active » Needs review

This patch doesn't necessarily remove the ability to create bundles in code, it just complains about it in the Status Report page because it isn't a sustainable approach. With this UI, the existing documentation becomes irrelevant since it always was, after all, a temporary solution. The change in the menu hierarchy was made for semantics, as well as to reduce the depth by one.

This patch also includes an upgrade path that will automatically save previously defined bundles into the database. So as long as update.php is ran, there should be no hiccups.

cangeceiro’s picture

Status: Needs review » Needs work

after applying the patch I no longer see the panels entities, and the path 'admin/structure/panels/fieldable-panels-panes' and 'admin/structure/panels/fieldable-panels-panes/add' which is represented in the documentation do not exist. The hook_requirements isnt necessary either. If a developer wants to create a bundle in code they should be allowed to with out getting a warning. There is nothing wrong with that approach. This patch needs work.

helior’s picture

Did you not run update.php? That's kind of a requirement ;)

There is a problem with providing bundles in code, actually. What happens when you configure fields to your bundle, but then later that code that provides the bundle is changed/removed or that module is simply disabled? You are then left with stray field tables since no post-processing was done to run garbage collection.

It would be great if you can hop on IRC so we can discuss this in real-time.

EDIT: In case you didn't notice, this patch depends on #1618264: Improved DX for Export UI -- That /might/ be why your paths aren't showing up.

cangeceiro’s picture

yes, i ran update, im not a fool. in regards to the ctools patch, thats merlins domain. so i can't really go any further on that until he decides whether or not he wants to commit it to ctools. i'd hop on irc right now, but i gotta pack up for the night. if you are around in #drupal or #drupal-support tomorrow we can chat though.

helior’s picture

Status: Needs work » Postponed

I'll make sure I'm available, thanks!

helior’s picture

helior’s picture

FileSize
29.68 KB

Re-rolling patch to accommodate changes in HEAD. This still depends on #1618264: Improved DX for Export UI

helior’s picture

Issue summary: View changes

Updated issue summary.

helior’s picture

Status: Postponed » Needs review
FileSize
29.92 KB

All the dependent patches have been committed; this patch is now ready for review!

Requires the latest dev snapshot from CTools

Here's an updated patch against HEAD so far..

helior’s picture

Priority: Normal » Major

Changing the priority to major –- this feature is kind of a big deal.

merlinofchaos’s picture

I think we'll want that feature to be in a CTools release before we can feasibly commit it. Unless the feature itself simply won't show up without it, but I don't want something that'll go crashy crashy if things aren't in sync.

jwilson3’s picture

Status: Needs review » Needs work
/*
 * Add Fieldable Panels Panes Type table.
 * Save existing bundles to database.
 */
function fieldable_panels_panes_update_7106(&$sandbox) {

A nitpick, but there's a missing "*" from the update script function comment that causes drush to not detect the message while running updates:

# drush updatedb
The following updates are pending:

fieldable_panels_panes module : 
  7106 - 

Additionally, the comment for the update script could be shortened into a single-line comment like "Improve bundle management to allow CTools exportability."

jwilson3’s picture

Also, I can concur that the same thing happens to me as with comment #4:

after applying the patch I no longer see the panels entities, and the path 'admin/structure/panels/fieldable-panels-panes' and 'admin/structure/panels/fieldable-panels-panes/add' which is represented in the documentation do not exist. The hook_requirements isnt necessary either.

For me, not only admin/structure/panels/fieldable-panels-panes doesn't exist, but neither does admin/structure/fieldable-panels-panes (which is the path linked from the Panels Dashboard). The "Fieldable Panels Panes" item has disappeared from the list on admin/structure page.

Using latest snapshots of ctools-7.x-1.x-dev & fieldable_panels-panes-7.x-1.x-dev plus patch from #10.

jwilson3’s picture

ctools_export_crud_load_all('fieldable_panels_panes_type') is returning an empty array.

Note that I didn't have any custom fieldable pane types, other than the default one provided by the module before applying the patch and running the update.

jwilson3’s picture

It appears (and i could be way off) that the issue is a mix between a) an update script gone wrong, not creating a "default" bundle provided by the module… and b) a hook_menu implementation issue where the admin page doesnt even exist, because there wasnt a default in the database which is needed in order to build the menu structure... there is too much CTools menu voodoo going on for me to understand whats wrong, but I assume fixing A will solve B.

acrollet’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
29.45 KB

I tried out the patch in #10 and had some issues with it - chiefly that features-diff did not seem accurate, and the file in the exported feature was named fpp2..inc. Patch attached resolves these issues, and works for me to create features and revert them. I'll attach a truncated interdiff, the other difference between my patch and the patch in #10 is that I've removed the function wrapping the plugin definition in plugins/export_ui/fieldable_panels_pane.inc.

This patch also addresses #13 and (I believe) #15. The issue in #14 is resolved by clearing caches after applying the patch.

jwilson3’s picture

Status: Needs review » Needs work

Patch in #16, does indeed correct the missing default bundle, however I'm still missing the menu item for "Fieldable Panels Panes" under the Admin > Structure page/menu.

Am getting a whole slew of errors... tested both after running the update script (after patching a pre-installed dev snapshot), as well as running from a clean version with no prior installation (enabling the module dev snapshot directly with patch already applied).

Warning: Missing argument 2 for ctools_export_ui_load(), called in .../includes/menu.inc on line 592 and defined in ctools_export_ui_load() (line 666 of .../ctools/ctools.module).
Notice: Undefined variable: plugin_name in ctools_export_ui_load() (line 671 of .../ctools/ctools.module).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 376 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 388 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 389 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: schema in ctools_export_ui->load_item() (line 47 of .../ctools/plugins/export_ui/ctools_export_ui.class.php).
Notice: Undefined index: export in ctools_export_crud_load() (line 75 of .../ctools/includes/export.inc).
Warning: Missing argument 2 for ctools_export_ui_load(), called in .../includes/menu.inc on line 592 and defined in ctools_export_ui_load() (line 666 of .../ctools/ctools.module).
Notice: Undefined variable: plugin_name in ctools_export_ui_load() (line 671 of .../ctools/ctools.module).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 376 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: schema in ctools_export_ui->load_item() (line 47 of .../ctools/plugins/export_ui/ctools_export_ui.class.php).
Notice: Undefined index: export in ctools_export_crud_load() (line 75 of .../ctools/includes/export.inc).
Warning: Missing argument 2 for ctools_export_ui_load(), called in .../includes/menu.inc on line 592 and defined in ctools_export_ui_load() (line 666 of .../ctools/ctools.module).
Notice: Undefined variable: plugin_name in ctools_export_ui_load() (line 671 of .../ctools/ctools.module).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 376 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: schema in ctools_export_ui->load_item() (line 47 of .../ctools/plugins/export_ui/ctools_export_ui.class.php).
Notice: Undefined index: export in ctools_export_crud_load() (line 75 of .../ctools/includes/export.inc).
Warning: Missing argument 2 for ctools_export_ui_load(), called in .../includes/menu.inc on line 592 and defined in ctools_export_ui_load() (line 666 of .../ctools/ctools.module).
Notice: Undefined variable: plugin_name in ctools_export_ui_load() (line 671 of .../ctools/ctools.module).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 376 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: schema in ctools_export_ui->load_item() (line 47 of .../ctools/plugins/export_ui/ctools_export_ui.class.php).
Notice: Undefined index: export in ctools_export_crud_load() (line 75 of .../ctools/includes/export.inc).
Warning: Missing argument 2 for ctools_export_ui_load(), called in .../includes/menu.inc on line 592 and defined in ctools_export_ui_load() (line 666 of .../ctools/ctools.module).
Notice: Undefined variable: plugin_name in ctools_export_ui_load() (line 671 of .../ctools/ctools.module).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 376 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: name in ctools_export_ui_get_handler() (line 392 of .../ctools/includes/export-ui.inc).
Notice: Undefined index: schema in ctools_export_ui->load_item() (line 47 of .../ctools/plugins/export_ui/ctools_export_ui.class.php).
Notice: Undefined index: export in ctools_export_crud_load() (line 75 of .../ctools/includes/export.inc).
helior’s picture

FileSize
30.5 KB

@jwilson3 the symptoms you're describing in #14 and #15 indicate that update.php was not ran correctly on your end. I just tested this locally on a clean install to confirm the upgrade runs properly and everything worked as expected. I would encourage you to go ahead and try the upgrade again from a clean Drupal install – I'm pretty sure it was just a quirk on your local install.

You're right about the asterisk typo you mentioned in #13, in fact, there were a few other comment-related typos in that same document, even from older code. As an aside, there isn't anything inherently wrong with having update hook comments span multiple lines, but I'm not opposed to the change.

Fyi, the path "admin/structure/panels/fieldable-panels-panes" was moved to "admin/structure/fieldable-panels-panes" as of 32f71fe. Check out #1607028: Entity base path is too deep.

@acrollet adding the 'api' key on the export array corrected that glitch on the filename for Features export -- But to be clear, this is actually a bug in Features, see #1155310: Problem exporting data without a ctools "api" string Furthermore, most of your other changes in #17 aren't actually fixing anything:

- changing the export ui plugin definition to the shortcut $plugin notation is unnecessary. In fact, it will make it particularly difficult to maintain, given the complexity of the definition, since when using the shortcut notation we have to be extremely careful not to accidentally clobber any variables that are used in ctools_plugin_load_includes() (plugin files are loaded directly into that function); having to be mindful of this is a burden to developers, and I would argue that only very simple plugins should use the $plugin shortcut notation.
- 'key name' on export array should be a label to the unique identifier field, not a description. I left this key out since the default that CTools provides is appropriate (Name).
- 'primary key' on the export array is also unnecessary, since this is automatically derived from the schema.

This latest patch takes the recommendation from #13, as well as the 'api' key on the export included in the patch in #17, but not the aforementioned items.

helior’s picture

Status: Needs work » Needs review
acrollet’s picture

@helior - thanks for the clarifications, I haven't worked with export_ui before and was doing the old 'change something and see if it works'. I'll review this patch.

acrollet’s picture

patch in #19 works nicely for me, the feature filename bug is no longer present and creating/diffing features works nicely. I found getting an empty screen on the 'list' operation page when no entities have been created to be somewhat disconcerting, so I made a minor modification to the patch to provide output for the user:

> --- a/plugins/export_ui/fieldable_panels_pane.class.php
545c527
> @@ -0,0 +1,210 @@
648c630,635
< +    return views_embed_view('fieldable_pane_entities', 'default', $item->name);
---
> +    $view = views_get_view('fieldable_pane_entities', 'default', $item->name);
> +    $view->execute();
> +    if (count($view->result)) {
> +      return views_embed_view('fieldable_pane_entities', 'default', $item->name);
> +    }
> +    return t('There are currently no entities of this type.');
helior’s picture

I certainly agree that there should be some sort of feedback when there is no entities to display. However, that is a separate problem that existed before this patch, I feel it's out of scope of this patch. There should be a different issue in the queue for that.

acrollet’s picture

Status: Needs review » Reviewed & tested by the community

@helior it seems strange to add an issue against code that has not yet been committed. In any case, I consider the patch in #19 to be RTBC, and will create a separate issue for this entity listing page when and if it lands.

merlinofchaos’s picture

Status: Reviewed & tested by the community » Needs review

Also, the view itself should have an empty text, so that's the better way.

acrollet’s picture

Also, the view itself should have an empty text, so that's the better way.

Of course, that makes sense - I created a separate issue with a patch #1719798: Informational text should be displayed on the 'list' tab when no fieldable panels pane entities have been created.

merlinofchaos’s picture

You know, I bet we could use a little bit of trickery in our own hook_entity_info_alter, if we set our weight so we run last, to automatically correct URL paths with a simple substr. That would alleviate a potentially common problem with tabs being funky, and that would make it much easier to commit this because it shouldn't break any of the existing bundles.

helior’s picture

FileSize
31.82 KB

Thanks for the tip! This patch adds hook_entity_info_alter() to fix obsolete paths that existed before #1607028: Entity base path is too deep , as well as hook_module_implements_alter() to ensure that we're getting called last for the entity info alter. The messages in the status report were also updated to reflect the changes.

kunago’s picture

@helior: Very nice work. I have one improvement to add. On a fresh system when I access the page "admin/config" I get this warning:

Warning: Invalid argument supplied for foreach() in fieldable_panels_panes_entity_info_alter() (line 88 of modules\fieldable_panels_panes\fieldable_panels_panes.module).

The function returns an empty array on the page for me.

I suggest wrapping the foreach with

if (is_array($info) && !empty($info)) {
 ...
}

When I did so, the warning went away.

acrollet’s picture

Status: Needs review » Needs work

Setting status based on #29.

acrollet’s picture

Status: Needs work » Needs review
FileSize
31 KB

I can verify the bug and fix in #29, otherwise things seem to work nicely. Patch attached incorporating the fix in #29.

arne_hortell’s picture

Title: User interface for managing bundles » error at row 376 and 392

This is mostly for me to remember when it happens again.
Hopefully someone put it into the real module later.

function ctools_export_ui_get_handler($plugin) {

//At row 376
//add this first line

if (array_key_exists("name",$plugin)) // This is my fix, new row 376
if (empty($cache[$plugin['name']])) { // this is previous row 376, new row 377
...
Then at end of function
replace returnstatement with this

return !( !array_key_exists("name",$plugin) ||
empty($cache[$plugin['name']])) ?
$cache[$plugin['name']] : FALSE;

// Voila no errors messages...

Robin Millette’s picture

Just resetting the issue title after #32

Robin Millette’s picture

Title: error at row 376 and 392 » User interface for managing bundles

Just resetting the issue title after #32 (and my mistake in #33)

helior’s picture

FileSize
33.85 KB

I discovered a bug in packaging FPP as an installation profile dependency. The 'install' $phase of hook_requirements() was not returning the correct data type and caused a fatal error. This may have already been apparent to some people if they tried including this patch with distros like Panopoly, or whatever.

Hydra’s picture

Status: Needs review » Needs work

Sadly the patch isn't applying against the latest commits. Checking out e4ab5031b47e82c1bd61b300218f8b622459ab88 did the trick. On this state, the patch works great.

helior’s picture

Status: Needs work » Needs review
FileSize
33.85 KB

No worries @Hydra, I got yo' back! :) Re-rolled at current HEAD which is at 1bb6b5e at time of writing.

Jason Dean’s picture

After applying #37 to 7.x-1.3, I had two fieldable_panels_panes_update_7106() functions in fieldable_panels_panes.install (so Drupal couldn't bootstrap). I renamed one of them to _7107 and ran update.php no problems.

Maybe I should have applied to latest dev instead? Either way, the UI is working well - thanks!

helior’s picture

FileSize
31.72 KB

D'oh! My mistake, I must have uploaded the wrong patch (or inadvertently renamed the previous one.)

Hydra’s picture

Status: Needs review » Reviewed & tested by the community

Well this looks good to me, thanks! I think we should give it a shot., very nice work.

gmclelland’s picture

#39 worked for me

Mschudders’s picture

So can we add this to the current dev version ?

Dave Reid’s picture

I'll try and review this shortly.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work

It looks like there is a mix of translation code that conflicts with what landed in #1536944: Allow panes to be translated, especially considering this makes a duplicate EntityTranslationFieldablePanelsPaneHandler class. Translation code should be removed from patch #39 before it can be committed.

helior’s picture

Status: Needs work » Needs review
FileSize
30.97 KB

This should be better.

gmclelland’s picture

Applied the patch in #45. Everything seems to work well. I was able to create and delete a fieldable bundle.

minorOffense’s picture

Same here.

Hydra’s picture

Status: Needs review » Reviewed & tested by the community

Still looking good, maybe Dave Reid will have another look?

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work

I think my main concern is all the stuff in fieldable_panels_panes.install is really necessary. Can't we just convert existing bundles to config, and then just display a message with something like 'The following modules define fieldable panel pane bundles using hook_entity_info_alter() and should have their bundles exported via Features and the hook removed.' I guess I should actually test it and see how it works and it may be nice. I just got the impression that to help support the old versions we're doing an awful lot that we'd have to maintain vs keeping it lean.

--

+++ b/fieldable_panels_panes.module
@@ -365,20 +389,19 @@ function fieldable_panels_panes_admin_menu_map() {
-  $map['admin/structure/fieldable-panels-panes/manage/%fieldable_panels_panes_type'] = array(
+  $map['admin/structure/fieldable-panels-panes/%ctools_export_ui'] = array(

Isn't clear to me why /manage/ is being removed and why it can't just stay the existing way. Also, shouldn't fieldable_panels_panes_type be converted to use ctools exportables? And if so, why can't it still be used in the menu path?

--

+++ b/fieldable_panels_panes.install
@@ -162,12 +267,50 @@ function fieldable_panels_panes_schema() {
+      'name' => array(
+        'description' => 'The machine-readable name of this type.',
+        'type' => 'varchar',

Should maybe be 'machine_name' to better describe what it is?

+++ b/fieldable_panels_panes.install
@@ -162,12 +267,50 @@ function fieldable_panels_panes_schema() {
+      'title' => array(
+        'description' => 'The human-readable name of this type.',

And should this be 'name' instead? In core for bundles we typically use 'name'. I think this would be better as both 'name' and 'title' are very similar. But 'machine_name' vs 'name' is clear.

--

+++ b/fieldable_panels_panes.install
@@ -162,12 +267,50 @@ function fieldable_panels_panes_schema() {
+      'description' => array(
+        'description' => 'A brief description of this type.',
+        'type' => 'text',
+        'size' => 'big',
+        'not null' => TRUE,
+      ),

Schema for description should probably be the same as {node_type}.description?

      'description' => array(
        'description' => 'A brief description of this type.', 
        'type' => 'text', 
        'not null' => TRUE, 
        'size' => 'medium', 
        'translatable' => TRUE,
      ), 

--

+++ b/fieldable_panels_panes.install
@@ -233,3 +376,35 @@ function fieldable_panels_panes_update_7106() {
+  $messages[] = t('Table %table_name has been added.', array('%table_name' => 'fieldable_panels_panes_type'));

Not really a necessary message. Let's exclude this.

--

+++ b/fieldable_panels_panes.install
@@ -5,6 +5,111 @@
+  $results = db_query('SELECT name FROM {fieldable_panels_panes_type}');
+  foreach ($results as $type) {
+    field_attach_delete_bundle('fieldable_panels_pane', $type->name);
+  }

Should this be loading all the panes from ctools exportables and not just the database?

--

+++ b/fieldable_panels_panes.install
@@ -162,12 +267,50 @@ function fieldable_panels_panes_schema() {
+  $schema['fieldable_panels_panes_type'] = array(

Should this table actually be named 'fieldable_panels_pane_type' since the pane types is the plural. Sounds odd to say 'Panes Types'.

--

+++ b/fieldable_panels_panes.install
@@ -233,3 +376,35 @@ function fieldable_panels_panes_update_7106() {
+  $schema = drupal_get_schema('fieldable_panels_panes_type', TRUE);

You cannot use drual_get_schema() in update functions. I know it sucks but you must copy the schema array entirely here. Otherwise this will break future changes to the fieldable_panels_panes_type table.

--

+++ b/fieldable_panels_panes.module
@@ -365,20 +389,19 @@ function fieldable_panels_panes_admin_menu_map() {
-  $map['admin/structure/fieldable-panels-panes/manage/%fieldable_panels_panes_type'] = array(
+  $map['admin/structure/fieldable-panels-panes/%ctools_export_ui'] = array(

Isn't clear to me why /manage/ is being removed and why it can't just stay.

--

Looks like fieldable_panels_panes_type_load() and fieldable_panels_panes_entities_title() are missing conversion to the ctools exportables code.

--

Can the user change machine names on the pane types? If so, it doesn't look like we're accounting for it and calling field_attach_rename_bundle().

gmclelland’s picture

Just a fyi... As of 1.4 the patch in #45 no longer applies.

minorOffense’s picture

I'm going to be working on fixing some of the issues which were brought up and re-roll the patch against the latest dev.

minorOffense’s picture

Alright, I'm about half way through redoing the patch. I'm at the point where we need to decide if we continue special handling for modules which used hook_entity_info_alter to add bundles or leave them be.

Personally, I would say anyone keen enough to write the bundle from scratch doesn't need our help with creating an export of their configuration. But that's just me.

There's also the option of exploring the Entity API module as I believe it supports exportables for entities by default (see http://drupal.org/project/entity) but that's a bit bigger of a change. But it would most likely give us what we're looking for.

pinkonomy’s picture

Does the #45 patch applies to the latest version of FPP?

pinkonomy’s picture

Issue summary: View changes

Updated issue summary.

realityloop’s picture

Issue summary: View changes
FileSize
19.3 KB

#45 rerolled against dev

realityloop’s picture

FileSize
27.87 KB

Table name changed as per davereids comments

realityloop’s picture

Status: Needs work » Needs review

The last submitted patch, 19: bundle-ui-1618308-19.patch, failed testing.

The last submitted patch, 17: fieldable_panels_panes-ui_manging_bundles-1618308-16.patch, failed testing.

The last submitted patch, 10: bundle-ui-1618308-11.patch, failed testing.

The last submitted patch, 9: bundle-ui-1618308-9.patch, failed testing.

The last submitted patch, 8: bundle-ui-1618308-8.patch, failed testing.

The last submitted patch, 1: bundle-ui-1618308-1.patch, failed testing.

The last submitted patch, 35: 1618308-bundle-ui-35.patch, failed testing.

The last submitted patch, 22: fieldable_panels_panes-ui_manging_bundles-1618308-22.patch, failed testing.

The last submitted patch, 28: bundle-ui-1618308-28.patch, failed testing.

The last submitted patch, 31: fieldable_panels_panes-ui_manging_bundles-1618308-31.patch, failed testing.

The last submitted patch, 45: 1618308-bundle-ui-45.patch, failed testing.

The last submitted patch, 39: 1618308-bundle-ui-39.patch, failed testing.

The last submitted patch, 37: 1618308-bundle-ui-37.patch, failed testing.

magicmyth’s picture

I've just rebased the patch on the latest git and made some cleanup.

Note that patch #55 was missing the ctools export_ui plugins which really through me out for quite a while trying to get this to work. I pulled it from patch #45 and updated the changed paths.

fieldable_panels_panes_admin_menu_map() and fieldable_panels_panes_admin_menu_map_alter() could do with extra review attention as I have not tested Admin Menu with this and HEAD had split the logic from fieldable_panels_panes_admin_menu_map() over to the _alter().

I've added a compatibility for function calls to: fieldable_panels_panes_type_load(). This was renamed in the previous patch but as it is a public facing function, it should not be removed in the 7.x-1.x branch ever as other modules may have been making calls to it to access the types.

I've simplified the upgrade code in fieldable_panels_panes_update_7108(). Previously it was duplicating the schema definition. Now it just loads it with drupal_get_schema_unprocessed();

I've also (potentially) fixed the bug where some user's ended up with no bundles after the install (@cangeceiro #4). If a user upgraded the module but cleared their cache before running update.php. The old default bundle created in code would have been removed from the entity cache as the code defining it no longer exists! So what I've done is check if $entity_info['bundles'] is empty and if it is, the default is added to that array and the upgrade runs as before. However I think it is worth considering if the old default should remain in code because if a site has bundles defined in code and upgrade, cache clear, then run update.php. The default original bundle will no longer exist. This would be easy for site admins to fix though simply by going to the new bundle UI and adding a new type with the machine ID 'fieldable_panels_pane'. Maybe add a note in README.txt and possibly add a status message after upgrade explaining this?

magicmyth’s picture

Forgot to mention in response to Dave Reid's comment 49:

You cannot use drual_get_schema() in update functions. I know it sucks but you must copy the schema array entirely here. Otherwise this will break future changes to the fieldable_panels_panes_type table.

I've used drupal_get_schema_unprocessed() instead of drupal_get_schema(). This gets the raw un-cached version but I'm not quite sure on how you think it would break future changes to the table schema. Is it because drupal_get_schema() can return a cached altered version or because future update hooks that make alterations to the existing table might fail becuase update_7108() had already installed the latest definition? If the later, should the hook not check the existing installed schema first and only make the alteration if it did not already exist? E.g:

// Imagineray update
function fieldable_panels_panes_update_7109() {
  $schema = drupal_get_schema('fieldable_panels_panes');
  if (!isset($schema['fields']['new_field'])) {
    $field = array(...);
    db_add_field('fieldable_panels_panes', 'new_field', $field);
  }
}

Thanks all.

magicmyth’s picture

Just discovered fieldable_panels_panes_requirements() will break sites all over the place. This is because entity_info_alter() implementations will be expecting parameter &$entity_info to contain actual entity info instead of the blank array the previous patches passed in. Many modules do something like the following:

$entity_info['node']['view modes'] += array(...);

Which will break with a unsupported operand type error. We would have to pass in a properly filled &$entity_info but we cannot use get_entity_info() for this as it will already contained the altered array(). And since bundles do not list what modules defined them, we cannot detect which modules are implementing fieldable_panels_pane_type bundles. The only way I can see to do this is to re-implement the logic in get_entity_info() but not the part that calls drupal_alter. We would then count() the fieldable_panels_pane bundles and store that in a variable. Then calling module_implements() we would check if the array had increased in size:

$hook = 'entity_info_alter';
               // Does similar to get_entity_info() minus caching and alter.
$entity_info = _fpp_get_entity_info();
$bundle_count = count($entity_info['fieldable_panels_panes']['bundles']);
$modules_implementing_bundles = array();

foreach (module_implements($hook) as $module) {
  $function = $module . '_' . $hook;
  $function($entity_info);
  if ($bundle_count != count($entity_info['fieldable_panels_panes']['bundles'])) {
    // The number of bundles increased so this module must have added one.
    $modules_implementing_bundles[] = $module;
    $bundle_count = count($entity_info['fieldable_panels_panes']['bundles']);
  }
}

foreach($modules_implementing_bundles as $module) {
  // Put out the message on redundant bundle code
}

However. entity_get_info() caches for a reason. Building that data is slooow!. And _fpp_get_entity_info() would run at least on every viewing of of the status report page. I really don't think it is worth bothering people about the old bundles defined in code at this cost in both performance and module maintenance.

So I've attached an updated patch that removes fieldable_panels_panes_requirements();

DamienMcKenna’s picture

Status: Needs review » Needs work

The last submitted patch, 73: fieldable_panels_panes-n73.patch, failed testing.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
31.08 KB

Of course if I build the patch right it might work better X-)

DamienMcKenna’s picture

I've added a follow-up issue to make the title field changeable: #1618308: User interface for managing bundles

magicmyth’s picture

@DamienMcKenna, would it be better if the schema definition was moved to a function like _fieldable_panels_panes_schema_v1() which would be used both in fieldable_panels_panes_schema() and fieldable_panels_panes_update_7108()? Then when the schema is altered a new function (e.g. _fieldable_panels_panes_schema_v2()) is created and fieldable_panels_panes_schema() is updated to call this function instead.

function fieldable_panels_panes_schema() {
  // Always point to our current schema version
  return _fieldable_panels_panes_schema_v1();
}

I hate any code duplication and this makes it easier to see how the schema evolves over time. I've also seen other modules doing this so its not an original idea ;)

azinck’s picture

Would it make sense to add DB fields to contain default access configuration for new entities of the given bundle? I'm not too clear on the best architecture here but I'd love to be able to specify default access configuration.

Otherwise patch in #75 looks good to me so far.

DamienMcKenna’s picture

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed. Thanks for everyone's help!

  • Commit a73ba0d on 7.x-1.x by DamienMcKenna:
    Issue #1618308 by helior, acrollet, realityloop, magicmyth,...
DamienMcKenna’s picture

Status: Fixed » Needs review
FileSize
1.08 KB

Caught a bug in the menu paths, if they use 'admin/structure/fieldable-panels-panes/%ctools_export_ui' the Field API integration breaks.

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed.

  • Commit 4ddff9e on 7.x-1.x by DamienMcKenna:
    Issue #1618308 by DamienMcKenna: Follow-up to fix menu paths.
    

Status: Fixed » Closed (fixed)

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

banoodle’s picture

Hi, I'm trying to upgrade to OpenAtrium 7.x-2.42, but the upgrade db updates for this module aren't applying...

The following updates returned messages

fieldable_panels_panes module

Update #7108
Failed: DatabaseSchemaObjectExistsException: Table fieldable_panels_pane_type already exists. in DatabaseSchema->createTable() (line 657 of /var/www/vhosts/intranet.synberc.org/dev/includes/database/schema.inc).

Since this problem seems related to this issue, I attempted applying the patch in #82 but application failed. I imagine this is because I'm attempting to apply the patch to release 7.x-1.6 while the patch is intended for 7.x-1.x-dev.

I'm not sure what to do in this case. Do I upgrade this module to the dev release and then apply the patch? That is what I would normally do, but this module is part of the OpenAtrium distro, so I'm not sure. I'm also not sure if I should report this to the OA issue queue?

Argus’s picture

Hi banoodle, this issue is closed so not many people will notice it. You better open a new issue and relate it to this one. Also your issue is Open Atrium related, so it might be better to post it in the OA issue queue.

banoodle’s picture

Thanks @Argus - I just added it to the OA issue queue: https://www.drupal.org/node/2514854#comment-10072640 - it doesn't look like anyone else has reported this issue yet.