Skip to content

[Form] Use form.post_set_data in ResizeFormListener #51041

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

Merged
merged 1 commit into from
Oct 11, 2024

Conversation

HeahDude
Copy link
Contributor

@HeahDude HeahDude commented Jul 20, 2023

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? yes
Tickets ~
License MIT
Doc PR TODO

The form.pre_set_data event allows to change the initial data, modifying the form based on it should be done early during form.post_set_data instead.

Ref symfony/symfony-docs#18587.

Also, the collect of form config should be done as late as possible to account for modification from other listeners.

@HeahDude HeahDude force-pushed the feat/resize-form-on-post-set-data branch from 0967026 to 8818732 Compare July 21, 2023 12:36
@HeahDude HeahDude force-pushed the feat/resize-form-on-post-set-data branch from 8818732 to ba36dd3 Compare July 24, 2023 07:03
@HeahDude HeahDude force-pushed the feat/resize-form-on-post-set-data branch 2 times, most recently from 42ef705 to 7422f04 Compare August 3, 2023 09:36
@@ -45,18 +50,52 @@ public function __construct(string $type, array $options = [], bool $allowAdd =
public static function getSubscribedEvents(): array
{
return [
FormEvents::PRE_SET_DATA => 'preSetData',
FormEvents::PRE_SET_DATA => 'preSetData', // deprecated
FormEvents::POST_SET_DATA => ['postSetData', 255], // as early as possible
Copy link
Member

@stof stof Aug 3, 2023

Choose a reason for hiding this comment

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

is it actually allowed to modify the structure of the form on postSetData ? To me, this is too late as those child fields won't be populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, the form is not cloned and can be modified by reference https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Form/Form.php#L327.
The only event that forbids it is POST_SUBMIT thanks to this check https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Form/Form.php#L260, in such case $this->defaultDataSet is true, but not enough, only data cannot further be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me, this is too late as those child fields won't be populated.

They will be: https://github.com/symfony/symfony/blob/6.4/src/Symfony/Component/Form/Form.php#L776.

Copy link
Contributor Author

@HeahDude HeahDude Aug 3, 2023

Choose a reason for hiding this comment

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

I've refactored the tests to ensure data is mapped consistently: 6588362.

@HeahDude HeahDude force-pushed the feat/resize-form-on-post-set-data branch from 2662d0b to 6588362 Compare August 3, 2023 12:07
@nicolas-grekas nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023
@OskarStark
Copy link
Contributor

Needs a rebase, thanks

@xabbuh xabbuh modified the milestones: 7.1, 7.2 May 15, 2024
@nicolas-grekas
Copy link
Member

Friendly up @HeahDude

@xabbuh xabbuh force-pushed the feat/resize-form-on-post-set-data branch 6 times, most recently from ca3acd9 to dcf7a29 Compare September 2, 2024 10:34
@xabbuh
Copy link
Member

xabbuh commented Sep 2, 2024

I have rebased the PR

Comment on lines 74 to 75
$this->postSetData($event);
$this->usePreSetData = true;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$this->postSetData($event);
$this->usePreSetData = true;
try {
$this->postSetData($event);
} finally {
$this->usePreSetData = true;
}

}

// Remove FormEvent type hint in 8.0
final public function postSetData(FormEvent|PostSetDataEvent $event): void
Copy link
Member

Choose a reason for hiding this comment

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

@final since Symfony 7.3 insead

Suggested change
final public function postSetData(FormEvent|PostSetDataEvent $event): void
/**
* @param PostSetDataEvent $event
*/
public function postSetData(FormEvent|PostSetDataEvent $event): void

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

LGTM after minor comments.

@xabbuh xabbuh force-pushed the feat/resize-form-on-post-set-data branch from dcf7a29 to 768c6b4 Compare October 11, 2024 18:47
@xabbuh xabbuh force-pushed the feat/resize-form-on-post-set-data branch from 768c6b4 to de06a0f Compare October 11, 2024 18:48
@xabbuh xabbuh added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 11, 2024
@xabbuh
Copy link
Member

xabbuh commented Oct 11, 2024

Thank you Jules.

@xabbuh xabbuh merged commit 23c9d8c into symfony:7.2 Oct 11, 2024
8 of 10 checks passed
@HeahDude HeahDude deleted the feat/resize-form-on-post-set-data branch October 14, 2024 08:31
@HeahDude
Copy link
Contributor Author

Thank you Christian :).

@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Deprecation Feature Form ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants