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.
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.
- 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.
- 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.
Comment | File | Size | Author |
---|---|---|---|
#163 | menu_block-2x-ctools_exportables-693302-163.patch | 43.36 KB | rooby |
#161 | menu_block-ctools_exportables-693302-161.patch | 42.85 KB | hswong3i |
#115 | interdiff.txt | 858 bytes | Deciphered |
#113 | interdiff.txt | 2.6 KB | jwilson3 |
#113 | menu_block-ctools-693302-113.patch | 39.66 KB | jwilson3 |
Comments
Comment #1
JohnAlbinCtools/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:
On the fence on this one.
Comment #2
JohnAlbinDecided 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.
Comment #3
JohnAlbinRe-opening for possible 7.x-3.x or 6.x-2.x patches. Submissions welcome!!! :-)
Comment #4
JohnAlbinUm… now patches are welcome. :-D
Comment #5
JohnAlbinJust to be clear, I would also accept patches for the 6.x-2.x branch as well. :-)
Comment #6
NikLP CreditAttribution: NikLP commentedWould be loving some D6 support for this idea!!
Comment #7
nedjoCtools is now updated to D7, see #589636: Port to Drupal 7.
Comment #8
nedjoBecause 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().
Comment #9
dandaman CreditAttribution: dandaman commentedsubcribing
Comment #10
rcross CreditAttribution: rcross commented+1
Comment #11
NikLP CreditAttribution: NikLP commentedIs replacing variables with a custom table a step forward or a step back...?
Comment #12
sunA custom table is a step forward. This kind of configuration does not belong into variables in the first place.
Comment #13
Shadlington CreditAttribution: Shadlington commentedD7 CTools is pretty stable now.
Subbing.
Comment #14
Cyberwolf CreditAttribution: Cyberwolf commentedSubscribing.
Comment #15
fearlsgroove CreditAttribution: fearlsgroove commented@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
Comment #16
NikLP CreditAttribution: NikLP commentedLooks like Entity is going into core anyway, no?Something something - http://drupal.org/node/1018602#comment-5017458If there's a sane, core-centric way to do this that involves a deployment-friendly outcome, then +1 is a no-brainer I think? :)
Comment #17
rogical CreditAttribution: rogical commented+1 features is quite used now
Comment #18
jenlampton+1 for entity api in D7/D8
Comment #19
Torenware CreditAttribution: Torenware commentedAny 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.
Comment #20
TravisCarden CreditAttribution: TravisCarden commented@Torenware: I've "solved" this before by simulating a form submission in a .install file. See the attached example for the fictitious
inglorioushack
module. ;-)Comment #21
AdamGerthel CreditAttribution: AdamGerthel commented@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.
Comment #22
Torenware CreditAttribution: Torenware commented@TravisCarden,
Thanks. Yeah, ugly. But that would work, at least.
Comment #23
Deciphered CreditAttribution: Deciphered commentedFirst attempt at a patch, moves variables into a database table and adds CTools Export and Export UI integration.
Comment #24
rudiedirkx CreditAttribution: rudiedirkx commentedPatched 2.x-dev version with #23. Applies without warnings.
A few functional issues:
Update path doesn't work completely: labels/titles aren't copied, so existing blocks will be titleless on the blocks admin page.The "Administrative title" field should be required IMO. Currently it isn't, so even new blocks can be titleless on the blocks admin page.menu_block_export
(which is good), so remove that from the project? (Paths are actually broken when enabled.)The configure link on the modules page(toadmin/config/user-interface/menu-block
) (via the.info
)is now invalid, so remove or better: change toadmin/structure/menu_block
.And technical:
The db table should be named(singular), because the module's called that. (Like`menu_block`
`node`
, not like`users`
.)All the. Only files with classes should be listed there (for the registry). You can (should) remove all.files[]
entries in the.info
are unnecessary and BAD`block`
table inhook_uninstall
. Blocks are rehashed regularly, so Drupal/blocks.module will fix that.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?
Comment #25
Deciphered CreditAttribution: Deciphered commentedI 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.
Comment #26
rudiedirkx CreditAttribution: rudiedirkx commentedYou're not gonna? :) It's almost perfect!
Comment #27
Deciphered CreditAttribution: Deciphered commentedI 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).
Comment #28
rudiedirkx CreditAttribution: rudiedirkx commentedI'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.
Comment #29
Deciphered CreditAttribution: Deciphered commentedrduiedirkx,
Can you please include an interdiff.txt so I/anyone can see the differences between the two patches (easier to review that way)?
Comment #30
rudiedirkx CreditAttribution: rudiedirkx commentedAlright. I've tried the http://drupal.org/node/1488712 way (with
git show
, which is weird) and my own way (withgit diff
). Both attached. I don't understand thegit 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.Comment #31
rudiedirkx CreditAttribution: rudiedirkx commentedAah 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 amenu_block_load()
(new, with ctools). I broke that in #28.Comment #32
rudiedirkx CreditAttribution: rudiedirkx commentedComment #33
TravisCarden CreditAttribution: TravisCarden commentedRe #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!Comment #34
rudiedirkx CreditAttribution: rudiedirkx commentedNope, 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.
Comment #35
big_smile CreditAttribution: big_smile commentedI have applied the patch in #28.
When I go to update my site using update.php, I get the following error message:
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?)
Comment #36
rudiedirkx CreditAttribution: rudiedirkx commentedYes, #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.
Comment #37
ericaordinary CreditAttribution: ericaordinary commentedJust 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.
Comment #38
big_smile CreditAttribution: big_smile commentedericaordinary, 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!
Comment #39
Deciphered CreditAttribution: Deciphered commentedbig_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.
Comment #40
ericaordinary CreditAttribution: ericaordinary commentedI'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.Comment #41
ericaordinary CreditAttribution: ericaordinary commentedThat 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.
Comment #42
ericaordinary CreditAttribution: ericaordinary commentedBugger, 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.
Comment #43
big_smile CreditAttribution: big_smile commentedFor 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).
Comment #44
ericaordinary CreditAttribution: ericaordinary commented@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?
Comment #45
big_smile CreditAttribution: big_smile commentedThanks 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.
Comment #46
big_smile CreditAttribution: big_smile commentededit: Ignore this post
Comment #47
ericaordinary CreditAttribution: ericaordinary commented@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.
Comment #48
big_smile CreditAttribution: big_smile commentedYes, 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!
Comment #49
Deciphered CreditAttribution: Deciphered commentedUpdated patch, minor cleanup on the CTools Export UI side of things and cleanup of the menu_block_config_load() function.
Still needs work.
Comment #50
Deciphered CreditAttribution: Deciphered commentedOoops... left a DPM in there....
Comment #51
oriol masjuan CreditAttribution: oriol masjuan commentedI 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?
Comment #52
mordonez CreditAttribution: mordonez commentedI tested with dev version and It works. Thanks!
Is possible to make a patch for stable version (2.3)?
Comment #53
Deciphered CreditAttribution: Deciphered commented@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).
Comment #54
oriol masjuan CreditAttribution: oriol masjuan commented@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.
Comment #55
gagarine CreditAttribution: gagarine commented@Deciphered still need work or his ready for testing?
Comment #56
joelcollinsdc CreditAttribution: joelcollinsdc commentedThis 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?
Comment #57
Deciphered CreditAttribution: Deciphered commented@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.
Comment #58
scor CreditAttribution: scor commented#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.
Comment #59
gagarine CreditAttribution: gagarine commented@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.
Comment #60
gagarine CreditAttribution: gagarine commentedSometimes I get this error (after clear cache). I also use http://drupal.org/project/features_extra..
Comment #61
gagarine CreditAttribution: gagarine commentedNeed to add ctools_include('export'); in menu_block_config_load
Comment #62
rvilar@gagarine Can you upload a patch with this code?
Comment #63
gagarine CreditAttribution: gagarine commentedYes this is the
#51#50 with my modification... no time for inter diff and so (sorry).Comment #64
lee20 CreditAttribution: lee20 commentedThe patch from #63 applied with no problems. But when I went to edit a menu block block, I got the following error:
Comment #65
DYdave CreditAttribution: DYdave commentedHi 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!
Comment #66
Deciphered CreditAttribution: Deciphered commentedHi 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.
Comment #67
acrollet CreditAttribution: acrollet commentedI'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.
Comment #68
acrollet CreditAttribution: acrollet commentedlee20: 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.
Comment #69
dkingofpa CreditAttribution: dkingofpa commentedUsing latest 7.x-2.x (from 08/31/2012) and patch from #63
http://drupalcode.org/project/menu_block.git/snapshot/fa9065e4b8182c9daa...
Trying to get property of non-object in menu_block_config_load()
(see attached)I wasn't even able to test the features export of Menu Blocks. Version 7.x-2.3 worked fine (menu showed up).
Comment #70
dkingofpa CreditAttribution: dkingofpa commentedNo "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:
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.
Comment #71
Deciphered CreditAttribution: Deciphered commented@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.
Comment #72
rudiedirkx CreditAttribution: rudiedirkx commentedI'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.
mlid
that's not 0. That's how menu_block works and I haven't changed that. Featurizing a menu block with amlid
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 =)
Comment #73
Deciphered CreditAttribution: Deciphered commentedHi 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.
Comment #74
rudiedirkx CreditAttribution: rudiedirkx commentedI 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?
Comment #75
Deciphered CreditAttribution: Deciphered commentedRule 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.
Comment #76
rudiedirkx CreditAttribution: rudiedirkx commented#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.
Comment #77
rudiedirkx CreditAttribution: rudiedirkx commented#42 works and looks great. Just the following:
(These might've been addressed already, but #42 is the only patch that applies for me.)
Just my 2c.
Comment #78
Deciphered CreditAttribution: Deciphered commentedIf 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.
Comment #79
tangent CreditAttribution: tangent commentedPatch #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.
Comment #80
tangent CreditAttribution: tangent commentedOops, empty file. Here's a good one.
Comment #81
rudiedirkx CreditAttribution: rudiedirkx commentedMissing a
ctools_include('export');
.After that, everything from #77 PLUS:
I'll try to fix:
Comment #82
rudiedirkx CreditAttribution: rudiedirkx commentedI apparently really don't understand ctools. Interdiff for 2 tiny fixes attached.
Found another bug:
Comment #83
tangent CreditAttribution: tangent commentedHere 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.
Comment #84
tangent CreditAttribution: tangent commentedI 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' ?
Comment #85
rudiedirkx CreditAttribution: rudiedirkx commentedI 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 wheremenu_block_get_config()
was called from.Comment #86
tangent CreditAttribution: tangent commentedI 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?
Comment #87
rudiedirkx CreditAttribution: rudiedirkx commentedNo 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 returnmenu_block_config_load()
if the update has run... Contrib (and custom) modules will be using the deprecatedmenu_block_get_config()
and there's no real warning... (A watchdog for the site admin, but not for the module contribbers.)Comment #88
brockfanning CreditAttribution: brockfanning commented#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!
Comment #89
mariacha1 CreditAttribution: mariacha1 commentedThanks 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:
you could get something like this:
Comment #90
rudiedirkx CreditAttribution: rudiedirkx commented@mariacha1 That should be (and probably is) another issue. This one is about export only, not changing functionality or even fixing bugs. IMO.
Comment #91
Deciphered CreditAttribution: Deciphered commentedAgreed, marking this back to 'Needs review' as the request seems to be off topic.
Comment #92
rudiedirkx CreditAttribution: rudiedirkx commentedWorks great!
* Update path is perfect
* Creating, reverting features is perfect
* UI looks good
* No fatal errors anywhere! =)
There's only 1 issue left IMO:
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.
Comment #93
mpgeek CreditAttribution: mpgeek commentedI 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, alaInvalid 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
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:
Comment #94
mpgeek CreditAttribution: mpgeek commentedGarg. Borked. Try this:
Comment #95
Deciphered CreditAttribution: Deciphered commented@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:
Comment #96
mpgeek CreditAttribution: mpgeek commented@Deciphered, thanks for the heads up, here is the patch with the interdiff:
Comment #97
rudiedirkx CreditAttribution: rudiedirkx commentedPerfect. 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?Comment #98
rudiedirkx CreditAttribution: rudiedirkx commentedWe can do this before D8 releases! =)
Comment #99
mpgeek CreditAttribution: mpgeek commented@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?
Comment #100
rudiedirkx CreditAttribution: rudiedirkx commentedThe 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 changemenu_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.
Comment #101
Deciphered CreditAttribution: Deciphered commentedWhile 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.
Comment #102
rudiedirkx CreditAttribution: rudiedirkx commentedRTBC it is.
I'll create a compat issue when a maintainer commits this.
Comment #103
iMiksuI 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.
Comment #104
mpgeek CreditAttribution: mpgeek commentedI'm using the patch at #96 and the menu callback defined at line 53 in
menu_block.module
redirects fromadmin/structure/block/add-menu-block
toadmin/structure/menu_block/add
. The redirect is working fine for me, i wonder if there's a caching issue or something preventing the redirect?Comment #105
iMiksuWhat was your version you tried the patch with? At time of testing I was having latest dev version (+37 I think).
Comment #106
jwilson3I 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.
Comment #107
jwilson3What 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).
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.
Comment #108
jwilson3This patch also renamed an API function
menu_block_configure_form()
tomenu_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:
Comment #109
jwilson3As 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:
And then I'm redirected to:
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:
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.
Comment #110
jwilson3This 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).Comment #111
jwilson3For some reason that interdiff is not right, here is a better one...
Comment #112
jwilson3Small fix that creates a better pseudo form-state that the new export ui can interpret.
Comment #113
jwilson3Fixing some more issues on the menu_tree plugin.
Comment #114
setvik CreditAttribution: setvik commentedWow, 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!
Comment #115
Deciphered CreditAttribution: Deciphered commentedMinor 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.
Comment #116
adamdicarlo CreditAttribution: adamdicarlo commentedSo 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.
Comment #117
adamdicarlo CreditAttribution: adamdicarlo commentedAh, 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?
Comment #118
Deciphered CreditAttribution: Deciphered commentedIf 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.
Comment #119
Deciphered CreditAttribution: Deciphered commentedAnother 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.
Comment #120
jwilson3@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.
Comment #121
Dave ReidI'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.
Comment #122
Dave ReidI 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.
Comment #123
Deciphered CreditAttribution: Deciphered commentedPatch still applies to the 7.x-3.x branch, and it looks like it works as previously did.
Comment #124
Chris Burge CreditAttribution: Chris Burge commented@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.
Comment #125
Deciphered CreditAttribution: Deciphered commentedChris,
#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.
Comment #126
basvredelingCouple of notes to this patch:
otherwise, this works fine
Comment #127
realityloopreroll of 119
Comment #128
Deciphered CreditAttribution: Deciphered commentedPatch #127 is broke, missing a whole file. Incoming fix.
Comment #129
Deciphered CreditAttribution: Deciphered commentedUpdated patch against 7.x-2.x, 7.x-3.x is just falling behind, but i'll keep it as the active versions.
Comment #130
realityloopReroll at #129 is working
Comment #131
Dave ReidNot sure if this still applies to 7.x-2.x/7.x-3.x.
Comment #155
Dave ReidComment #156
Dave ReidOk I'd love to officially start making a push for this in 7.x-3.x so that we can start porting to Drupal 8, which has all the latest 7.x-2.x code added to it.
Comment #157
netw3rker CreditAttribution: netw3rker commentedIt took quite a bit of work, but I managed to get this to work against 7.x-2.x and 7.x-3.x. The code has been through quite a bit of reorganizing since the patch was written.
A couple of notes:
1) I can't help but think this is overkill. This really shouldn't require a database table to track and manage a few settings. The original way of using a few variables seems like a much lighter weight option, and using the features export api rather than a direct ctools implementation might be quicker and easier.
2) this is using a magic number, rather than machine identifiable value for the advanced option of "target menu item". This really should be the path, not the MLID since those numbers are extremely not constant.
I'll see what I can do about re rolling with a path rather than mlid & see what happens. Hope this helps get things rolling!
Comment #158
netw3rker CreditAttribution: netw3rker commented.... aaaand here's the patch with the MLID fix in it. With this change, the "fixed parent item" option under advanced now exports as "[menu]:[link_path]" which doesn't vary by install/environment. There are a couple of todo items left here:
1) the size of the column that stores the parent needs to be resized to fit the menu_name + up to a 255 character url.
2) the 7004 update needs to convert the [menu]:[mlid] to the new format [menu]:[link_path]
hope this helps!
Comment #159
somya4c CreditAttribution: somya4c commentedError: After using https://www.drupal.org/commitlog/commit/60023/b4ee393d2e843be36f66869e24... patch file, when i add a new menu block then the preferred menus used by
always select development rather than main menu.
Comment #160
hswong3i CreditAttribution: hswong3i commentedschema.module report following error after apply #158:
Comment #161
hswong3i CreditAttribution: hswong3i commentedBased on #158, fix schema issue reported by #160
Comment #162
rooby CreditAttribution: rooby commentedThe patch currently doesn't apply to the 2.x branch.
What is the status of the 3.x branch? Should I stay away for production sites?
Comment #163
rooby CreditAttribution: rooby commentedHere is #161 rerolled for 2.x.
Haven't tested yet.
Comment #165
rooby CreditAttribution: rooby commentedWhoops I forgot the do not test.
Comment #166
joachim CreditAttribution: joachim commentedThat's going to fail because the test bot applies patches to the branch the issue is set to.
It would be great to see this in 2.x, but it needs to get resolved for 3.x first.
Comment #167
rooby CreditAttribution: rooby commentedYeah I had meant to put do-not-test in the name to avoid that.
Comment #169
mansspams CreditAttribution: mansspams at Wunder commentedThis brakes functionality - no menus are shown in blocks, menu selection resets to (in my case) development menu.
Comment #170
rooby CreditAttribution: rooby commentedWithout having looked at the actual code, is that commit for D8 relevant here, or is it so different so as not to influence the approach for D7?
Should we back port that or just continue on with these patches?
Comment #171
JrgenGNielsen CreditAttribution: JrgenGNielsen at DBC commentedNote that if you've made an installation using menu_block-ctools-693302-96.patch, and later try to update the module using menu_block-2x-ctools_exportables-693302-163.patch, your code will break.
The first patch create a 'menu_blocks' database table in menu_block_update_7203(); the other does it in menu_block_update_7204()