Lomkit / laravel-rest-api

Generate Api in seconds
https://laravel-rest-api.lomkit.com/
MIT License
298 stars 18 forks source link

[1.x] Adjusting comments, applying Laravel standard styling and fixing try Catch #35

Closed rmunate closed 9 months ago

rmunate commented 10 months ago

Hello engineers, I was taking a complete walk through the source code, I wanted to contribute with the adjustment of the comments and styles, this according to the same standards that the Laravel framework uses, I hope that although it is a very small contribution, it is useful and accepted, I hope then have time to be able to provide good and improved features.

GautierDele commented 10 months ago

Thanks @rmunate for the proposal it's a really good idea and work.

Since I added StyleCi on this repository, it made quite a lot of conflicts with your PR, so I did coauthored the rest of the PR on https://github.com/Lomkit/laravel-rest-api/pull/39

About the try catch adding, can you give a bit of context on why you proposed this ? This is to force the rollback I think but i don't see why this is mandatory since the closed request won't commit ?

rmunate commented 10 months ago

@GautierDele I took the time to adjust the conflicts, I hope with this you can consider the idea of combining, in response to your question I saw that you are using DB::beginTransaction() and DB::commit() to start and commit a transaction Database. This is excellent practice, however, it would also be good to handle possible database exceptions with DB::rollback() in case a transaction error occurs. Otherwise, pending transactions may be blocked. I think that any new contributor could intuit this in the same way, however if I am wrong I would apologize and delete that change.

GautierDele commented 9 months ago

@rmunate I had a look at multiple conversations on this and seems like its not necessary: https://stackoverflow.com/questions/4896479/what-happens-if-you-dont-commit-a-transaction-to-a-database-say-sql-server#:~:text=As%20long%20as%20you%20don,be%20rolled%20back%20and%20terminated.

What do you think ?

rmunate commented 9 months ago

@GautierDele Hello engineer. Perfect, in that case I will close the request and I hope to be able to contribute to you in other things.

GautierDele commented 9 months ago

I hope so too ! Thanks