[LiveComponent] Fix issue with onUpdated not triggered on nested array proprety #2013
Conversation
|
Hi @TheDutchScorpion: thanks for this PR. Could you add some test here? (especially one with something that did not work before and now would work?) |
| if (LiveProp::IDENTITY === $propName) { | ||
| if (!$dehydratedUpdatedProps->hasPropValue($frontendName)) { | ||
| if (!$dehydratedUpdatedProps->hasPropValue($frontendName) | ||
| && !$dehydratedUpdatedProps->hasNestedPathsForProperty($frontendName)) { | ||
| continue; |
There was a problem hiding this comment.
I'm not sure this is the right place... for a LiveProp IDENTITY the only thing that can change is... the prop itself no ? Is this not something we should do line 680+ ?
There was a problem hiding this comment.
I don't think thats the best place either to check if there are changes to a array property. My intention was to tigger the onUpdated whenever there is a change within the array without providing a specific nested path.
There was a problem hiding this comment.
Maybe I'm wrong, but it's also strange that if the $propMetadata->onUpdated() is a string, the update hook only triggers if the property has been completely changed to another property and not if part of it has been modified.
There was a problem hiding this comment.
I get that, but i fear doing so here will have side effects .. wdyt ?
|
I just saw you changed a bit your PR :) Could you update your first message with a basic case scenario, describing what you wanted todo, what did not work and what will after your PR ? It's easier for everyone to review and understand the PR :) |
|
Could this branch be reviewed and merged? |
|
I'd like to get the real use case behind this, as i don't see right now how a map can be mapped to a form (without it being a collection) Then i'd like to see is before/after for
I think we need to restrict this syntax to "*" => "method", or that does not make a lot of sense to me. At core, this is a callback when a props is updated, triggered because the value will change. That does not feels logical to call multiple times the same function. Other comments regarding code :) |
| $this->ensureOnUpdatedMethodExists($component, $funcName); | ||
| $propertyOldValue = $dehydratedOriginalProps->getPropValue($key); | ||
| $component->{$funcName}($propertyOldValue); | ||
| foreach ($fullPaths as $fullPath) { |
There was a problem hiding this comment.
This loop concern one callback for a prop. It should not be called multiple times. I this you could "just" replace the prevous "hasPropValue" to some "matchPropValue" or something like this and let the rest flow.
| $this->ensureOnUpdatedMethodExists($component, $funcName); | ||
| $propertyOldValue = $dehydratedOriginalProps->getPropValue($fullPath); |
There was a problem hiding this comment.
Let's not compute this every loop
| foreach ($fullPaths as $fullPath) { | ||
| $this->ensureOnUpdatedMethodExists($component, $funcName); | ||
| $propertyOldValue = $dehydratedOriginalProps->getPropValue($fullPath); | ||
| $component->{$funcName}($propertyOldValue); |
There was a problem hiding this comment.
Here the receiving callable as no way to get what is "old value". If a "*" is used, we should pass the full prop
| @@ -113,6 +113,13 @@ public function getNestedPathsForProperty(string $prop): array | |||
| return $nestedPaths; | |||
| } | |||
|
|
|||
| public function searchFullPathsForProperty(string $prop): array | |||
There was a problem hiding this comment.
Let's add some tests if possible (with failing / problematic cases, not just happy path)
| if (!$dehydratedUpdatedProps->hasPropValue($key)) { | ||
| continue; | ||
| } | ||
| $fullPaths = $dehydratedUpdatedProps->searchFullPathsForProperty($key); |
There was a problem hiding this comment.
Let's do this only under conditions, and keep previous code if no "*"
smnandre
added
Status: Needs Work
|
Upon rereading my message, I realize it came across more strongly than I intended—apologies for that. Sorry as well for the delayed response, and thank you for your contribution @TheDutchScorpion and for the poke @kevinmade |
When modifying a array property the
onUpdatedhook is only triggered when the exact key is specified.This PR will add the ability to use wildcards within the key.
Or