Skip to content

Conversation

haddowg
Copy link
Contributor

@haddowg haddowg commented Aug 19, 2024

It is currently impossible to set custom headers on a Resource class response without a custom controller since the specific response object's headers are replaced by the wrapping DataResponse object.

This PR changes the behaviour of witHeaders to merge rather than replace.

BREAKING CHANGE: direct use of withHeaders will now merge rather than replace this may lead to unexpected results

@haddowg
Copy link
Contributor Author

haddowg commented Aug 19, 2024

the more I think about it, i am not sure this would/should be considered breaking. Since the only headers added by the package are the content type and location when required on a 201, if you were calling withHeaders without setting these yourself then having them added should definitely not be breaking.

@lindyhopchris lindyhopchris self-assigned this Aug 20, 2024
@lindyhopchris
Copy link
Contributor

Thanks for the PR! I think this is probably safe to merge as non-breaking. I'm doing JSON:API things Weds evening so will check it over and merge/tag then.

@lindyhopchris
Copy link
Contributor

I'm going to merge this as a non-breaking change, as I believe it's a bug - it should be merging the headers rather than replacing them.

I'll add a withoutHeaders() method so that people can remove any headers they want if they've been relying on this behaviour.

Thanks again for the PR.

@lindyhopchris lindyhopchris merged commit e96381f into laravel-json-api:develop Aug 21, 2024
2 checks passed
@lindyhopchris lindyhopchris changed the title fix!: withHeaders should merge headers rather than replace Bugfix: withHeaders should merge headers rather than replace Aug 21, 2024
@lindyhopchris
Copy link
Contributor

I've tagged this as 4.2.0

@haddowg haddowg deleted the fix/with-headers branch August 21, 2024 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants