-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fixed zerofill_count field being ignored during set up #16081
Fixed zerofill_count field being ignored during set up #16081
Conversation
PR Summary
|
Do we want to assume 5 as a zerofill? Some folks don't use one at all |
@snipe that's a good point. I think I pulled that from the column default? Not sure. Either way, I set it to 0 if nothing is provided. ( |
@@ -192,7 +192,7 @@ public function postSaveFirstAdmin(SetupUserRequest $request) : RedirectResponse | |||
$settings->next_auto_tag_base = 1; | |||
$settings->auto_increment_assets = $request->input('auto_increment_assets', 0); | |||
$settings->auto_increment_prefix = $request->input('auto_increment_prefix'); | |||
$settings->zerofill_count = $request->input('zerofill_count', 5); | |||
$settings->zerofill_count = $request->input('zerofill_count') ?: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just:
$settings->zerofill_count = $request->input('zerofill_count', 0);
would work, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so too but when I tried it the result of the function was null
which triggered an exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how we set defaults when no value has been passed everywhere else though. 🤔 I don't see why it wouldn't work here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I figured it out.
These are checkboxes on the front end and if they are unchecked the browser does not include them in the payload:
$settings->full_multiple_companies_support = $request->input('full_multiple_companies_support', 0);
$settings->auto_increment_assets = $request->input('auto_increment_assets', 0);
This is a text input so the browser always includes it but Laravel sets it to null if nothing was input:
$settings->zerofill_count = $request->input('zerofill_count', 0);
So it ends up using null
because it is in the request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, and it looks like that db field is not nullable, hence the error
Currently, any user supplied value for the "Length of asset tags, including zerofill" on the set up page is ignored upon submission. This PR fixes that.