Posted by agentrickard on January 12, 2012 at 7:23pm
10 followers
| Project: | Migrate |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (to be ported) |
Issue Summary
Looks like I'll be needing this for a project. In effect, I have an XML document that defines a menu hierarchy. Parsing that as a source is handled, but we need a target mapping for the menu system.
Filing this issue in case anyone is also working on this problem.
EDIT: Since I cannot guarantee that all items will be nodes, I don't think I can group this logic with a node importer. I think a generic menu destination would be more useful.
Comments
#1
Looking again. Is it possible to just fake this by using the MigrateDestinationTable class?
That is, define the {menu_links} table in the import class and leverage MigrateDestinationTable, since we're directly writing to a single table?
#2
There can be a bunch of hooks that fire when you save menu items and menu links. I suggest writing a dest plugin that calls the right menu CRUD API functions.
Caveat: I dunno if those API functions call menu_rebuild() but if they do thats a performance killer.
#3
Yeah, good point about the API functions.
And, yes, menu_link_save() does a cache clear, and clears the menu cache of the affected target menu. The code doesn't force a menu_rebuild() when it is called, but when the next page is generated.
I'll do some testing.
#4
And we actually need menu.inc and menu_links.inc, which are separate targets.
#5
Technical question here.
I have both plugins working (mostly) and wonder what the Migrate policy is here.
How is this kind of cross-dependency normally handled? Is the migration class (for the specific project) supposed to reconcile this, or do I write a handler to track the dependency?
#6
I will be very glad to review/continue this.
@agentrickard: It would be great if you can post a patch soon, so I do not start from scratch.
#7
Here they are. I didn't bother rolling as a patch, largely because I haven't figured out how to make a git patch for new files ;-(.
To use, download, rename to .inc and stick in a plugins directory for migrate. Be sure to register them in a .info, too.
#8
Ok, I will test this soon, in the meantime a patch.
#9
Sorry, I will include the info file changes on the next patch.
#10
Ok, here the same patch with the info file additions.
I have just tested
MigrateDestinationMenuand it works fine.As mentioned the only really thing to change could be replacing the
menu_{save,delete}()functions with custom ones that clone its functionality except from the call tomenu_cache_clear_all(), and then onMigrateDestinationMenu::postImport()for menu_save clone and onMigrateDestinationMenu::postRollback()for menu_delete clone call the cache clear.I guess that's the only way to avoid rebuilding the cache for each menu_custom.
Please let me know about your opinions on that approach for menu_custom's and I could code that change(I am importing too little menu_custom's to really see the impact).
I will review the
MigrateDestinationMenuLinkssoon.#11
Copied from table_copy.inc, eh?;) There's some cruft that's not relevant for your plugin - specifically, setting tableName and keySchema, for example, which are specific to table_copy.inc.
There's no reason to use bulkRollback without a bulk API to call - just implement rollback(), deleting one menu/link at a time.
In the prepare(Rollback) and complete(Rollback) methods, you're passing 'Menu' and 'MenuLinks' as the destination type - this corresponds to the values passed to registerTypes and the convention is lower-case underscore delimited: menu and menu_links.
Thanks.
#12
Thanks. It does work, but needs some cleanup. Rollback support isn't real clear in the docs.
#13
@mikeryan - So are you recommending deleting bulkRollback()? That's what I interpreted from your comment.
You also talk about prepare(Rollback) and complete(Rollback) methods, in moving the registerTypes convention to lower-case underscore delimited (menu and menu_links), I'm assuming this means:
migrate_handler_invoke_all('Menu', 'completeRollback', $menu_names);migrate_handler_invoke_all('Menu', 'complete', $object, $row);
&
migrate_handler_invoke_all('MenuLinks', 'prepare', $object, $row);migrate_handler_invoke_all('MenuLinks', 'complete', $object, $row);
but also in menu.inc the same functions need to be changed in prepareRollback() & completeRollback().
I'm still exploring this module, but need this function, so am trying to help this issue along.
Here's a patch for review though based on your feedback.
#14
I didn't mean to remove rollback capability completely - just implement rollback() (one item at a time) instead of bulkRollback() (which only makes sense where the core API has a bulk delete function).
In the MigrateDestinationMenuLinks prepareRollback() and completeRollback methods(), the first argument to migrate_handler_invoke_all() should be menu_links (matching the calls in prepare() and complete()).
#15
Ah, the developer API / example docs weren't real clear on which rollback method to use, so I just picked one.
#16
I've update the menu_links destination a little.
It's working well for saving menu link items.
What is needed is some help with Stub migrations.
Some menu items are imported before their parents.
I've implemented a createStub function in my custom class that extends MigrateDestinationMenuLinks.
And it does create a stub menu link when it runs.
But it doesn't update the Stub when the parent link is imported later.
Any pointers appreciated.
#17
This one works with Stub migrations too.
#18
Now with a proper patch
#19
MigrationDestinationMenuLinks should implement rollback(), instead of bulkRollback(), as per Mike's comment in #14.
MigrationDestinationMenu doesn't seem to implement rollback at all.
#20
The original in #7 has bulkRollback support, but that needs to be replaced by rollback().
#21
Thanks. I'll work on switching menu and menu_links to rollback(), then.
#22
Ok, I took the patch in #18, implemented rollback() for menu.inc, and also tweaked and added comments to prepareRollback() and completeRollback(). The rest should be the same as before. I'll do the same for menu_links.inc tomorrow.
#23
And now with rollback() implemented for menu_links.inc.
Other than that, I did a basic implementation for menu and menu links, and it works for me so far. I'll try something more complicated next that forces menu link stubs to be generated.
#24
Alan, stub generation works for me as well. I've cleaned up the comments some more, and reordered a couple functions so that menu and menu_links are consistent. This patch is ready for review.
Here's how I implemented createStub(), in case anyone else wants to use it to test with. This works for a D6 to D7 migration. For other sources, just change plid and menu_name to the right names.
<?php/**
* Creates a stub menu link, for when a child is imported before its parent
*
* @param $migration
* The source migration
* @return
* int $mlid on success
* FALSE on failure
*/
protected function createStub($migration) {
// if plid is 0, that means it has no parent, so don't create a stub
if (!$migration->sourceValues->plid) {
return FALSE;
}
$menu_link = array (
'menu_name' => $migration->sourceValues->menu_name,
'link_path' => 'stub-path',
'router_path' => 'stub-path',
'link_title' => t('Stub title'),
);
$mlid = menu_link_save($menu_link);
if ($mlid) {
return array($mlid);
}
else {
return FALSE;
}
}
?>
Edit: removed menu_cache_clear_all(), as the cache is now cleared in postImport() per #25.
#25
Some cleanup - comments, variable names changes, ... The one meaningful change is to do menu_cache_clear() in postImport/postRollback, so it only gets done once per import process, not for every item imported.
Untested... Sam?
#26
Tested and works as expected.
For menu.inc:
menu_cache_clear_all() is called on import and rollback, once for each menu.
For menu_links.inc:
menu_cache_clear_all() is called once during postImport and postRollback, rather than for each link.
#27
Committed and pushed for D7, thanks to all who contributed!
Leaving open for backport to D6, should anyone be motivated...
#28
I have just created a follow up on #1621906: Support destination system of record for menu links migrations