CircleOfNice / DoctrineRestDriver

GNU General Public License v3.0
153 stars 44 forks source link

Handle response codes with greater accuracy #64

Closed robwasripped closed 6 years ago

robwasripped commented 7 years ago

What is this feature about (expected vs actual behaviour)?

Responses from the REST endpoint aren't handled as neatly as they could be:

Does it take minutes, hours or days to fix?

hours

Any additional information?

@TobiasHauck I noticed this comment in the code:

// as the error handling proposed by doctrine
// does not work, we use the way of PDO_mysql
// which just throws the possible errors

Can you expand on that? Is it related to this ticket? I can only presume you've tried implementing these fixes yourself based on that comment.

robwasripped commented 7 years ago

I've also noticed that when populating the result, only the fields requested in the SELECT statement are returned. I would argue that this functionality should be changed as we're dealing with a datasource that can't deal with partials - it's all or nothing with a REST API.

By removing this, we can also remove the overhead of generating the $tokens array a second time and passing it through the result classes.. The only other place where the $tokens array is used as far as I can tell is in determining whether the request was for a SELECT, UPDATE, INSERT, or DELETE query, but I would argue that this can be determined by the request method.

TobiasHauck commented 7 years ago

Well the "official" way is to return the boolean value false to start the Doctrine error handling. But that didn't happen at all. So I had a look at other drivers, especially the official ones. These drivers just threw Exceptions instead of returning false - so I did. This comment is used for people having a deeper understanding of how Doctrine works. They should know I was aware of the official way, but it didn't work :-D

So at least this comment is just a disclaimer: "Hey guys, I know this might be strange for you, I'm sorry!" :-D

TobiasHauck commented 7 years ago

Well if you really want to get just a part of the whole payload it's very pleasing if you can rely on that. What definitely needs to be changed is that PUT is populated with PATCH payload. PUT should send all entity fields, not just the changed ones.

robwasripped commented 7 years ago

I'd like to do the PUT request seperate to this ticket - I'm trying to handle the response more than the request here.

With the token parsing however, I have found that it's also applying some sorting. It looks like a it's done in the case that the endpoint doesn't support sorting. I'm not sure this would be safe to remove as a minor release. I would like to make the token parsing only necessary if sorting was part of the query to reduce processing of other requests.

TobiasHauck commented 7 years ago

That's fine I think