One of the annoying bits of block configuration is that the configuration has to be stored and retrieved in the database. No matter where you put it (a dedicated db table, variable_get/set()), it still requires additional db-to-website communication.

  1. variable_get()/variable_set(). This is the current method that menu_block uses to store each block’s configuration. The benefit of this method is that the variable table is read once from the database for each page request and menu_block doesn't need to pull any additional data from the database (i.e. zero extra db queries). The negative side to this is that Drupal’s variable table is already pretty bloated and this just adds to the problem.
  2. dedicated menu_block db table. This reduces the strain on Drupal’s variable table. But that means an additional db query. IMO, extra queries add up. One or two here, one or two there… I can probably do some tricks to load all the active menu blocks' configs at the same time.

So exportable configuration sound like a real win. The config would be stored in code and menu_block would just pull the config from a custom module's codebase. No database queries required.

I want to look at CTool's exportables, but haven't decided on any particular API for this yet.

Files: 
CommentFileSizeAuthor
#119 ctools_exportable-693302-119.patch41.69 KBDeciphered
#119 interdiff.txt3.53 KBDeciphered
#115 ctools_exportable-693302-115.patch39.7 KBDeciphered
#115 interdiff.txt858 bytesDeciphered
#113 interdiff.txt2.6 KBjwilson3
#113 menu_block-ctools-693302-113.patch39.66 KBjwilson3
#112 interdiff.txt928 bytesjwilson3
#112 menu_block-ctools-693302-112.patch38.58 KBjwilson3
#111 interdiff-96-110.txt20.19 KBjwilson3
#110 menu_block-ctools-693302-110.patch38.73 KBjwilson3
#110 interdiff.txt11.77 KBjwilson3
#96 menu_block-ctools-693302-96.patch35.59 KBmpgeek
#96 interdiff.txt510 bytesmpgeek
#94 menu_block-ctools-693302-94.patch35.59 KBmpgeek
#93 menu_block-ctools-693302-93.patch27.17 KBmpgeek
#83 menu_block-ctools-693302-83.patch35.62 KBtangent
#82 menu_block-ctools-693302-81-interdiff.txt854 bytesrudiedirkx
#81 screen-drupal-menu_block-edit.png26.12 KBrudiedirkx
#80 menu_block-ctools-693302-80.patch35.56 KBtangent
#79 menu_block-ctools-693302-79.patch0 bytestangent
#72 menu_block-features-693302-72.patch3.09 KBrudiedirkx
#69 delete_menu_block_error.png193.21 KBdkingofpa
#63 menu_blocks-exportable_api-693302-63.patch36.59 KBgagarine
#50 ctools_exportable-693302-50.patch36.56 KBDeciphered
#49 ctools_exportable-693302-49.patch36.58 KBDeciphered
#49 interdiff.txt2.83 KBDeciphered
#42 menu_block-ctools_export-693302-42.patch37.86 KBericaordinary
#42 interdiff.txt2.16 KBericaordinary
#41 interdiff.txt2.16 KBericaordinary
#41 menu_block-ctools_export-693302-41.patch41.34 KBericaordinary
#40 menu_block-ctools_export-693302-40.patch37.86 KBericaordinary
#30 693302-interdiff-23-28-diff.txt9.18 KBrudiedirkx
#30 693302-interdiff-23-28-show.txt36.17 KBrudiedirkx
#28 ctools_export-693302-28.patch39.48 KBrudiedirkx
#23 ctools_export-693302-23.patch36 KBDeciphered
#20 inglorioushack.install.txt1.07 KBTravisCarden
#2 exports-693302-2.patch5.01 KBJohnAlbin
#1 exports-693302-1.patch4.91 KBJohnAlbin

Comments

Status:Active» Needs review
StatusFileSize
new4.91 KB

Ctools/exportables looks like the way to go, but there's no D7 version of it yet. Which is what I'm currently working on.

Here's a dirt simple export-to-code interface:

<?php
/**
* Return a list of configurations for menu blocks.
*
* Modules that want to have menu block configurations exported to code should
* provide them using this hook.
*
* @see menu_tree_build() for a description of the config array.
*/
function hook_menu_block_blocks() {
  return array(
   
// The array key is the block id used by menu block.
   
'custom-nav' => array(
     
// Use the array keys/values described in menu_tree_build().
     
'menu_name'   => 'primary-links',
     
'parent_mlid' => 0,
     
'title_link'  => FALSE,
     
'admin_title' => 'Drop-down navigation',
     
'level'       => 1,
     
'follow'      => 0,
     
'depth'       => 2,
     
'expanded'    => TRUE,
     
'sort'        => FALSE,
    ),
   
// To prevent clobbering of the block id, it is recommended to prefix it
    // with the module name.
   
'custom-active' => array(
     
'menu_name'   => '_active',
     
'title_link'  => TRUE,
     
'admin_title' => 'Secondary navigation',
     
'level'       => 3,
     
'depth'       => 3,
     
// Any config options not specified will get the default value.
 
);
}
?>

On the fence on this one.

Version:6.x-2.x-dev» 7.x-2.x-dev
Status:Needs review» Fixed
StatusFileSize
new5.01 KB

Decided not to commit to 6.x-2.x branch. Ctools exportables/features integration should be the solution for that.

I've committed this to 7.x-2.x since there's no ctools for d7 yet. But I'm not committing it to 7.x-3.x; I'll wait for ctools for that branch.

Title:Add API for exportable menu_blocksAdd API for exportable menu_blocks / Features integration
Version:7.x-2.x-dev» 7.x-3.x-dev

Re-opening for possible 7.x-3.x or 6.x-2.x patches. Submissions welcome!!! :-)

