[LiveComponent] Prevent LiveCollectionTrait from reusing removed indexes#3465
[LiveComponent] Prevent LiveCollectionTrait from reusing removed indexes#3465Amoifr wants to merge 1 commit intosymfony:3.xfrom
Conversation
carsonbot
added
Bug
| * @var array<string, list<int>> | ||
| */ | ||
| #[LiveProp] | ||
| public array $liveCollectionRemovedKeys = []; |
There was a problem hiding this comment.
@Amoifr Do you think this could be "private" instead of "public"? I can’t think of a case where it would be useful outside this context
There was a problem hiding this comment.
Good catch — I'd love to make it private, but Symfony's PropertyAccess (used by LiveComponentHydrator) can't read or write private properties without an accessor, so the dehydration/rehydration cycle would silently drop the tombstone. I verified it with a tiny repro:
class T {
#[LiveProp] private array $privateArray = ['init' => 1];
#[LiveProp] public array $publicArray = ['init' => 1];
}
$pa = PropertyAccess::createPropertyAccessor();
$pa->getValue(new T(), 'privateArray'); // ❌ "Can't get a way to read the property "privateArray"…"
$pa->getValue(new T(), 'publicArray'); // ✅Every existing #[LiveProp] in the codebase (ComponentWithFormTrait::$formName, $formValues, $isValidated, $validatedFields, ValidatableComponentTrait::$isValidated, …) is public for the same reason. I'll keep public here to match that convention — let me know if you'd rather I add a private + getter/setter pair instead, happy to rework.
There was a problem hiding this comment.
@Amoifr Thanks a lot for the explanation, I didn’t dig deep enough to understand the full context !
There was a problem hiding this comment.
@JacquesMougin you're welcome ! I get to much pain in the first hours of the live component with this kind of things to remember about that 😄
Amoifr
force-pushed
the
fix/live-collection-reuse-removed-index
branch
from
1f55bc4 to
03400ff
Compare
📊 Packages dist files size differenceThanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
3 participants
Problem
LiveCollectionTrait::addCollectionItem()picks the next index as:This only looks at the current keys. If a user removes the last item and then adds a new one, the new item reuses the index that was just freed. When the form is bound to a Doctrine collection, the form machinery matches the new entry back to the (previously removed) entity at that key, so the removed entity silently reappears and no new object is created.
Repro from the issue:
[0, 1, 2].removeCollectionItemon index2→[0, 1].addCollectionItem→max([0,1]) + 1 = 2→ form rebinds to the just-removed entity.Fix
Track indexes freed by
removeCollectionItem()in a#[LiveProp]tombstone keyed by collection field name, and include them in themax()computation ofaddCollectionItem(). After the scenario above, the next index becomes3as expected.$liveCollectionRemovedKeysprop is additive and non-writable; existing serialized state defaults to[]for older clients.Scope
LiveCollectionTypegenerates). String-keyed collections are already unsupported by the existingmax(array_keys(...))logic and are out of scope.Thanks for the Live Component work — it's been a pleasure to dig into. 🙏