Skip to content

Adds Form Request for Creating Departments #16973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

spencerrlongg
Copy link
Collaborator

This adds a form request for the store department method in the API. For some reason watson\validating wasn't picking up that there was an array in a request which resulted in a 500, this ensures that's caught before we do the ->fill(...).

Adds tests for this as well. For testing errors, ->assertOk (which asserts a 200 status), and assertStatusMessageIs('error') in combination more or less assert one of our validation errors.

@spencerrlongg spencerrlongg requested a review from snipe as a code owner May 21, 2025 18:40
@spencerrlongg spencerrlongg marked this pull request as draft May 21, 2025 18:42
@spencerrlongg
Copy link
Collaborator Author

Moving to draft so I can add in some assertions for the db to ensure values are being stored properly.

@spencerrlongg spencerrlongg marked this pull request as ready for review May 21, 2025 23:21
@spencerrlongg
Copy link
Collaborator Author

Added DbHas assertion

@spencerrlongg
Copy link
Collaborator Author

Also, those attributes need to be added to the API docs as currently only name is there.

*/
public function authorize(): bool
{
return Gate::allows('create', new Department);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably need to allow this for users who can edit departments as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just the store request, update request would be separate

@snipe
Copy link
Member

snipe commented May 22, 2025

I'm not sure a form request is the right move here tbh

@spencerrlongg
Copy link
Collaborator Author

spencerrlongg commented May 27, 2025

I'm not sure, I can see why you'd say that, but I'm still of the mind that form requests are generally good and an improvement in terms of both security and DX (which is obviously subjective).

Security wise in this one I don't like that we were filling with a $request->all() - sure filling should be gated by the fillable property on the model itself, but it still makes me nervous. Filling with $request->validated() absolutely ensures that there's nothing getting into the controller that isn't explicitly already validated, even if the validation is as simple as string.

Next, I can't figure out why watson wasn't catching that values were arrays in the first place, is it because we were filling and filling is doing something db wise now? Was save actually happening before watson? Neither should be the case. Either way, troubleshooting that sounded like I was going to go down a rabbit hole of Xdebug so I went ahead and decided that the tried and true method of form requests was the way to go. Also, this issue is re-creatable. (https://app.shortcut.com/grokability/story/29245/illuminate-database-queryexception-array-to-string-conversion-connection-mysql-sql-insert-into-departments-name-company-id)

I still think that we should continue expanding our form requests to near every request, so as I see issues like this pop up I'm generally going to go that route.

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