Status:Fixed» Active

Um… now patches are welcome. :-D

Just to be clear, I would also accept patches for the 6.x-2.x branch as well. :-)

Would be loving some D6 support for this idea!!

Ctools is now updated to D7, see #589636: Port to Drupal 7.

Because the config is currently in variables, it's already exportable with features (through strongarm).

To convert to ctools exportables, the first step will be to replace the variable use with a custom table. Then add the data needed by ctools to hook_schema().

subcribing

+1

Is replacing variables with a custom table a step forward or a step back...?

A custom table is a step forward. This kind of configuration does not belong into variables in the first place.

D7 CTools is pretty stable now.

Subbing.

Subscribing.

@JohnAlbin: Any objections or concerns to using Entity API as storage for menu blocks? You'd get (nearly) free exportables out of it as well. I might be able to find a few minutes to work on that if you don't object to the approach or extra dependency

Looks like Entity is going into core anyway, no?Something something - http://drupal.org/node/1018602#comment-5017458

If there's a sane, core-centric way to do this that involves a deployment-friendly outcome, then +1 is a no-brainer I think? :)

+1 features is quite used now

+1 for entity api in D7/D8

Any progress on this? I'm having the biggest deploy problem I've had in D7 with getting menu_blocks to behave. Documentation would be very helpful. A workable API to get links to work would help.

Features generally works well for common deploy tasks. As near as I can tell, the menu link settings that aren't getting exported aren't in the variable table either. I'm about to tear out my use of menu_blocks and custom code this, which is extreme, but looking increasingly necessary.

StatusFileSize
new1.07 KB

@Torenware: I've "solved" this before by simulating a form submission in a .install file. See the attached example for the fictitious inglorioushack module. ;-)

@Torenware you could make a custom module that creates the block using hook_update. Use update.php to trigger the update that creates the block. It's messy though.

The other way is to simply always create the menu block on production site, download the database and use that block when packing features. The block won't get exported itself, but since it will be identical to the one in the production environment you won't have any problems.

Or you could manually create the menu block on the live server after your deploy. As long as the ID and name is the same, it will work.

@TravisCarden,

Thanks. Yeah, ugly. But that would work, at least.

Version:7.x-3.x-dev» 7.x-2.x-dev
Status:Active» Needs work
StatusFileSize
new36 KB

First attempt at a patch, moves variables into a database table and adds CTools Export and Export UI integration.

Patched 2.x-dev version with #23. Applies without warnings.

A few functional issues:

  1. Update path doesn't work completely: labels/titles aren't copied, so existing blocks will be titleless on the blocks admin page.
  2. The "Administrative title" field should be required IMO. Currently it isn't, so even new blocks can be titleless on the blocks admin page.
  3. This patch completely deprecates module menu_block_export (which is good), so remove that from the project? (Paths are actually broken when enabled.)
  4. The configure link on the modules page (to admin/config/user-interface/menu-block) (via the .info) is now invalid, so remove or better: change to admin/structure/menu_block.

And technical:

  1. The db table should be named `menu_block` (singular), because the module's called that. (Like `node`, not like `users`.)
  2. All the files[] entries in the .info are unnecessary and BAD. Only files with classes should be listed there (for the registry). You can (should) remove all.
  3. I don't think a module has to delete its blocks from the `block` table in hook_uninstall. Blocks are rehashed regularly, so Drupal/blocks.module will fix that.
  4. Menu Block already had a hook to collect menu blocks: hook_menu_block_blocks. Is that still used? For backward compatibility quite important... Maybe in a legacy manner, with a lot of angry comments etc? =)

I haven't tried the actual Menu block functionality (I assume you haven't changed that), but the export/import works excellently! CTools export is the best! Including revert and a nice admin page etc. Excellent.

Why didn't you patch against 7.x-3.x? What's in 3.x anyway?

I didn't apply it to the 3.x branch because it doesn't appear to actually be a valid branch, the only two commits it's had are to the changelog, whereas the 2.x branch still seems to be active.

I marked the patch as needs work as I'm well aware it's not perfect, but it's a start and anyone who wants to continue it are more than welcome to do so.

You're not gonna? :) It's almost perfect!

I have a ton on my plate and I don't currently have any use for this module. When I have more time I might come back to it, but ideally it would be better for someone who is actively using the module and needs the exportable functionality to take the existing patch I've supplied and continue the work. As you said, It's almost perfect, which means it shouldn't be too hard for someone else to finish.

Otherwise, my company is always open for sponsored development (although I can't guarantee sponsorship status on a contrib module I don't maintain).

Status:Needs work» Needs review
StatusFileSize
new39.48 KB

I've fixed the crossed-through items in #24. Also fixed a few bugs. New patch attached.

I'm a little worried about maintaining backward compatibility. It'd be best if this update got a new major release, so no backward compatibility is required, but that's kinda dickish...

I'll change the issue status, so someone will notice and review.

rduiedirkx,

