Skip to content

Conversation

@llebc
Copy link

@llebc llebc commented May 4, 2022

Hi @lindyhopchris ; hope you don't mind me giving this one ago. I've seen that there's a few issues and want to help as much as possible where I can.

I did some investigating into this one and it's because the way that the regex works within the illuminate source code and when it's split it matches due to the way that the split is done.

As we are only concerned if the header is an exact match to application/vnd.api+json then I would presume it is safe to override the method and check for an exact match disregarding the regex which is there in the illuminate source code.

I've added some tests to back up this which just check for a response with the correct header and a response with an invalid header.

Addresses #184

@lindyhopchris
Copy link
Contributor

Hi! Thanks for taking the time to submit a PR.

I have a preference not to override the Laravel method - because in theory developers could be using that method for other purposes, and would expect it to behave in the way Laravel normally does. So my preference is to change the code in the ResourceRequest::isJsonApi() method.

I was going to leave comments on your PR asking for changes, but I've realised I'd also need to request changes on the tests you wrote as well, as they aren't set up to test the problem that we're trying to fix here. So I'm going to close this PR and push a fix for the problem myself. I am grateful you took the time to put the PR together - it's just it'll be quicker for me to fix this one rather than requesting more of your time to change your PR.

@lindyhopchris
Copy link
Contributor

Actually was good I fixed this, as it turned out the fix needed to be in ResourceQuery::isAcceptableMediaType() - so not even I knew what the fix was until I actually dug into it!

If you're interested, here's the commit that fixed it: 92cd7fc

@llebc
Copy link
Author

llebc commented May 6, 2022

@lindyhopchris Amazing. Thanks for the input and especially linking me to the commit for it.

I use the package for multiple projects so would like to give back as much as possible. If there's any PR's that you would like me to take a look at please do let me know.

@lindyhopchris
Copy link
Contributor

@llebc thanks, your offer of help is really appreciated! I really like your idea to add a filter for a LIKE DB query, so will provide some comments on that as that's something you can progress!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants