Fix injection of not null fields #542
Conversation
| public function isNullable($field) | ||
| { | ||
| return true; // By default, all fields can be null | ||
| return in_array($field, [ |
There was a problem hiding this comment.
For the user, that's a long list. Wouldn't it be easier to manage an exclusion list?
In any case, it needs to be sorted alphabetically, which will make it easier to maintain.
There was a problem hiding this comment.
I did it that way to respect the code already written in this PR :
as there was already an example in this file :
There was a problem hiding this comment.
Your example only has 15 columns, so you can easily display everything on one screen without scrolling, which is not the case here.
For consistency, you can reorder the other case as well.
| public function isNullable($field) | ||
| { | ||
| return true; // By default, all fields can be null | ||
| return in_array($field, [ |
There was a problem hiding this comment.
After running a few tests, only these fields need to be added to the list. The others do not return any errors, so I don't think it's necessary to add them.
public function isNullable($field)
{
return in_array($field, [
'begin_date',
'date_sync',
'end_date',
'last_login',
'substitution_end_date',
'substitution_start_date',
]);
}
Mary-Clb
force-pushed
the
fix/injection-nullable-fields
branch
from
6efb00c to
6ab35aa
Compare
Checklist before requesting a review
Description
glpi_userstable, fields with empty values that are defined asNOT NULLin the database do not take their assigned default values.The fix simply fills the isNullable() function in the corresponding class with the fields that can be null, allowing the plugin to distinguish
NOT NULLfields and ensure they comply with their database constraints.As discussed before with @Lainow:
@stonebuzz @Rom1-B would it be better to fix that in all classes in the PR ? I need a fix for the client but the problem will probably happen again with another class.