Can you please include an interdiff.txt so I/anyone can see the differences between the two patches (easier to review that way)?

Alright. I've tried the http://drupal.org/node/1488712 way (with git show, which is weird) and my own way (with git diff). Both attached. I don't understand the git show one.

I actually messed up something big. I changed menu_block_get_config() to the new method, which destroys the update path... I'll fix that this afternoon and do a new total diff and 23-# interdiff.

Aah damn it! I quit! I fixed my bugs and improved on #28 and then I created an empty patch somehow (?) and reverted to test the update path. So now I lost all my code.

I quit!

To anyone, you can use #28, but have to first revert menu_block_get_config() (legacy, for update path) and create a menu_block_load() (new, with ctools). I broke that in #28.

Status:Needs review» Needs work

Re #31: If you made a commit before rolling the patch, @rudiedirkx, you can still get at it with git reflog. If not, I'm sorry you lost your work!

Nope, didn't commit. Made a diff (or so I thought) from my dirty working dir and reset that after creating (or not) the diff. Stupid GIT =) It should ask me more: are you sure, are you absolutely, absolutely very sure?

I'll get at it again in a few days. At least to fix the update path bug I introduced in #28.

I have applied the patch in #28.
When I go to update my site using update.php, I get the following error message:

An unrecoverable error has occurred. You can find the error message below. It is advised to copy it to the clipboard for reference.
Please continue to the error page
An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: http://localhost/<sitehost>/lololabs/update.php?op=selection&token=Spl5Y4dnQwk4Vr8aKgUQJ6Lut4kQKYZjSSt9FwQ66hw&id=2&op=do StatusText: OK ResponseText: Fatal error: Call to undefined function ctools_export_crud_load() in /Users/<sitehost>/Sites/lololabs/sites/all/modules/menu_block/menu_block.module on line 179

The update process was aborted prematurely while running update #7203 in menu_block.module. All errors have been logged. You may need to check the watchdog database table manually.

How can I make the patch work? (Or are there any other methods to export a Menu Block, and the region/position of the block).

(As I understand the patch should work with new blocks, just not ones that need backwards compatibility. is that correct?)

Yes, #35, that's correct, but not until it's fixed. The patch fixes AND introduces bugs, so it's not applicable as is.

I'll give it another go tomorrow.

Be sure to back up your database before running updates after applying this patch. It removes the 'old' menu block storage.

Just a heads up, for anyone applying the patch in #23 you will need to specify -p1 for it to create a plugins/export_ui directory. The patch succeeds without the -p1, but the export_ui directory doesn't get created which breaks the add menu block functionality.

ericaordinary, if possible, please could you elaborate on what you mean by -p1 (or point to a link that explains it)?

I guess it must be a patching terminology. However, I have had a good look on Google and could not find anything that explains it.

Thanks for any help you can offer!

big_smile,

She means that the patch should be applied with the -p1 flag, which is usual practice for all Drupal patches (http://drupal.org/patch/apply).

Cheers,
Deciphered.

StatusFileSize
new37.86 KB

I've updated Deciphered's patch from #23 as it was still using variable_get to return the block configuration in hook_block_view. It's not a fantastic effort and I haven't tested it beyond our own setup, but it's a quick fix to get the blocks displaying with the correct configuration.

StatusFileSize
new41.34 KB
new2.16 KB

That last patch was a bit mangled, so please ignore it. Here's a new one, plus an interdiff with the previous patch in #23. This patch updates hook_block_view so that it uses the new ctools stuff to provide the $config, instead of variable_get.

StatusFileSize
new2.16 KB
new37.86 KB

Bugger, that patch is failing on the .info file I think because of http://drupal.org/node/1528422.

I've made another one from the current stable module which is applying correctly locally. Fingers crossed it will also work in a makefile. The interdiff is between this patch and #23.

For ericaordinary's patch (in #42) do I :
a) Apply it to the stable release
b) Apply it to the dev release
c) Apply #23 to the dev release and then apply ericaordinary's patch.

I've tried all three, but I get errors about missing chunks.

(I appreciate that this is a n00bish question, so thanks for your patience).

@big_smile

My patch in #42 is an update of the patch in #23, so you only need to apply the patch in #42. It works for me when applied to the current stable release, I haven't tested applying it to the latest dev. I have also tested applying it to the current stable with a drush makefile and it's working.

It might help if you could post the command you are using to apply the patch, as option a) should work if you use the -p1 flag. It might also be an idea to re-download the current stable version menu_block just to make sure you are using a clean, current copy of the module. And maybe an obvious question, but are you definitely trying to apply it to the D7 version of the module?

Thanks for all your help ericaordinary! I got it working without any error messages by downloading a fresh copy.

However, on the source site, I get the following WSOD error message:
Fatal error: Call to undefined function ctools_export_crud_load() in /[site]/Sites/labs/sites/all/modules/menu_block/menu_block.module on line 146

If I run the Drush command, drush cc all, then the message will disappear, but will then come back after a while.

When I export the feature to the target website, the WSOD message also appears, except running drush cc all will not remove it.
The only way to get rid of the message and access the site is to take the menu_block module out of the site/all/modules directory. But then I cannot test if the feature has exported correctly.

I would appreciate any pointers or suggestions.

edit: Ignore this post

