cobwebch / external_import

Other
20 stars 16 forks source link

Allow nullable field to be saved in database #302

Closed dogawaf closed 1 year ago

dogawaf commented 1 year ago

Hello

Given the following field:

// TCA column definition
        'price_member' => [
            'label' => $ll . 'tx_site_domain_model_publication.price_member',
            'config' => [
                'type' => 'input',
                'eval' => 'double2,null',
            ],
        ],
-- SQL field definition
    price_member double precision NULL,

And this transformation:

        'transformations' => [
            10 => [
                'isEmpty' => [
                    'default' => null,
                ],
            ],
        ],

If the incoming value is null, the null value will never be set in the database.

The reason is in https://github.com/cobwebch/external_import/blob/master/Classes/Step/StoreDataStep.php#L667 with the usage of isset. I suggest to use array_key_exists to allow null value to be stored in database.

dogawaf commented 1 year ago

I would love to make a pull request, but I can't manage to start the functional tests...

fsuter commented 1 year ago

I totally get your point, but I'm embarrassed, because this is actually a breaking change. With the current situation, people don't need to worry about having sane default values everywhere: when a value is empty, it is simply not imported. I agree that this is not an ideal behaviour. It has caused a number of issues in the past, in particular with updated fields not being nulled/emptied when there was no value to import anymore.

However the change means that many existing import configurations could results in errors, with DB fields not ready to accept NULL.

One possibility I see is to check if the fields are nullable according to the TCA. This would mean an eval property including null (as in your case) or, starting with TYPO3 12, checking for the nullable property.

What do you think?

dogawaf commented 1 year ago

Hello

Thanks for getting back on this subject.

Reading TCA configuration to check if the field is nullable is a good idea. This way the change will be backward compatible: if the field is nullable, and if isEmpty.default is null, null will be stored in the database.

fsuter commented 1 year ago

Note that I fixed the bug only for "main" columns not for children columns, as this would introduce a whole level of complexity. So I'll avoid it until someone needs it...