Make handle xml http request response protected

[nieuwenhuisen]

Subject

Change the visibility of the CRUDController methodes handleXmlHttpRequestSuccessResponse and handleXmlHttpRequestErrorResponse from private to protected to allow overriding.

In my application I want to extend the Xml Http response, but there is no way to override it. The only option is to override the editAction but that's not desirable.

I am targeting this branch, because the change is not backwards compatible.

Changelog

### Changed
- `CRUDController::handleXmlHttpRequestSuccessResponse` method is now protected
- `CRUDController::handleXmlHttpRequestErrorResponse` method is now protected

[phansys]

These changes could not be considered BC, since the classes extending CRUDController could have registered the same methods with different signatures or with lower visibilities than protected.

[nieuwenhuisen]

Oke, I have changed the base branch to master

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[VincentLanglet]

These changes could not be considered BC, since the classes extending CRUDController could have registered the same methods with different signatures or with lower visibilities than protected.

I kinda disagree. If you consider this as a BC-break, every new function is a BC-break because we can use the same argument (if a class extend and register the function with différent signature or lower visibility...).

For instance, in https://github.com/sonata-project/SonataAdminBundle/pull/6108/files#diff-97179c4fc4120f802148ade89fc48508R342 we could have said the same.

This can be merged on 3.x IMHO.

[phansys]

For instance, in https://github.com/sonata-project/SonataAdminBundle/pull/6108/files#diff-97179c4fc4120f802148ade89fc48508R342 we could have said the same.

I think technically both changes can lead to the same result, but the difference is here we are changing the visibility of an existing method.
IMO, there are less probabilities of reaching a collision starting from a method with a new name, than with a method with an already known name.
By instance, users generally extend the classes they need, copy and paste the methods or properties the class is requiring to work.
In this case, the most probable scenario is the users who need a custom behavior at handleXmlHttpRequestSuccessResponse() surely have already declared its own implementation.

[VincentLanglet]

IMO, there are less probabilities of reaching a collision starting from a method with a new name, than with a method with an already known name.
By instance, users generally extend the classes they need, copy and paste the methods or properties the class is requiring to work.
In this case, the most probable scenario is the users who need a custom behavior at handleXmlHttpRequestSuccessResponse() surely have already declared its own implementation.

This does not make a real difference to me, we're not suppose to guess the name of the method created by the users.
This is not a BC-break for Symfony symfony/symfony#24206 (the PR was closed for others reason), I don't see why it would be different for us.

It's more important especially since we're trying to close the Api for the next major.
Multiple people will ask for changing private method to protected or protected to public, and we should be able to change the visibility without releasing a new major.

[nieuwenhuisen]

I am not sure. Do I need to change anything to this PR?

[VincentLanglet]

I am not sure. Do I need to change anything to this PR?

For me, this PR can be merge on 3.x. For @phansys, it can't be.
We need to decide with other @sonata-project/contributors

[jordisala1991]

The BC promise we are following is this one: https://symfony.com/doc/current/contributing/code/bc.html#changing-classes

Same as Symfony (which is really restrictive), and we are sometimes making exceptions (like commands)...

About this change:

Private methods
Make public or protected -> Yes (It is allowed)

About the need:

IMO we should not open more the API. We are focusing on master on closing as much as possible.

In conclusion:

If I have to vote on the BC topic, it is allowed (Symfony is allowing it), but for me we shouldn't open more the api right now.

[nieuwenhuisen]

I am looking for an extension point for these methodes.
If you don't want to open the api, is it an option to add a pre methode?

protected function preHandleXmlHttpRequestSuccessResponse(Request $request, $object): JsonResponse
{
}

private function handleXmlHttpRequestSuccessResponse(Request $request, $object): JsonResponse
{
    $preResponse = $this->preHandleXmlHttpRequestSuccessResponse($request, $object);
    if (null !== $preResponse) {
        return $preResponse;
    }
    ...
}

Or is there a better extension point?

[jordisala1991]

Maybe it is not super nice, but in this case, can't you override the renderJson method? or it is private too?

[VincentLanglet]

IMO we should not open more the API. We are focusing on master on closing as much as possible.

I would say that we're trying to close if possible.
But in multiple case, it make sens to me that method are protected instead of private.
For instance, we recently merged this PR: #5965

If someone wants to use our code, with good reason, I think she should always provide an easy way to do it.

In which way you want to override handleXmlHttpRequestSuccessResponse and handleXmlHttpRequestErrorResponse @nieuwenhuisen ?

BTW, you can finally change the branch to 3.x.

[nieuwenhuisen]

I have changed the branch to 3.x

In which way you want to override handleXmlHttpRequestSuccessResponse and handleXmlHttpRequestErrorResponse @nieuwenhuisen ?

I want to add an extra field with the full html render of the form. This makes it easy to fully replace the form with the correct error's visible. We need AJAX for this because we want to update a couple form at the same time.

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[core23]

Can you please rebase, otherwise the CI system won't run and we can't merge your changes @nieuwenhuisen

[phansys]

Forcing the pipeline tasks to be executed again.

[VincentLanglet]

Are you still interested in this PR @nieuwenhuisen ?

[nieuwenhuisen]

Yes, I will do a rebase this week

[VincentLanglet]

Thanks @nieuwenhuisen

[VincentLanglet]

Hi @nieuwenhuisen, can you give an example about how you override the method.
We're trying to close the API for Sonata 4 and I'd like to see if there is another solution for your use case.