@big_smile: Do you have ctools module installed and enabled on both the original and target sites? It's not a dependency of menu_block but probably should be with the new ctools exportable method introduced by the patch.

If you do have it installed it's possibly a bug in the new code and you might need to do some investigating. Particularly, try and find out what you are doing when the error appears (eg saving a node/loading a particular page). Also check what version of ctools module you have installed, and maybe try upgrading to the most current stable if you are a few versions behind.

Yes, that was the problem. Ctools was missing. Thanks for your help! It works fine now.

I have tested your patch on two different test sites and have found no problems.

Thanks again!

StatusFileSize
new2.83 KB
new36.58 KB

Updated patch, minor cleanup on the CTools Export UI side of things and cleanup of the menu_block_config_load() function.

Still needs work.

StatusFileSize
new36.56 KB

Ooops... left a DPM in there....

I have tested #50 patch without success.

The exportation in Features has been done but the importation didn't work properly. The menu blocks were not created.

Does somebody knows what could be happend?

How can I help on this issue?

I tested with dev version and It works. Thanks!

Is possible to make a patch for stable version (2.3)?

@mordonez,

It's possible to make a patch for 2.3, and I quite often do that for my own personal use on a site as it's easier to know exactly what I'm getting, however for the purposes of getting this committed, the main patch has to be against dev.

 

@omasjuan,

When you say import, are you referring to the regular 'turn on the feature, menu blocks get automatically imported' or 'copy exportable and paste into import interface' approach? I do have this running on a site in development and can confirm that it appears to be working (the features approach), but I don't doubt there could still be issues, so any further information you can provide would be great (including steps and the data/feature itself).

@Deciphered

Thanks for your answer.

After installing the dev version of the Menu_block module and applying the patch #50 and also enabling menu_block export the problem has been solved.

@Deciphered still need work or his ready for testing?

This is pretty awesome. I tested 50 and it works.

I did notice that the exported item appears as "1" instead of the readable name of the object like it does with other components. Is there a way to export it as the name of the menu or something?

@gagarine,

Can't remember off the top of my head, but I suspect I wouldn't have left it in 'needs work' unless I truly felt that was the case. I suspect it just needs some additional cleanup and culling of redundant functionality.

@joelcollinsdc,

The exporting as "1" is related to the upgrade path, to take the existing Menu blocks and move them to the Exportables approach. When creating new types you should be able to set the machine name to whatever you like.

#50 works for me!

@joelcollinsdc the best way to do this is probably to clone your old menu_block (using the new UI provided by the patch), and you will be given the option to give the clone a new machine name, keeping the same settings. Then you can export this menu_block in your feature.

Status:Needs work» Needs review

@Deciphered ... so we need review to know what we have to work on ;). I read the code and didn't see any big ugly things. For me #50 can be committed in dev.

Status:Needs review» Needs work

Sometimes I get this error (after clear cache). I also use http://drupal.org/project/features_extra..

Fatal error: Call to undefined function ctools_export_crud_load() in /drupal/sites/all/modules/contrib/menu_block/menu_block.module on line 146

Need to add ctools_include('export'); in menu_block_config_load

