Skip to content

Fix addBlocks and add option to append#11

Merged
zunnu merged 2 commits intozunnu:mainfrom
josbeir:main
Sep 1, 2025
Merged

Fix addBlocks and add option to append#11
zunnu merged 2 commits intozunnu:mainfrom
josbeir:main

Conversation

@josbeir
Copy link
Copy Markdown
Contributor

@josbeir josbeir commented Aug 24, 2025

Didn't caught this one in my previous MR.
HtmxComponent::addBlocks() adds its argument array as a child property of the blocks array.

Also added an append option.

Copilot AI review requested due to automatic review settings August 24, 2025 18:59
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in the HtmxComponent::addBlocks() method where the entire array was being added as a single element instead of merging its contents. Additionally, it adds an optional $append parameter to control whether blocks should be appended to existing blocks or replace them entirely.

  • Fixed incorrect array assignment that was adding the entire input array as a nested element
  • Added optional $append parameter with default value of false for backward compatibility
  • Updated parameter name and documentation for clarity

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread src/Controller/Component/HtmxComponent.php
@zunnu zunnu merged commit 23ac160 into zunnu:main Sep 1, 2025
1 check passed
@zunnu
Copy link
Copy Markdown
Owner

zunnu commented Sep 1, 2025

Thanks, I will do a release once I get the tests up.

@skie
Copy link
Copy Markdown

skie commented Sep 4, 2025

This is breaking change!
Better was add setBlocks, or keep merge as true by default!

@zunnu
Copy link
Copy Markdown
Owner

zunnu commented Sep 4, 2025

True default behavior should be to append the block.

@josbeir
Copy link
Copy Markdown
Contributor Author

josbeir commented Sep 4, 2025

@skie Please check the commit, how can this be a breaking change if the original method did not work ? :-)

public function addBlocks(array $block): static
        $this->blocks[] = $block;
}

//result
$blocks = [
   ['block']
];

//should be
$blocks [
   'block'
];

I hope you see the issue ? Maybe you are confusing it with the addBlock (singular) counterpart.

@zunnu agreed :-)

@skie
Copy link
Copy Markdown

skie commented Sep 6, 2025

    public function addBlocks(array $blocks, bool $append = false): static
    {
        if ($append) {
            $this->blocks = array_merge($this->blocks, $blocks);
        } else {
            $this->blocks = $blocks;
        }

        return $this;
    }
Thats what i see in commit. So append should be true by default

@josbeir
Copy link
Copy Markdown
Contributor Author

josbeir commented Sep 7, 2025

    public function addBlocks(array $blocks, bool $append = false): static
    {
        if ($append) {
            $this->blocks = array_merge($this->blocks, $blocks);
        } else {
            $this->blocks = $blocks;
        }

        return $this;
    }
Thats what i see in commit. So append should be true by default

again: the method was broken so your original statement about breaking change does not apply.

Concerning true or false, that's a design choice. But true seems fine by me :-)

@zunnu
Copy link
Copy Markdown
Owner

zunnu commented Sep 14, 2025

Changing it back to true in #13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants