Posted by quicksketch on May 19, 2008 at 2:54am
| Project: | MultiBlock |
| Version: | 5.x-1.0 |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
In #259976: Use a form for Block Deletion, an API function is added for instance deletion. This patch is the complement, providing an API function for instance creation. It's completely functionally equivalent to the version before the patch, just breaking out the creation of instances into a new multiblock_add() function.
This patch conflicts with #259976, which will need to be applied first.
| Attachment | Size |
|---|---|
| multiblock-add.patch | 4.3 KB |
Comments
#1
#2
#3
Again, the patch is generally good but I have a couple comments:
1. I think the form function should be named multiblock_add_form since multiblock_form isn't as descriptive
2. Any reason we are converting that array in multiblock_blockinfo_from_form() into an object? I think if anything it's better as an array since it makes it clear it's not the same data structure as from multiblock_get_block
3. I think this somehow overlaps the deletion patch so when that's committed it would be great if you could reroll this one
Any thoughts?
#4
Yep, great comments. I agree on both points and I'll indeed reroll after the deletion patch goes in. Here's a preliminary re-roll with the following changes:
- The form is now named "multiblock_add_form" instead of "multiblock_add"
- multiblock_blockinfo_from_form() now returns an array as it did originally.
- multiblock_add now returns the delta of the newly added block, rather than a boolean.
#5
Everything in this patch looks good to me besides one line:
- !array_key_exists($orig_block['delta'], module_invoke($orig_block['module'], 'block', 'list'))) {+ !array_key_exists($orig_block->delta, module_invoke($orig_block['module'], 'block', 'list'))) {
I think the first reference to $orig_block should treat it as an array (not an object), right? Fix this and also reroll it to apply to the latest CVS and I will commit.
Thanks again!
#6
Ah, very right! Cleaned up and rerolled to work with the latest CVS.
#7
Committed that last patch as-is. Thanks again for everything
#8
Automatically closed -- issue fixed for two weeks with no activity.