Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix oldFormState containing new values #13973

Open
wants to merge 2 commits into
base: 3.x
Choose a base branch
from

Conversation

bumbummen99
Copy link
Contributor

@bumbummen99 bumbummen99 commented Aug 17, 2024

Description

When Livewire tries to update multiple properties in one request, it does trigger an update event for every property.

This will run the updatingInteractsWithForms trait hook and in turn set oldFormState to equal data of the component for every property.

When reacting on a properties updated event, this will cause the expected $old parameter to be wrong, as oldFormState has already been overwritten by updating multiple properties at once so it does include the new and not the old value.

I would propose to only set entries in the oldFormState in case they are not already set, as to prevent the entries being overwritten by new values.

Visual changes

There are none.

Functional changes

  • Only set oldFormState entries in case they are not set
  • Changes have been tested to not break existing functionality.
  • Documentation is up-to-date.

@bumbummen99 bumbummen99 changed the title Update InteractsWithForms.php Fix oldFormState containing new values Aug 17, 2024
@danharrin danharrin added the bug Something isn't working label Aug 18, 2024
@danharrin danharrin added this to the v3 milestone Aug 18, 2024
Copy link
Member

@danharrin danharrin left a comment

Choose a reason for hiding this comment

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

Not every form has a data property, that all depends on the state path of the form.

I am confused about why data would ever be a key in oldFormState, since we are setting the key of the full form component. Are you suggesting that when setting data.one.two, this function is called for data, data.one, AND data.one.two?

@bumbummen99
Copy link
Contributor Author

bumbummen99 commented Aug 18, 2024

Not every form has a data property, that all depends on the state path of the form.

I am confused about why data would ever be a key in oldFormState, since we are setting the key of the full form component. Are you suggesting that when setting data.one.two, this function is called for data, data.one, AND data.one.two?

Yes you are right, so for example if i change data.heading, data.blocks.XYZ.content, and so on it will use the string before . as the key to be set in oldFormState, so only data is set. I'll update the code accordingly.

Edit: Should be correct now. Locally i have used the Arr helper below my changes too but i do not want to change more lines than required.

@danharrin
Copy link
Member

Can you please create a reproduction repository for the original bug? I'm honestly quite lost now, if the entire state path is in the array key then I don't understand how multiple property updates (with different state paths) would overwrite each other, so I'd like to check it out

@bumbummen99
Copy link
Contributor Author

Can you please create a reproduction repository for the original bug? I'm honestly quite lost now, if the entire state path is in the array key then I don't understand how multiple property updates (with different state paths) would overwrite each other, so I'd like to check it out

The overwrite happens because data is written to oldFormState, but data could already have been modified by a previous properties "update cycle". So the first time Filament executes the updating hook, oldFormState['data'] is set from NULL to its initial, old value. But when said hook is executed again, the already modified data is written to oldFormState again. This means, if i run the updating hook over and over, oldFormState will be the "old" data of the last iteration done by Livewire to change the property and therefore include all previous property changes that are actually part of this update request / stack.

I will put up an example repository to illustrate the issue using examples exactly as shown from the V3 documentation.

@bumbummen99
Copy link
Contributor Author

@danharrin I have set up the mentioned repository as well as a test to verify the issue:

https://github.com/bumbummen99/filament-bug-oldFormState

Please take note of the mentioned links in the NewsResource as tose are copy & paste examples from the documentation.

Also i have added an explaination of the issue to the test case:
https://github.com/bumbummen99/filament-bug-oldFormState/blob/master/tests/Feature/Livewire/CreateNewsTest.php

@bumbummen99
Copy link
Contributor Author

Is there anything else I can provide or do to help get this request merged?

@danharrin
Copy link
Member

I haven't had a chance to look at the resources you have provided yet, but when I do I will let you know if I need anything else. Thank you for your patience

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

2 participants