<?php
function menu_block_config_load($delta) {
 
ctools_include('export');
?>

@gagarine Can you upload a patch with this code?

Status:Needs work» Needs review
StatusFileSize
new36.59 KB

Yes this is the #51 #50 with my modification... no time for inter diff and so (sorry).

Status:Needs review» Needs work

The patch from #63 applied with no problems. But when I went to edit a menu block block, I got the following error:

Fatal error: Call to undefined function menu_block_export_ui_form() in /home/clients/websites/w_rtech/public_html/rtech/sites/all/modules/contrib/menu_block/menu_block.admin.inc on line 141

Hi guys,

Thank you so much for all the work on this.

I allow myself to quickly step in to let you know I posted a quick document in the tracker on #1894280: Exporting Menu Blocks with Features Extra and Strongarm, and since I thought it would also be related with this ticket, I would greatly appreciate if I could have your feedback, comments or questions on this.

Thanks very much to all in advance.
Cheers!

Hi DYdave,

It seems that these are two conflicting approaches, and given that this approach is meant to bring a native Features exportability to the module I would think it would be better to unify the effort on getting this patch committed.

Status:Needs work» Needs review

I've applied and tested this patch, and cannot reproduce the error reported in #64. It's working well for me, so setting back to 'needs review'. Deciphered, if you take a look, it would be great to get this moved forward. I may have time to work on any needed improvements to the patch.

lee20: I should add that if you can post instructions to reproduce the issue you saw on a clean installation, that would be very helpful in getting it fixed if it's not just an artifact of your installation.

Status:Needs review» Needs work
StatusFileSize
new193.21 KB

Using latest 7.x-2.x (from 08/31/2012) and patch from #63
http://drupalcode.org/project/menu_block.git/snapshot/fa9065e4b8182c9daa...

  1. Install drupal 7.19 with features
  2. Log in as user 1 (admin)
  3. Create some links on the Main Menu
  4. Create Menu Block with machine name of "menu_main" and set depth to 3
  5. Add created Menu Block to region on block admin page
  6. View frontpage....no menu showing up
  7. Delete the Menu Block from the Menu Block admin page
  8. Menu Block is deleted, but get an error: Trying to get property of non-object in menu_block_config_load() (see attached)
  9. Have to flush cache to get rid of error on screen.

I wasn't even able to test the features export of Menu Blocks. Version 7.x-2.3 worked fine (menu showed up).

No "delete callback" is defined in the export schema. As a result, only the default ctools_export_crud_delete in ctool's export.inc is called. That will only delete the actual exportable object. But it leaves behind all the other configuration data for the menu_block. For example, all the other config data that was previously cleaned up by menu_block_delete_submit:

<?php
function menu_block_delete_submit($form, &$form_state) {
 
// Remove the menu block configuration variables.
 
$delta = $form_state['values']['delta'];
 
$block_ids = variable_get('menu_block_ids', array());
  unset(
$block_ids[array_search($delta, $block_ids)]);
 
sort($block_ids);
 
variable_set('menu_block_ids', $block_ids);
 
variable_del("menu_block_{$delta}_title_link");
 
variable_del("menu_block_{$delta}_admin_title");
 
variable_del("menu_block_{$delta}_parent");
 
variable_del("menu_block_{$delta}_level");
 
variable_del("menu_block_{$delta}_follow");
 
variable_del("menu_block_{$delta}_depth");
 
variable_del("menu_block_{$delta}_expanded");
 
variable_del("menu_block_{$delta}_sort");
 
db_delete('block')
    ->
condition('module', 'menu_block')
    ->
condition('delta', $delta)
    ->
execute();
 
db_delete('block_role')
    ->
condition('module', 'menu_block')
    ->
condition('delta', $delta)
    ->
execute();
 
drupal_set_message(t('The block "%name" has been removed.', array('%name' => $form_state['values']['block_title'])));
 
cache_clear_all();
 
$form_state['redirect'] = 'admin/structure/block';
  return;
}
?>

I tried to add a delete callback to the schema after reading ctools/help/export.html, but was unsuccessful. It just kept calling the default crud function. I'm still trying to get my mind wrapped around this. But if someone with more knowledge of features could get that added to the patch in #63, it would be much appreciated.

@dkingofpa

I don't believe that any of those deletions are necessary any more as the changes have moved the Menu Blocks out of variables into their own database, and it's not essential to adjust the 'block' or 'block_role' database as it may not even be present as there is no valid reason why the Block module even needs to be enabled at all.

However I will try to look into the previous errors you logged during DrupalCon Sydney and see if I can get a new patch rolled and tested.

StatusFileSize
new3.09 KB

I've created a VERY SIMPLE PATCH, fully backward compatible, with no update path needed. The 'correct' way is taking too long. Do with it what you want, I'm using it.

A few notes.

  1. The feature items have a prefix, because features doesn't like numeric deltas (which is what menu_block makes). The prefix is added and removed automatically, so it exists only in code.
  2. The revert/rebuild functions use the same storage as menu_block already did (a bunch o' variables), which makes it ugly, wrong and simple. A revert doesn't 'delete from database so code will be used' (like Views and Context et al), but saves the config from the feature into the database. I've used the exact same approach for block_class.
  3. The diff doesn't work for me. It shows something completely wrong. I've no idea why. Reverting the component works as expected: the config in the feature is restored, not the weird info in the diff.
  4. It won't play well with a mlid that's not 0. That's how menu_block works and I haven't changed that. Featurizing a menu block with a mlid probably won't work (as expected).

I'll be looking forward to the correct way. I've given up on that as you can see in #31, but I hope someone will finish it =)

Hi rudiedirkx,

I understand exactly where you are coming from, however, posting your patch in this issue can only complicate matters, especially for beginners as your patch is now the most recent patch and therefore is the patch people will assume should be being used.

Instead, you should have opened a new issue, posted it there, and then marked that issue as a duplicate of this issue.

As far as this issue goes, I have used the patch I wrote on many sites with no major issues, I simply add it to my makefile so it's applied automatically during a site build. I would suggest that others should simply test the patch and determine if there are any issues if they want the issue to ever get resolved.

I did say I would look into the issues that have already been looked at, I have not yet done so, but that doesn't stop anyone else from stepping up and getting involved.

I haven't looked at the ctools method since #36-ish. Just wanted to share this patch for anyone who just wants Features support and nothing more.

Do you want me to edit my comment? I can change the text, but I can't delete the patch... Sorry.

Which patch should I try? #63?

Rule of thumb is to always use the latest patch as it should be the most up to date patch, hence why your patch will cause confusion.

#63 doesn't apply (2.x-dev). Fails in 3 of 5 files.

So which patch do I try?

[edit] Doesn't work for 2.3 either.

[edit] Same for patch in #50.

[edit] #42 works against 2.3. Let's try that.

#42 works and looks great. Just the following:

  • The search/filter form on the menu blocks page is completely unnecessary and moves the actual page content down. Waste of real estate IMO.
  • The Features component machine name should be "menu_block" and not "menu_blocks" IMO.
  • The Features list should show the menu block's admin title, not the machine name. (It'll show the machine name in the Features summary on the right.)
  • The admin title should be required. You can have a titleless menu block now.
  • You can't see if a menu block is disabled in the menu blocks list. It's not faded out or anything and there's no Enabled/Disabled/status column.

(These might've been addressed already, but #42 is the only patch that applies for me.)

Just my 2c.

If all those previously confirmed working patches don't work for you it implies that either you're applying to the patch to an incorrect version, or that dev has changed drastically since that patch was created and therefore the patch needs to be rerolled against the latest dev release.

Status:Needs work» Needs review
StatusFileSize
new0 bytes

Patch #50 nor #63 apply to 7.x-2.x HEAD any more. I've rerolled #63. I haven't experienced any of the issues mentioned by comments after that.

StatusFileSize
new35.56 KB

Oops, empty file. Here's a good one.

Status:Needs review» Needs work
StatusFileSize
new26.12 KB

Fatal error: Call to undefined function ctools_export_crud_load() in /var/www/quadrupal7/sites/default/modules/dev/menu_block/menu_block.module on line 163

Missing a ctools_include('export');.

After that, everything from #77 PLUS:

  • My menu block form looks like [attached screenshot] ("Basic options" and "Advanced options" should be tabs or something?)

I'll try to fix:

  • menu block's admin title in features
  • admin title should be required
  • menu block disabled style

StatusFileSize
new854 bytes

I apparently really don't understand ctools. Interdiff for 2 tiny fixes attached.

Found another bug:

  • If you "Sort by" "Title" on admin/structure/menu_block the table is always empty. Resetting will show the menu blocks again.

Status:Needs work» Needs review
StatusFileSize
new35.62 KB

Here is an updated patch incorporating interdiff #82 and fixing the issue with the basic/advanced button styling caused by an invalid filename for the stylesheet.

Fatal error: Call to undefined function ctools_export_crud_load() in /var/www/quadrupal7/sites/default/modules/dev/menu_block/menu_block.module on line 163

Missing a ctools_include('export');.

I can't reproduce this. I see no errors without the ctools_include call. Are you sure you've installed this fresh or done a 'drush updb' ?

I did a drush updb. Every call to the ctools export api needs those includes, so menu_block_get_config() needs it too. I saw it on the frontpage (in the frontend theme). I didn't do a backtrace to find out where menu_block_get_config() was called from.

I cannot reproduce that error without the ctools_include call but since you are not the only person to have reported seeing perhaps another installed module is affecting me.

I do see the "sort by title" issue but that doesn't seem like a roadblock to getting this committed to dev. It won't affect exports. Are there any other showstoppers?

No showstoppers I think, but I haven't tried #83. Are you adding the ctools_include? (That is sort of a fatal showstopper for me.)

I do have a concern about breaking backward compatibilty in menu_block_get_config(). Maybe it should check for schema_version and then return menu_block_config_load() if the update has run... Contrib (and custom) modules will be using the deprecated menu_block_get_config() and there's no real warning... (A watchdog for the site admin, but not for the module contribbers.)

#83 is working fine for me. One minor issue though is that I get a PHP warning when menu_block is enabled and my (custom) features module (the one with the exported menu_block components) is NOT enabled. The warning is:
Invalid argument supplied for foreach() block.module:389
After enabling my (custom) features module, that warning no longer appears.

Hope this helps, thanks for adding this functionality!

Status:Needs review» Needs work

Thanks for adding this functionality! It's a great help and working nearly flawlessly.

There's one thing that makes this feature quite delicate, though, which is the reliance on parent_mlids. Menu link ids are pretty brittle, and can change from build to build depending on your own content and the existence of new menus.

Could we perhaps implement this using the link path instead of parent_mlid? The default menu_features_api that ship with features uses the [menu-delta:link_path] to identify a menu item, which seems to be more robust.

So instead of something like:

  $menu_block = new stdClass();
  $menu_block->disabled = FALSE; /* Edit this to true to make a default menu_block disabled initially */
  $menu_block->api_version = 1;
  $menu_block->name = 'about_menu';
  $menu_block->label = 'About menu';
  $menu_block->description = '';
  $menu_block->depth = 1;
  $menu_block->expanded = 0;
  $menu_block->follow = '0';
  $menu_block->level = 1;
  $menu_block->parent = 'main-menu:348';
  $menu_block->sort = 0;
  $menu_block->title_link = 1;
  $export['about_menu'] = $menu_block;

you could get something like this:

  $menu_block = new stdClass();
  $menu_block->disabled = FALSE; /* Edit this to true to make a default menu_block disabled initially */
  $menu_block->api_version = 1;
  $menu_block->name = 'about_menu';
  $menu_block->label = 'About menu';
  $menu_block->description = '';
  $menu_block->depth = 1;
  $menu_block->expanded = 0;
  $menu_block->follow = '0';
  $menu_block->level = 1;
  $menu_block->parent_path = 'main-menu:node/14';
  $menu_block->sort = 0;
  $menu_block->title_link = 1;
  $export['about_menu'] = $menu_block;

@mariacha1 That should be (and probably is) another issue. This one is about export only, not changing functionality or even fixing bugs. IMO.

Status:Needs work» Needs review

Agreed, marking this back to 'Needs review' as the request seems to be off topic.

Status:Needs review» Needs work

Works great!

* Update path is perfect
* Creating, reverting features is perfect
* UI looks good
* No fatal errors anywhere! =)

There's only 1 issue left IMO:

I do have a concern about breaking backward compatibilty in menu_block_get_config(). Maybe it should check for schema_version and then return menu_block_config_load() if the update has run... Contrib (and custom) modules will be using the deprecated menu_block_get_config() and there's no real warning... (A watchdog for the site admin, but not for the module contribbers.)

Backward compatibility =(

After that, I say do it. I might be wrong about this (I'm not sure I know Drupal's backward compat rules), in which case this is definitely RTBC.

StatusFileSize
new27.17 KB

I tried this patch within a Deploy scenario where the features included menu_block blocks, and there was a small issue with drush site-install. When menu_block is enabled but no blocks have been created yet, i received warnings, ala

  • Invalid argument supplied to foreach() in block.module line 389.

Also, in the UI when working in the Features UI, and menu_bock was enabled but no blocks set up yet, the same error cropped up. Adding back

$blocks = array();

At line 119 in menu_block.admin.inc fixed the warning and allowed menu_blocks to be installed with drush. A patch is attached which is the same as #85 except the addition mentioned.

As a sidenote, I would concur with @mariacha1 that p/mlids are delicate, and that some other robust method of identifying menu links would make this better. But it would be a separate issue, as @rudiedirkx pointed out.

In any case, the patch:

StatusFileSize
new35.59 KB

Garg. Borked. Try this:

@mpgeek

Please, please, please include an interdiff.txt with your patch if it is an update of an existing patch, otherwise it's terribly difficult for anyone to review exactly what changes your patch makes:

StatusFileSize
new510 bytes
new35.59 KB

@Deciphered, thanks for the heads up, here is the patch with the interdiff:

Perfect. Great interdiff @mpgeek ;)

Now all that's left IMO is the backward compat problem of menu_block_get_config() which I won't repeat again. Any thoughts on that?

We can do this before D8 releases! =)

