Enhance save action return type

[jnoordsij]

Note: please merge #4480 first; it contains the first (non-breaking) commit of this change.

WHY

BEFORE - What was wrong? What was happening before this PR?

The performSaveAction returned either a \Illuminate\Http\RedirectResponse or an array, with the latter being auto-converted into a JSON response by Laravel. This seems to be a bit ambiguous.

AFTER - What is happening after this PR?

The array is manually converted into a \Illuminate\Http\JSONResponse instance, which seems more consistent with the other possible return type.

HOW

Is it a breaking change?

Technically yes; however only those who were manually using the return result AND checking for it to be an array, then treating it as an array. Since this was never before listed as possible return type, this seems unlikely.

[pxpm]

Hey @jnoordsij I 100% agree with this change, but not sure we can consider it non-breaking as like you said, someone could be doing:

$response = $this->crud->performSaveAction();
// request wants json
if(is_array($response)) {
    $response = array_merge($response, $myCustomConfigs);
}
return $response;

But I really think it's something we should be doing in the next version as I also agree with you that it would be more consistent.

Will be moving this to v6, feel free to disagree with me if you think I am wrong.

Thanks for the contribution 🙏

[jnoordsij]

Hey @jnoordsij I 100% agree with this change, but not sure we can consider it non-breaking as like you said, someone could be doing:

$response = $this->crud->performSaveAction();
// request wants json
if(is_array($response)) {
    $response = array_merge($response, $myCustomConfigs);
}
return $response;

But I really think it's something we should be doing in the next version as I also agree with you that it would be more consistent.

Will be moving this to v6, feel free to disagree with me if you think I am wrong.

Thanks for the contribution 🙏

That seems fair to me. I think chances of someone doing those things are slim, but being careful doesn't sound bad to me.

[jnoordsij]

With v6 coming up, is this ready to merge? Or is there some other branch I can target this on to include it in v6? 😇

[pxpm]

Hey @jnoordsij

We expect to have the alpha/beta out next week in a separate branch. (it's already usable, but is a bit confusing to install because it's split in multiple smaller branches).

Next week we get back to this 🙏 Thank you

Cheers 🥂

[jnoordsij]

Rebased this onto the v6 branch to ensure everything still runs, and changed target to v6.

[tabacitu]

Thanks @jnoordsij - this is FINALLY going to see the light of day in 2 weeks 🎉🙏