[27]: add RemoveArrayItem visitor

[neellii]

refs: #27

[coveralls]

Pull Request Test Coverage Report for Build 20020770879

Details

• 33 of 33 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.4%) to 98.571%

---

Totals
Change from base Build 19623686606:	0.4%
Covered Lines:	138
Relevant Lines:	140

---

💛 - Coveralls

[Copilot]

Pull request overview

This PR adds a new removeArrayPropertyItem method to PHPFileBuilder that allows removing specific items from array properties in PHP classes and traits. This complements the existing addArrayPropertyItem functionality and addresses issue #27.

Key Changes

• Added RemoveArrayPropertyItem visitor class that extends SetPropertyValue to filter out specified items from array properties
• Integrated the new visitor into PHPFileBuilder with a fluent removeArrayPropertyItem() method
• Added comprehensive test coverage including edge cases (non-existent properties, non-array properties, nested arrays)

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Visitors/RemoveArrayPropertyItem.php	New visitor class implementing array item removal logic with node comparison methods
src/PHPFileBuilder.php	Added removeArrayPropertyItem() method that instantiates and registers the new visitor
tests/PHPFileBuilderTest.php	Added three test methods covering normal operation, exception handling, and non-existent property scenarios
tests/fixtures/PHPFileBuilderTest/original/*.php	Updated/added fixture files to support testing the removal functionality
tests/fixtures/PHPFileBuilderTest/results/*.php	Updated/added expected result fixtures showing proper item removal behavior
README.md	Added brief documentation for the new removeArrayPropertyItem method

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The areArrayNodesEqual method only checks if all expected items exist in the actual array, but doesn't verify that both arrays have the same length. This allows partial matches where the expected array is a subset of the actual array.

For example, if expected is [1, 2, 3, 4] and actual is [1, 2, 3], the method would fail correctly (index 3 returns null). But if expected is [1, 2] and actual is [1, 2, 3], it would incorrectly return true, treating it as a match.

Add a length check before the item comparison:

protected function areArrayNodesEqual(Array_ $expected, Array_ $actual): bool
{
    if (count($expected->items) !== count($actual->items)) {
        return false;
    }
    
    return Arr::every($expected->items, function ($expectedItem, $index) use ($actual) {
        $actualItem = Arr::get($actual->items, $index);

        return !is_null($actualItem) && $this->areNodesEqual($expectedItem->value, $actualItem->value);
    });
}

[Copilot]

Arr::where() returns an array with preserved keys, which can cause issues with array indices. When removing items from an array that uses numeric keys, the indices should be re-indexed to maintain contiguous numbering (0, 1, 2, ...).

Use array_values() to re-index the array after filtering:

$arrayProperty->items = array_values(Arr::where(
    array: $arrayProperty->items,
    callback: fn (Node $item) => !$this->shouldRemoveItem($item),
));

Suggested change

	$arrayProperty->items = Arr::where(
	array: $arrayProperty->items,
	callback: fn (Node $item) => !$this->shouldRemoveItem($item),
	);
	$arrayProperty->items = array_values(Arr::where(
	array: $arrayProperty->items,
	callback: fn (Node $item) => !$this->shouldRemoveItem($item),
	));

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

In the callback, $expectedItem could be null (PHP-Parser allows null items in Array_ nodes). Accessing $expectedItem->value without a null check will cause a fatal error. Add a null check before accessing the value property: return !is_null($expectedItem) && !is_null($actualItem) && $this->areNodesEqual($expectedItem->value, $actualItem->value);

[Copilot]

The method signature and implementation may be confusing for associative arrays. When calling removeArrayPropertyItem('prop', ['key' => [1, 2]]), the 'key' is ignored and only the value [1, 2] is used for matching. This means any entry in the array with value [1, 2] will be removed regardless of its key. While this works, it could be unintuitive for users who expect the key to be considered. Consider either documenting this behavior clearly or modifying the implementation to respect keys when they are provided in the removal specification.

[Copilot]

The documentation for removeArrayPropertyItem is incomplete. It should clarify that if the specified property doesn't exist, the operation is silently skipped (no error is thrown and no property is created). Consider adding: "Remove items from the array class property. If the property does not exist, no action is taken."

[DenTray]

it would be better to implement new abstract class PropertyVisitor class and use it as parent of this one and SetPropertyValue visitor

[DenTray]

it would be better to allow to receive null in exception

Suggested change

	actualType: (is_null($node->type)) ? 'null' : (string) $node->type,
	actualType: $node->type,

[DenTray]

Suggested change

	$arrayProperty->items = array_values(Arr::where(
	array: $arrayProperty->items,
	callback: fn (Node $item) => !$this->shouldRemoveItem($item),
	));
	$arrayProperty->items = array_filter($arrayProperty->items, fn (Node $item) => !$this->shouldRemoveItem($item));

[DenTray]

could we use something like this?

Suggested change

	return match (true) {
	$expected instanceof Scalar && $actual instanceof Scalar => $expected->value === $actual->value,
	$expected instanceof ConstFetch && $actual instanceof ConstFetch => $expected->name->name === $actual->name->name,
	$expected instanceof Array_ && $actual instanceof Array_ => $this->areArrayNodesEqual($expected, $actual),
	default => false,
	};
	return $this->getPropertyValue($expected) === $this->getPropertyValue($actual);