@rudiedirkx, backwards compatibility for getting that config is not a part of my current use case, but checking schema version could be a solution. Does providing an update hook to update the schema not make sense in this case?

The update hook updates the schema and converts existing data (and cleans up). The patch does that. But it also changes the API (or actually it doesn't change the interface, but it does change the return value). So some logic will have to handle the difference between versions (before and after this patch). However checking the schema_version might be very expensive and not the best way. We (someone) could also decide to break backward compatibility (a major release would be necessary IMO) and not change menu_block_get_config().

It's not up to me. I'm just a reminder that there is that horrible backward compatibility to consider.

Like I said before, if backward compat isn't an issue, it's RTBC for me.

While no one but the maintainers/co-maintainers can really make this call completely, my vote is that if backwards compatibility is a requirement to anyone it be dealt with in a separate issue.

I haven't played with this functionality for too long that I myself can't RTBC, but if the code is bug free (passes Coder tests) and works as expected, and you are certain it is worthy, then don't let backwards compatibility hold you back.

Status:Needs work» Reviewed & tested by the community

RTBC it is.

I'll create a compat issue when a maintainer commits this.

Status:Reviewed & tested by the community» Needs work

I tried #96 patch and I noticed that I can't go to blocks page and create menu blocks. It redirects to an path which doesn't have any callbacks.

I'm using the patch at #96 and the menu callback defined at line 53 in menu_block.module redirects from admin/structure/block/add-menu-block to admin/structure/menu_block/add. The redirect is working fine for me, i wonder if there's a caching issue or something preventing the redirect?

What was your version you tried the patch with? At time of testing I was having latest dev version (+37 I think).

I was working on a project that had installed the patch at comment #63, and came across some issues that I filed separately with patch as #2035011: Menu tree ctools content type broken.

Now that I realized (DUH) that I wasn't working on a clean copy of the module, i've marked that issue as a duplicate of this one, and will have to grab the latest patches to see if the problems I encountered are still there. Namely, some functions got removed in the switch-up to exportability that were still being used by the Menu Tree ctools plugin.

+++ b/menu_block.admin.incundefined
@@ -186,12 +117,8 @@ function menu_block_delete_submit($form, &$form_state) {
-  foreach ($deltas AS $delta) {
-    $blocks[$delta]['info'] = _menu_block_format_title(menu_block_get_config($delta));
+  foreach (menu_block_load_all() as $delta => $menu_block) {
+    $blocks[$delta]['info'] = check_plain($menu_block->label);

What is the reasoning to remove the _menu_block_format_title() function and change these lines of code?

Additionally, I've confirmed that the latest patch in #96 still leaves two instances of calls to the _menu_block_format_title function (for which my issue #2035011: Menu tree ctools content type broken was created).

./menu_block.admin.inc:75:  $title = _menu_block_format_title(menu_block_get_config($delta));
./plugins/content_types/menu_tree/menu_tree.inc:144:    $output = _menu_block_format_title($conf);

The question now is should the _menu_block_format_title() function be restored so we don't loose the functionality of this clever naming scheme which shows admins (at a glance) a little more detail about the configuration of each menu block.

This patch also renamed an API function menu_block_configure_form() to menu_block_export_ui_form(); However the menu_tree.inc still references the former function, causing it to be completely broken.

There are actually two instances of mention of this function still exist in the codebase, after applying the patch in #96:

./CHANGELOG.txt:99:  menu_block_configure_form() to make the configuration form reusable by
./plugins/content_types/menu_tree/menu_tree.inc:107:  $form += menu_block_configure_form($form, $sub_form_state);

As mentioned in #103, with the latest patch in #96, when I go to the block admin page and click the 'Add menu Block' link, the link first goes to:

/admin/structure/block/add-menu-block

And then I'm redirected to:

/admin/structure/menu_block/add

Which apparently does not exist, thus, showing me the contents of the /admin/structure page.

Update: re #104, yes, I've cleared the Drupal cache multiple times. Working on latest menu_block 7.x-2.x branch from git, as well as ctools 7.x-1.x git branch.

I dont know much about CTools export ui, but it seems like this chunk of code is not doing its job properly:

<?php
function menu_block_menu_block_ctools_export_ui() {
  return array(
   
'schema' => 'menu_blocks',
   
'menu' => array(
     
'menu prefix' => 'admin/structure',
     
'menu item' => 'menu_block',
     
'menu title' => 'Menu Blocks',
     
'menu description' => 'Administer Menu Blocks.',
    ),
  
//...
}
?>

My question is why did we move the menu_block.inc out of plugins/export_ui (as it was there in previous patches)....

UPDATE: for the record, simply moving the menu_block.inc back to the plugins/export_ui made this work flawlessly. Patch coming.

Status:Needs work» Needs review
StatusFileSize
new11.77 KB
new38.73 KB

This patch does three things over the last one from #96:

* moves the menu_block.inc back to plugins/export_ui to fix the issue of non-working "Add menu block" link on Block ui (comment #103).
* makes the menu_tree.inc ctools content type plugin work (patch adapted from #2035011-2: Menu tree ctools content type broken).
* cleans up unused menu_block_delete() function (delete is now controled by the ctools export ui).

StatusFileSize
new20.19 KB

For some reason that interdiff is not right, here is a better one...

StatusFileSize
new38.58 KB
new928 bytes

Small fix that creates a better pseudo form-state that the new export ui can interpret.

StatusFileSize
new39.66 KB
new2.6 KB

Fixing some more issues on the menu_tree plugin.

Wow, just ran across this thread. Awesome patch.

I've installed and been experimenting with #113 for a few days.

Tested and is working great so far for me. Thanks!

StatusFileSize
new858 bytes
new39.7 KB

Minor tweak to the patch, something that's been bugging me, and is definitely an issue introduce by the patch at some point; Creating a menu block, only setting a Administrative title/Machine name and leaving everything else as default, the Menu is set to 'Main menu' while creating it, but when you edit said menu block the Menu is now set to ''.

The reason is that the default for the Fixed parent item is not in sync with the Menu item, and is not set to sync via JS until one of the fields is changed.

The fix simply sets the default value of the Fixed parent item to be a concatenated value of menu_name and parent_mlid, which is how it was prior to the patches.

I haven't dug any further into the patch at this stage, but will try to do a more thorough review, however I can't really RTBC being the last to provide a patch.

So why does this patch nuke `function menu_block_configure_form()`?

When trying to add a Menu Block content type from Panels, Drupal go boom because that function is missing.

Ah, I was using patch from comment #94 -- the latest patch (#115) doesn't have the same problem, as menu_block_configure_form() apparently doesn't get called anymore.

However, adding menu blocks to panel panes seems mostly broken. I found a "golden path" to add what I needed, but not to ever edit a menu block already added to the page in the backend. (Editing in the Panels IPE works.) Guessing some necessary JS isn't getting loaded on the front end.

Or is that a separate bug not caused by this patch?

If the bug was present before you applied this patch, then it's not an issue with the patch. If the bug was introduced by this patch, then it's an issue with this patch.

If you confirm it's an issue with the patch, please provide as much detail on the issue as you can, otherwise, please open a new issue.

StatusFileSize
new3.53 KB
new41.69 KB

Another fix, if you add a menu block, place it via the Blocks module then delete the menu block there will still be an entry in the block table and the menu block will still try to be rendered.

Fixed in two ways; Block table entry will be deleted when menu block is deleted, if a menu block is trying to be rendered but the config is no longer present it will no longer attempt to do anything and stop throwing errors.

@adamdicarlo: See comment #108 for more about what happened to the API function menu_block_configure_form() (hint, the function was renamed).

I dont remember now what the details of this change were, but if the input/output of the new function is the same as the input/output of the old function, then we should leave the function name the same... if the input/output is different, then the change is justified.

It sounds like you got it figured out anyway now though with the latest patch.

Version:7.x-2.x-dev» 7.x-3.x-dev
Issue summary:View changes

I've always wanted to make these exportable, but probably doing so in the 3.x branch would make the most sense. I don't have a good idea on the difference between those two branches right now, but it should be possible to continue this work there.

I just updated the 7.x-3.x with the current code from 7.x-2.x so this issue should be ready to play with in that branch.

Patch still applies to the 7.x-3.x branch, and it looks like it works as previously did.

@Dave Reid

What would you recommend for developers who want exportable menu_blocks now and are using the 7.x-2.x branch?

Currently #119 applies to the latest 7.x-2.x-dev. I have reservations about upgrade issues down the road if I deploy 7.x-2.x-dev patched with #119.

Chris,

#119 applies against 7.x-3.x, or did when I last commented, and all future rerolls will/should be rolled against 7.x-3.x, so at this stage you should be using 7.x-3.x, which is, or was, essentially the same as 7.x-2.x currently is.

Couple of notes to this patch:

  • active menu parent doesn't seem to work for exportables
  • follow "child" or "active" is exported without quotes
  • menu items are no longer overridable in the db

otherwise, this works fine