Conflict with Pimcore Members bundle - form is always submitted

[dolmit-tanel-paaro]

Hello,

I have encountered an issue that is caused by an override applied by CoreShop ResourceBundle.
Please see this issue I've created on Members bundle github: dachcom-digital/pimcore-members#188

My question is why was HttpFoundationRequestHandler overridden by CoreShop ResourceBundle?
Why is it important that form is always submitted, discarding the request method?

Reference to class signature:

Does not compare the form's method with the request's method.
Always submits the form, even if there are no fields sent.

[dpfaffenbauer]

CoreShop has this since 2.0, I can't remember the reason anymore for it.

Why is Members handling a Request when it hasn't been submitted in the first place? I guess we could also check in CoreShop if it is still needed, since I cannot remember the reason anymore.

[dolmit-tanel-paaro]

Why is Members handling a Request when it hasn't been submitted in the first place?

That's what I asked 😄

It would be best, yeah, if CoreShop team would check if this override is still necessary.
And I'll ask Members bundle team why form is handling the request when it's not necessary.

[solverat]

Nah, it is completely fine to handle the request all the time. In fact, it is not even required to check the method before calling handleRequest, like coreshop does it here:

CoreShop/src/CoreShop/Bundle/CoreBundle/Checkout/Step/CustomerAddressCheckoutStep.php

Lines 115 to 117 in 312c97b

	if ($request->isMethod('post')) {
	$form = $form->handleRequest($request);
	}

As mentioned in the original post, symfony will handle that for you internally:

https://github.com/symfony/symfony/blob/e84257aafc0a77b07257d420447fb56e03ac2518/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php#L51-L53

Besides, it is not even correct to set $request->isMethod('post'), because now a user can't change the method within the form config (Yes, I know, post makes a lot of sense here, but hopefully you get the idea).

[dpfaffenbauer]

#2443 Tests don't fail without it, so I think we don't really need it. I will have to do loads of backend tests, since we don't test that, and we have loads of forms there. I'll keep you posted

[dpfaffenbauer]

Merged now. I couldn't find any side effects of not having it.

[dolmit-tanel-paaro]

Thanks for the PR @dpfaffenbauer.
The PR: #2443