Crud: [Feature][3.5] Setting empty fields to null

[indosean]

Bug report

At part of #94 changes were made to the storeCrud and updateCrud methods which

// replace empty values with NULL, so that it will work with MySQL strict mode on

See https://github.com/Laravel-Backpack/CRUD/blob/master/src/app/Http/Controllers/Operations/Update.php#L52

This is now in Laravel 5.6 as the ConvertEmptyStringsToNull middleware.

What I did:

Have optional fields in my DB table and fields that are "NOT NULL".

I use a validation rule of: 'country' => 'string|nullable|max:20',

What I expected to happen:

The optional field should save an empty string to the DB.

What happened:

I got a Integrity constraint violation: 1048 Column 'country' cannot be null error.

What I've already tried to fix it:

I had to write Model Mutators to override this.

I would like to be able to use the standard Laravel functionality or have a setting to deactivate this code in the methods mentioned.

Backpack, Laravel, PHP, DB version:

• Laravel: 5.6
• Backpack CRUD: 3.4.25
• PHP: 7.2
• MySQL: 5.7

Using Laravel Homestead.

[welcome[bot]]

Hello there! Thanks for opening your first issue on this repo!

Just a heads-up: Here at Backpack we use Github Issues only for tracking bugs. Talk about new features is also acceptable. This helps _a lot_ in keeping our focus on improving Backpack. If you issue is not a bug/feature, please help us out by closing the issue yourself and posting in the appropriate medium (see below). If you're not sure where it fits, it's ok, a community member will probably reply to help you with that.

Backpack communication mediums:

• Bug Reports, Feature Requests - Github Issues (here);
• Quick help (_How do I do X_) - Gitter Chatroom;
• Long questions (_I have done X and Y and it won't do Z wtf_) - Stackoverflow, using the backpack-for-laravel tag;

Please keep in mind Backpack offers no official / paid support. Whatever help you receive here, on Gitter, Slack or Stackoverflow is thanks to our awesome _awesome_ community members, who give up some of their time to help their peers. If you want to join our community, just start pitching in. We take pride in being a welcoming bunch.

Thank you!

--
Justin Case
The Backpack Robot

[lloy0076]

Personally I think that line is a bug and that the relevant line shouldn't be there.

[AbbyJanke]

Just a thought.. but why not just make the column in your database be nullable? I don't understand that use case of an empty string within the database.

[indosean]

@AbbyJanke well if Laravel itself is allowing an override (https://laravel.com/docs/5.6/validation#a-note-on-optional-fields) then I think it should be changed here.

Debating the merits of nullable fields or otherwise is one thing, but at the moment Backpack is forcing me to put in a work around.

[tabacitu]

@indosean - I love doing stuff in Laravel, as opposed to doing it in the Backpack code. This is a change I'm glad to have, thank you :-) This was just a workaround for Laravel switching to using mysql strict all of a sudden - surprised us at the time.

However. We have to time this right. This change can only be introduced in the next version bump, since it's breaking. In order to have it:

• we need to remove Laravel 5.5 support;
• we need to tell people to double-check that they're using ConvertEmptyStringsToNull and TrimStrings in app/Http/Kernel.php, if they've upgraded from 5.5 to 5.6 (people love to skip upgrade guide steps);

So yes, let's have this. But in CRUD 3.5. Let's leave this open until then.

Cheers!

[indosean]

@tabacitu that makes sense. Thanks.

[synapse0]

Like @indosean I don't like allowing NULL in optional string database fields; I much prefer them to default to empty strings and avoid messing with nulls in my view files, string concatenations, etc.

Also, MySql and MariaDB defaulting to stricter sql standards was very much needed IMHO.

I've been playing with backpack for a couple of days, and a workflow that works well for me is the following (stock Laravel 5.6 with the default middlewares TrimStrings and ConvertEmptyStringsToNull active, and stock MariaDb 10.2 strictmode):

• set the string fields as NOT NULL default '' in the migration
• change the
  $request->request->set($key, null);
  into
  $request->request->remove($key)
  in both lines https://github.com/Laravel-Backpack/CRUD/blob/master/src/app/Http/Controllers/Operations/Create.php#L47 and https://github.com/Laravel-Backpack/CRUD/blob/master/src/app/Http/Controllers/Operations/Update.php#L55
• no model mutators needed!

In this way the NULL fields are omitted from the INSERT (or UPDATE) statements and the default fields values can do their jobs.

On the contrary, either with or without lines 44-49 https://github.com/Laravel-Backpack/CRUD/blob/master/src/app/Http/Controllers/Operations/Create.php#L44-L49 then the INSERT statement wants to assign NULL to these string fields, when NULL is not a valid value for them, causing a database exception. The only workaround being adding a mutator function for each and every string field that is not required. In my example Client model, I had to add many of those and they don't look pretty nor DRY :-)

    public function setIndirizzoAttribute($value)
    {
        if ($value == NULL) {
            $this->attributes['indirizzo'] = "";
        }
    }

    public function setCittaAttribute($value)
    {
        if ($value == NULL) {
            $this->attributes['citta'] = "";
        }
    }

    public function setCapAttribute($value)
    {
        if ($value == NULL) {
            $this->attributes['cap'] = "";
        }
    }

    public function setProvinciaAttribute($value)
    {
        if ($value == NULL) {
            $this->attributes['provincia'] = "";
        }
    }

    public function setPivaAttribute($value)
    {
        if ($value == NULL) {
            $this->attributes['piva'] = "";
        }
    }

    public function setTelefonoAttribute($value)
    {
        if ($value == NULL) {
            $this->attributes['telefono'] = "";
        }
    }

    public function setEmailAttribute($value)
    {
        if ($value == NULL) {
            $this->attributes['email'] = "";
        }
    }

That said, I'm a backpack noob and may be missing the forest for the trees, maybe @tabacitu can weigh in and shed some light :-)

[synapse0]

Forget it.
I missed that by changing
$request->request->set($key, null);
into
$request->request->remove($key)
you cannot save an optional foreign key you may have in your form as NULL, and then you would need some workaround for that in the update() method!

The best way to go seems to remove those lines altogether!

[tabacitu]

Done, PR here - https://github.com/Laravel-Backpack/CRUD/pull/1709

Cheers! Thanks for the feedback guys!