[BUG]: VARCHAR(255) NOT NULL does not accept an empty string

[zacek]

Questions? Forum: https://phalcon.io/forum or Discord: https://phalcon.io/discord

Describe the bug
Model with column declared as varchar(255) NOT NULL does not accept an empty string. It does not distinguish NULL value and the empty string.

To Reproduce

1. create a DB table

CREATE TABLE `testing` (
  `id` int(10) unsigned NOT NULL AUTO_INCREMENT,
  `address` varchar(255) NOT NULL,
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

2. create a corresponding model

<?php

class Testing extends \Phalcon\Mvc\Model
{
}

3. save a new entry in the DB

$testing = new Testing();
$testing->address = '';
$result = $testing->create();

var_export($testing->getMessages());

The result is

array (
  0 => 
  Phalcon\Messages\Message::__set_state(array(
     'code' => 0,
     'field' => 'address',
     'message' => 'address is required',
     'type' => 'PresenceOf',
     ...
  )),
)

Expected behavior
Empty string should be accepted, NULL not.

Details

Version => 5.2.1
Build Date => Mar 1 2023 13:57:18
Powered by Zephir => Version 0.17.0-$Id$

• Phalcon version: 5.2.1
• PHP Version: 8.1.17
• Operating System: Rocky Linux release 8.7 (Green Obsidian)
• Installation type: OS distribution: php-phalcon5.x86_64 5.2.1-1.el8.remi.8.1
• Zephir version (if any): 0.17.0-$Id$
• Server: irrelevant, CLI
• Other related info (Database, table schema): MySQL 5.7

Additional context
Running in docker.

[s-ohnishi]

There is a way to avoid this validation in the manual, but I don't think you should use it because the impact is too big.
\Phalcon\Mvc\Model::setup(array('notNullValidations' => false));

How is this a realistic method?

class Testing extends \Phalcon\Mvc\Model
{
     public function beforeValidation()
     {
         if ($this->address=== '') {
             $this->address= new \Phalcon\Db\RawValue('""');
         }
     }
}

or

class Testing extends \Phalcon\Mvc\Model
{
     public function initialize()
     {
         $this->modelsMetaData->setEmptyStringAttributes($this, ['address'=>true]);  // Correct mistakes made when copying and pasting
     }
}

I asked a similar question before, but no one seemed interested and I didn't get any replies.
I looked into it again, but I think there's something wrong with the processing around here.

Null and "" should have different meanings, but both are treated as isNull = true and are required.
(In the case of a new record, this can be avoided if it is an ID field or a default value is set. However, in the case of an existing record, "" may also be intentionally inserted.)
Also, I think it is strange behavior that if you set NOT NULL DEFAULT "" when defining a table, you cannot insert "" which is the same as the default value.
Furthermore, I think it is insufficient checking that isNull = true is set regardless of whether the default value is set or not, as shown below.

if isset emptyStringValues[field] {
     if value === null {
         let isNull = true;
     }
}

I think that the current problem can be avoided with these, but I am very doubtful whether this is a good implementation of Phalcon.

[zacek]

@s-ohnishi You are right, disabling the null-validation is not a good option.

I also agree that the line you reference to really is strange:

if isset emptyStringValues[field] {
    if value === null {
        let isNull = true;
    }
} else {
    if value === null || (value === "" && (!isset defaultValues[field] || value !== defaultValues[field])) {
        let isNull = true;
    }
}

It looks like some trick to automatically treat empty values from the forms when you get value= in the request which is translated to an empty string and should be saved to DB either as an empty string or null based on the column definition or annotations. IMHO, these tricks should be removed and the value is null if and only if value === null regardless of the circumstances. If you send the value in the request value= the value is an empty string. If you do not send it at all it is null.

In this case, a clean solution might be listing any column defined as VARCHAR in emptyStringValues - it is perfectly correct to save an empty string to it in all 4 scenarios:

• VARCHAR(255) NOT NULL
• VARCHAR(255) NOT NULL DEFAULT ''
• VARCHAR(255) NULL
• VARCHAR(255) NULL DEFAULT NULL
  It should be set by the introspection.

Additionally, I do not like the isset usage further in the code, see this line:

if isset defaultValues[field] {
    continue;
}

This evaluates to true only if the default value is set and is not null. I.e., if the default value is null it evaluates to false. There is a better method array_key_exists which treats the null default value correctly.

[s-ohnishi]

@zacek
I'm very happy to have someone tell me that my way of thinking wasn't wrong.

I'm also glad to see that a correction is being made.
Adjusting with beforeValidation() or registering with setEmptyStringAttributes() is a workaround for the "current situation" and is not the state it should be in.

[rudiservo]

Just checked it, introspections does not fill emptyStringValues, annotation strategy does.

My issue is with the emptyStringValues vs defaultValues, trying to figure out the why.

We do have a protected function in the model allowEmptyStringValues(), propably is supposed to be used in the initialize, but will not be cached.
Actually this was added in 3.3.0

@zacek can you check if allowEmptyStringValues() works has expected while I make some checks and tests.

[rudiservo]

OK so the default behaviour sinde 3.0 of the ORM is to assume emptyStrings as NULL, unless you set it to allow.
IMO if someone does not want a string to be empty they should use validation, in fact they will have to do it at least in the form validation, the ORM is quite complex already.

If this is a treated like a bug then fix is rather simple.
If this is the intended default behaviour, changing it could be a breaking change thus requires a new major version and probably that will not happen with cphalcon.

[s-ohnishi]

If this comment "Get string attributes that allow empty strings as defaults" is correct, then if you set an empty string in a field, the default value will be set instead, right?
But currently, isNull = treu and all existing records throw a "required" exception.

I think there are two problems.
First, when isset emptyStringValues[field] is true, null and empty string should be treated the same, but only value === null is checked.
Second, existing records are not checked for default values.

Inserting an empty string into a field that specifies NOT NULL is not a problem in the first place, so I think there was no need to verify it.
(Except when you want to treat the empty string above as null)

value === "" && (!isset defaultValues[field] || value !== defaultValues[field]) is false if both the default value and the specified value are empty strings.
I don't know what kind of situation was expected, but I think inserting an empty string is not a problem, so it is unnecessary.

Perhaps this is a misunderstanding caused by the name "emptystring".

[rudiservo]

@s-ohnishi the code is a bit confusing, I can try and simplify it.
Existing records are checked, ate least they seem to be.

The issue is that this is a change in behaviour, while I disagree with it, it will break some applications and we do not want that, v6 will have this sorted out.

Can you confirm that allowEmptyStringValues in initialize works and is somewhat a solution?

[s-ohnishi]

@rudiservo
I can't say for certain because I don't fully understand Zephir, but I think it will work if you specify it with setEmptyStringAttributes() and pass anything other than null.
So I think this is a temporary workaround.

However, if a default value is set for an existing record when the table is defined, that should be used by passing null, right?
In the current code, it appears that existing records are not checked for the presence of a default value.

Existing records are checked, ate least they seem to be.

Where is it checked?
Model.zep
I don't think it's checked here at least.

[rudiservo]

@s-ohnishi Don't worry about zephir, if you could test the setEmptyStringAttributes and check if it works has you expect would be great.

For your second question.
If there is a default value it will jump to the next iteration if it's a new record, meaning that if it does not exists in the database.

                                /**
                                 * The field have default value can be null
                                 */
                                if isset defaultValues[field] {
                                    continue;
                                }

So if it has default values, the continue will terminate the current key loop and move on to the next key.

If the record exists, this come in effect.

if value === null || (value === "" && (!isset defaultValues[field] || value !== defaultValues[field])) {
    let isNull = true;
}

so if I am reading this correctly, if the value is null on a non numeric field and the record exists, it is a 'PresenceOf' error.

Can you confirm this in your testing?

If this is unwanted behaviour or a bug, what would the expected behaviour be?

[s-ohnishi]

if the value is null on a non numeric field and the record exists, it is a 'PresenceOf' error.

Sorry, I was wrong.
My environment (the one I test most often) had a loose setting.
If I tried to UPDATE a column with NOT NULL DEFAULT "" with NULL, it would only issue a warning and update with the default value.
So it wasn't in strict mode.

In strict mode, you will get ERROR 1048 (23000) instead of a warning, so the current usage is correct, with the exception of setting NULL for an existing record.
I may have overlooked the fact that "strict mode" is the premise, but if it's not mentioned anywhere, it might be a good idea to specify it somewhere.

However, I don't think this part is very accurate.

value === "" && (!isset defaultValues[field] || value !== defaultValues[field])

If you set an empty string in a column that is DEFAULT "" for an existing record, isNull = true will result.
Even if it is currently possible to avoid this using setEmptyStringAttributes(), I think it is something that should be automatically avoided.

It seems like they are afraid that the behavior will change, but I think it would be a good idea to undo things that were originally possible but became exceptions.
Users who currently want to avoid this will do so by using something like setEmptyStringAttributes(), which may not be the intended use, so there will be no new problems, right?
After the source is modified, wouldn't it be possible for users who don't know the workaround to use it without any problems?

[rudiservo]

@s-ohnishi thanks for the feedback.

Making these kind of changes is something you have to make argument for, the against argument is it might break someone's application, the issue is not the work around it's the default behaviour changes.

You have to convince @niden , I would push for more changes in phalcon v5 but we are trying to push v6, so time is limited.

[niden]

We will need to leave this as is. Any change we make in the logic has the potential of breaking existing applications and that is something we do not want. Whatever change we make has to be communicated well in advance with the community so as upgrades can be a bit less painful.

In v6 we can change the behavior to become more clearer and easier to handle but not v5

[s-ohnishi]

This will change the logic, but the effect is that setting an empty string to a column that is NOT NULL DEFAULT="" will not result in an exception.
Leaving this as is is obviously not correct in terms of SQL, but if you can't get approval, you should clearly state it in the manual and call for attention.