Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
We need a user interface that allows us to create, manage, and delete bundles, such as is expected with any other fieldable entity.
This feature is dependent on #1618264: Improved DX for Export UI which will make it possible for Ctools Export UI to manage Drupal entities and fields.
Comment | File | Size | Author |
---|---|---|---|
#82 | fieldable_panels_panes-n1618308-82.patch | 1.08 KB | DamienMcKenna |
#75 | fieldable_panels_panes-n1618308-75.patch | 31.08 KB | DamienMcKenna |
Comments
Comment #1
helior CreditAttribution: helior commentedThis patch depends on #1618264: Improved DX for Export UI
Comment #2
cangeceiro CreditAttribution: cangeceiro commentedI 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.
Comment #3
helior CreditAttribution: helior commentedThis 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.
Comment #4
cangeceiro CreditAttribution: cangeceiro commentedafter 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.
Comment #5
helior CreditAttribution: helior commentedDid 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.
Comment #6
cangeceiro CreditAttribution: cangeceiro commentedyes, 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.
Comment #7
helior CreditAttribution: helior commentedI'll make sure I'm available, thanks!
Comment #8
helior CreditAttribution: helior commentedThis patch depends on #1607028: Entity base path is too deep and #1618264: Improved DX for Export UI in Ctools.
Comment #9
helior CreditAttribution: helior commentedRe-rolling patch to accommodate changes in HEAD. This still depends on #1618264: Improved DX for Export UI
Comment #9.0
helior CreditAttribution: helior commentedUpdated issue summary.
Comment #10
helior CreditAttribution: helior commentedAll 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..
Comment #11
helior CreditAttribution: helior commentedChanging the priority to major –- this feature is kind of a big deal.
Comment #12
merlinofchaos CreditAttribution: merlinofchaos commentedI 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.
Comment #13
jwilson3A nitpick, but there's a missing "*" from the update script function comment that causes drush to not detect the message while running updates:
Additionally, the comment for the update script could be shortened into a single-line comment like "Improve bundle management to allow CTools exportability."
Comment #14
jwilson3Also, I can concur that the same thing happens to me as with comment #4:
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.
Comment #15
jwilson3ctools_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.
Comment #16
jwilson3It 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.
Comment #17
acrollet CreditAttribution: acrollet commentedI 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.
Comment #18
jwilson3Patch 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).
Comment #19
helior CreditAttribution: helior commented@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.
Comment #20
helior CreditAttribution: helior commentedComment #21
acrollet CreditAttribution: acrollet commented@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.
Comment #22
acrollet CreditAttribution: acrollet commentedpatch 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:
Comment #23
helior CreditAttribution: helior commentedI 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.
Comment #24
acrollet CreditAttribution: acrollet commented@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.
Comment #25
merlinofchaos CreditAttribution: merlinofchaos commentedAlso, the view itself should have an empty text, so that's the better way.
Comment #26
acrollet CreditAttribution: acrollet commentedOf 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.
Comment #27
merlinofchaos CreditAttribution: merlinofchaos commentedYou 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.
Comment #28
helior CreditAttribution: helior commentedThanks 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.
Comment #29
kunago CreditAttribution: kunago commented@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:
The function returns an empty array on the page for me.
I suggest wrapping the foreach with
When I did so, the warning went away.
Comment #30
acrollet CreditAttribution: acrollet commentedSetting status based on #29.
Comment #31
acrollet CreditAttribution: acrollet commentedI can verify the bug and fix in #29, otherwise things seem to work nicely. Patch attached incorporating the fix in #29.
Comment #32
arne_hortell CreditAttribution: arne_hortell commentedThis 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...
Comment #33
Robin Millette CreditAttribution: Robin Millette commentedJust resetting the issue title after #32
Comment #34
Robin Millette CreditAttribution: Robin Millette commentedJust resetting the issue title after #32 (and my mistake in #33)
Comment #35
helior CreditAttribution: helior commentedI 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.
Comment #36
Hydra CreditAttribution: Hydra commentedSadly the patch isn't applying against the latest commits. Checking out e4ab5031b47e82c1bd61b300218f8b622459ab88 did the trick. On this state, the patch works great.
Comment #37
helior CreditAttribution: helior commentedNo worries @Hydra, I got yo' back! :) Re-rolled at current HEAD which is at 1bb6b5e at time of writing.
Comment #38
Jason Dean CreditAttribution: Jason Dean commentedAfter 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!
Comment #39
helior CreditAttribution: helior commentedD'oh! My mistake, I must have uploaded the wrong patch (or inadvertently renamed the previous one.)
Comment #40
Hydra CreditAttribution: Hydra commentedWell this looks good to me, thanks! I think we should give it a shot., very nice work.
Comment #41
gmclelland CreditAttribution: gmclelland commented#39 worked for me
Comment #42
Mschudders CreditAttribution: Mschudders commentedSo can we add this to the current dev version ?
Comment #43
Dave ReidI'll try and review this shortly.
Comment #44
Dave ReidIt 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.
Comment #45
helior CreditAttribution: helior commentedThis should be better.
Comment #46
gmclelland CreditAttribution: gmclelland commentedApplied the patch in #45. Everything seems to work well. I was able to create and delete a fieldable bundle.
Comment #47
minorOffense CreditAttribution: minorOffense commentedSame here.
Comment #48
Hydra CreditAttribution: Hydra commentedStill looking good, maybe Dave Reid will have another look?
Comment #49
Dave ReidI 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.
--
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?
--
Should maybe be 'machine_name' to better describe what it is?
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.
--
Schema for description should probably be the same as {node_type}.description?
--
Not really a necessary message. Let's exclude this.
--
Should this be loading all the panes from ctools exportables and not just the database?
--
Should this table actually be named 'fieldable_panels_pane_type' since the pane types is the plural. Sounds odd to say 'Panes Types'.
--
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.
--
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().
Comment #50
gmclelland CreditAttribution: gmclelland commentedJust a fyi... As of 1.4 the patch in #45 no longer applies.
Comment #51
minorOffense CreditAttribution: minorOffense commentedI'm going to be working on fixing some of the issues which were brought up and re-roll the patch against the latest dev.
Comment #52
minorOffense CreditAttribution: minorOffense commentedAlright, 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.
Comment #53
pinkonomy CreditAttribution: pinkonomy commentedDoes the #45 patch applies to the latest version of FPP?
Comment #53.0
pinkonomy CreditAttribution: pinkonomy commentedUpdated issue summary.
Comment #54
realityloop#45 rerolled against dev
Comment #55
realityloopTable name changed as per davereids comments
Comment #56
realityloopComment #70
magicmyth CreditAttribution: magicmyth commentedI'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?
Comment #71
magicmyth CreditAttribution: magicmyth commentedForgot to mention in response to Dave Reid's comment 49:
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:
Thanks all.
Comment #72
magicmyth CreditAttribution: magicmyth commentedJust 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:
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:
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();
Comment #73
DamienMcKennaI've updated fieldable_panels_panes_update_7108() so it has a copy of the schema's definition, per Dave Reid's recommendation in #49.
I've tested the patch and so far it seems to be working nicely.
Comment #75
DamienMcKennaOf course if I build the patch right it might work better X-)
Comment #76
DamienMcKennaI've added a follow-up issue to make the title field changeable: #1618308: User interface for managing bundles
Comment #77
magicmyth CreditAttribution: magicmyth commented@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.
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 ;)
Comment #78
azinck CreditAttribution: azinck commentedWould 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.
Comment #79
DamienMcKennaComment #80
DamienMcKennaCommitted. Thanks for everyone's help!
Comment #82
DamienMcKennaCaught a bug in the menu paths, if they use 'admin/structure/fieldable-panels-panes/%ctools_export_ui' the Field API integration breaks.
Comment #83
DamienMcKennaCommitted.
Comment #86
banoodle CreditAttribution: banoodle commentedHi, I'm trying to upgrade to OpenAtrium 7.x-2.42, but the upgrade db updates for this module aren't applying...
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?
Comment #87
Argus CreditAttribution: Argus as a volunteer commentedHi 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.
Comment #88
banoodle CreditAttribution: banoodle commentedThanks @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.