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.

CommentFileSizeAuthor
#6 multiblock_add.patch3.47 KBquicksketch
#4 multiblock_add.patch4.1 KBquicksketch
multiblock-add.patch4.3 KBquicksketch

Comments

quicksketch’s picture

quicksketch
he/him
commented
Title: Create an API function for adding and instance » Create an API function for adding an instance
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Active » Needs review
andrewlevine’s picture

andrewlevine commented
Status: Needs review » Needs work

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?

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new multiblock_add.patch4.1 KB

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.

andrewlevine’s picture

andrewlevine commented
Status: Needs review » Needs work

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!

quicksketch’s picture

quicksketch
he/him
commented
Status: Needs work » Needs review
StatusFileSize
new multiblock_add.patch3.47 KB

Ah, very right! Cleaned up and rerolled to work with the latest CVS.

andrewlevine’s picture

andrewlevine commented
Status: Needs review » Fixed

Committed that last patch as-is. Thanks again for everything

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.