-
Notifications
You must be signed in to change notification settings - Fork 367
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
Set section to nullable on attributes #1259
Set section to nullable on attributes #1259
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
0442b91
to
67ae8d1
Compare
67ae8d1
to
d4bb121
Compare
We should probably update the validation as well to allow null. https://github.com/lunarphp/lunar/blob/0.5/packages/admin/src/Http/Livewire/Components/Settings/Attributes/AttributeEdit.php#L79 |
Done. That was a good example of why testing is so important. Obviously there is no such thing here and I'm currently only working with the core package. |
7700618
to
31f152b
Compare
31f152b
to
0539f2a
Compare
0539f2a
to
fbc0580
Compare
Hi @alecritson, would it be possible to merge this pull request or should I add a test for core and admin first? I've also discovered some more sensible but missing default values for some database field definitions like "boolean" values. If I create migrations for it, should I separate them per schema or is it okay for you, to collect them into one file? At some point it will be time to squash migrations anyway. |
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.
Thanks @netzknecht Will get this merged in and ready to go. If you feel like the changes proposed are all related to each other, a single PR should be fine :)
public function down() | ||
{ | ||
Schema::table($this->prefix.'attributes', function ($table) { | ||
$table->string('section')->nullable(false)->change(); | ||
}); | ||
} |
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.
@alecritson this probably shouldn't be needed
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.
Yes, and I can skip something like that if desired? Principle, migrations should also be reversible. Couldn't find any code relating to that column/property, is there any future plan for it? If not, we can simple drop the column.
The table column
section
should benullable
, because it is not a required column e.G. model property. I think, it's only relevant if the hub is used but not required, too. Currently, it has to be set to nonsense or [space] string.https://docs.lunarphp.io/core/reference/attributes.html#attributes-1