Skip to content
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

[NFR] \Phalcon\Mvc\Model doesn't save an empty string #440

Closed
iforp opened this issue Feb 21, 2013 · 28 comments
Closed

[NFR] \Phalcon\Mvc\Model doesn't save an empty string #440

iforp opened this issue Feb 21, 2013 · 28 comments
Labels
not a bug Reported issue is not a bug

Comments

@iforp
Copy link
Contributor

iforp commented Feb 21, 2013

$note = new Notes();
$note->note = '';
$note->save();
var_dump($note->getMessages());
array (size=1)
    0 => object(Phalcon\Mvc\Model\Message)[49]
        protected '_type' => string 'PresenceOf' (length=10)
        protected '_message' => string 'note is required' (length=16)
        protected '_field' => string 'note' (length=4)
        protected '_model' => null

But empty string is valid value

Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@phalcon
Copy link
Collaborator

phalcon commented Feb 21, 2013

Most of functions to filter/sanitize data return empty strings instead null values:

var_dump(strip_tags('<h1></h1>')); // string(0) ""
var_dump(strip_tags(null)); // string(0) ""
var_dump(trim(null)); // string(0) ""
var_dump(htmlspecialchars(null)); // string(0) ""
var_dump(filter_var(null, FILTER_SANITIZE_EMAIL)); // string(0) ""
var_dump(filter_var(null, FILTER_SANITIZE_URL)); // string(0) ""
var_dump(filter_var(null, FILTER_SANITIZE_MAGICQUOTES)); // string(0) ""

If Phalcon does not handle empty string the same as null values, the application could lead allow unexpected data as valid

@phalcon
Copy link
Collaborator

phalcon commented Feb 21, 2013

If you want you could globally remove the automatic not null validation (this applies for all models):

\Phalcon\Mvc\Model::setup(array(    
    'notNullValidations' => false
));

And implement your own validation in every model:

use Phalcon\Mvc\Model\Message;

class Notes extends Phalcon\Mvc\Model
{
    public function validation()
    {
        $notNullAttributes = $this->getModelsMetaData()->getNotNullAttributes($this);
        foreach ($notNullAttributes as $field) {
            if (!isset($this->$field) || $this->$field === null) {
                $this->appendMessage(throw new Message($field . ' is required'));
                return false;
            }
        }
        return true;
    }
}

@nikolasha
Copy link

This is a problem. Sometimes really comfortable prohibit NULL value, but allow an empty string. I think, by default, the Phalcon must check the exactly NULL value. If an empty string is also unacceptable, then the developer must explicitly say so. At least it is more obvious and consistent with the logic of the database.

P.S.
!isset($this->$field) || $this->$field === null this is equivalent to just !isset($this->$field) :)

@iforp
Copy link
Contributor Author

iforp commented Feb 21, 2013

@nikolasha is absolutely right.

var_dump(strip_tags('<h1></h1>')); // string(0) ""
and your other examples is expected behavior, but error on saving valid empty string is unobvious.

If an empty string is also unacceptable, then the developer must explicitly say so.

Implementation of own validation is voodoo magic in this case.

@phalcon
Copy link
Collaborator

phalcon commented Feb 21, 2013

This is not a problem, most of the cases in web applications we're receiving data from forms and external data:

$post->name = strip_tags($_POST['name']); // returns empty string
$post->content = strip_tags($_POST['content']); // returns empty string
$post->save(); //this will save with two fields empty, is that acceptable? is that the common case?

$user->name = strip_tags($_POST['name']); // returns empty string
$user->email = filter_var($_POST['email'], FILTER_SANITIZE_EMAIL); // returns empty string
$user->save(); //this will save with two fields empty, is that acceptable? is that the common case?

in a REST request:

http://localhost/api/products?name=
$products->name = trim($_GET['name']);
$products->save(); //this will save with and empty name, is that acceptable? is that the common case?

Do you want to implement string length validators for every char/varchar field in your model?
Is this something that would make the life of a developer easier, have to validate if every char/varchar field is null or empty string to satisfy less than 20% of cases?
Do you want to convert every empty string to null to make the "null validator" work?

If that is what you want, as I said you could disable the automatic validation and implement one that satisfy the level of explicitness you need. But this is something that is not going to be changed in Phalcon because it makes the programming more hard in most cases.

@iforp
Copy link
Contributor Author

iforp commented Feb 22, 2013

In my application I want to control data types. Null and empty string is not the same type.

For example in a REST request:

http://localhost/api/products?name=
var_dump($_GET['name']); // string '' (length=0)

vs

http://localhost/api/products
var_dump($_GET['name']); // null

and what about this case

http://localhost/api/products?name[]=
$products->name = trim($_GET['name']);
$products->save(); // this will save with name "Array", is that acceptable? is that the common case?

So your examples are contrived.
As a developer I would expect this behavior and would check data, not just trim and strip_tags.

Why I can not save data in the exact format in which I need it?
Is your recipe makes the programming more easy just to save empty string?

In the last resort it should be something like this:

class Notes extends \Phalcon\Mvc\Model
{
    public $id;
    public $note;
    public function initialize()
    {
        $this->allowEmptyString(array('note'));
        // or deny, if it allowed by default
        $this->denyEmptyString($fieldsList);
    }
}

@phalcon
Copy link
Collaborator

phalcon commented Feb 22, 2013

Data is stored as they come in the same format, the framework doesn't make any automatic transformation.

Data is usually filtered/sanitized before be stored in the database. As you can see most validations/filters/sanitizers return empty strings. If an application doesn't require filter/sanitize/validate data before store them that is simply wonderful! Users are doing a perfect job, and hackers are not allowed :)

If you're receiving an array you must be aware of that. Applying a string filter over an array doesn't make sense. I don't know if it's necessary point out that.

I understand this behavior is not perfect, but it has a good intention and removing it is a greater problem than an improvement. Why not just disable the automatic validation and implement a validation system that fits your needs?

More options:

1. Re-implement the automatic validation system
You could add a base model to your system, this gives you the freedom to implement any validation system, you can even translate the messages produced instead of using the hardcoded ones:

use Phalcon\Mvc\Model\Message;

class BaseModel extends Phalcon\Mvc\Model
{
    public function validation()
    {
        $notNullAttributes = $this->getModelsMetaData()->getNotNullAttributes($this);
        foreach ($notNullAttributes as $field) {
            if (!isset($this->$field) || $this->$field === null) {
                $this->appendMessage(throw new Message($field . ' is required'));
                return false;
            }
        }
        return true;
    }    
}

Use this class as base in your models:

class Notes extends BaseModel 
{
}

2. Convert empty strings into raw values
Alternatively, you can convert the empty strings to raw values skipping the automatic "not null" validator:

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

3. Change the field to allow null and move this validation to the application layer
If it's acceptable, marking the not null field in the table to allows nulls, implementing this validation in your application:

use Phalcon\Mvc\Model\Message;

class Notes extends Phalcon\Mvc\Model
{
    public function validation()
    {
        if ($this->note === null) {
            $this->appendMessage(throw new Message('Note cannot be null'));
            return false;
        }
    }
}

@iforp
Copy link
Contributor Author

iforp commented Feb 22, 2013

Thanks for your answers. In these ways I can solve my current problem.
But the real problem is the lack of flexibility.

For example look at the Yii solution

public function rules()
{
    return array(
        array('name', 'required'),
        array('name', 'length', 'min'=>3, 'max'=>12),
        array('note', 'type', 'type' => 'string'),
    );
}

@iforp
Copy link
Contributor Author

iforp commented Feb 22, 2013

2 . Convert empty strings into raw values

$note = new Notes();
$note->note = new Phalcon\Db\RawValue('');
$note->save();
// PDOException: Syntax error or access violation

cause it generate SQL query like this

INSERT INTO `notes` (`note`, `id`) VALUES (, null)

The correct usage is

$note->note = new Phalcon\Db\RawValue('""');

@nikolasha
Copy link

@phalcon, this is understandable and commendable that you are trying to make life easier for developers. But here it is necessary to know the measure. In practice, such a decision would only reduce flexibility of the framework, and non-obvious behavior. And in return, will not bring a noticeable benefit. Let me explain why. Typically, the string data required, require a more rigorous test than the test for an empty string. You want to check that the name contains valid characters, email address matches the format, password is long enough, etc. In these cases, an empty string problem is solved by itself. If the data does not need to be checked, it did not really matter that tells the user - a empty string or a stupid value.

Incidentally, the same problem will be with a date which does not allow NULL value and is set in the database automatically. In general, there is also the problem of NOT NULL fields that have default values ​​in the database.

A few more thoughts:

  1. Disable check for NULL value for all fields at once is a bad decision. Setting should be more flexible.
  2. Even if you leave it as is, then you must rename notNullValidations to notEmptyValidations :)

@nikolasha
Copy link

Perhaps it would be a good solution:

$this->allowEmptyString(array('note'));

@luiz-brandao
Copy link

@phalcon I also agree that an empty string IS a valid value and as such should be allowed

@hugoduraes
Copy link

+1

@sergiobc
Copy link

Confirm the issue.
When I faced the error message, I first thought it was a bug. Then reading this post I was surprised to find out this is a feature.
Agree with the previous posters, that empty string is a special case for validation. And default constraints should match the underlying database rules, since the framework uses DB field metadata to validate the values.

@nexik
Copy link

nexik commented Sep 11, 2013

👍

If I want field to be required I would rather write something like this:

/**
 * @Column(type="string", length=80, nullable=false, required=true)
 */

@rebelmiles
Copy link

I think this is a problem too. I have it it several times and had to work around it.

The latest concerns a CHAR field which is a primary key but can contain an empty string.

I should not have to restructure a database to "fit in" with how phalcon thinks things "should be done".

@DaanBiesterbos
Copy link

@rebelmiles You don't have to. You can assign empty values like this: $robot->foo = new \Phalcon\Db\RawValue('""');
But I do agree with you. The solution is not pretty. I would like to see a better solution.

@rebelmiles
Copy link

@DaanBiesterbos I tried that solution. I think "not pretty" is a gross understatement.

@jimmycdinata
Copy link

I agreed with @nikolasha . It will be great if Phalcon allowed empty string as valid value.

+1

@informagician
Copy link

does anyone have any updates/progress on this issue?

@vlad-khramov
Copy link
Contributor

@phalcon, you comment #440 (comment) helps, but i think it hacks.

In this case phalcon orm trying to do validations of business logic, but it not problem of orm.

All cases in #440 (comment) are acceptable. It task of business logic to allow or not allow empty name, email.

Another example:
model User (name, comment not null, default ''). I save user (name='robot', comment=''). Than

$user = User::findFirst("name='robot'");
$user->name='robot2';
$user->save();//!!!!!!

Programmer must write validations to not allow empty values in some fields, not to allow empty values for all fields.

@Nevario
Copy link

Nevario commented Aug 19, 2014

+1 I would also like to see a cleaner way to allow an empty string.

@virgofx
Copy link
Contributor

virgofx commented Aug 21, 2014

+1 Empty strings should be allowed

@phalcon
Copy link
Collaborator

phalcon commented Aug 21, 2014

Empty strings are allowed, who said they aren't allowed?

@vlad-khramov
Copy link
Contributor

@phalcon, example:

class Robot extends \Phalcon\Mvc\Model {
    public $name;//not null, default ''
    public $comment;//not null, default ''
}
$r = Robot::findFirst(1); //(name='robot', comment='')
$r->name = 'robot2';
$r->save();
var_dump($r->getMessages());

Result:

array (size=1)
  0 =>
    object(Phalcon\Mvc\Model\Message)[37]
      protected '_type' => string 'PresenceOf' (length=10)
      protected '_message' => string 'comment is required' (length=19)
      protected '_field' => string 'comment' (length=7)
      protected '_model' => null
      protected '_code' => int 0

@phalcon
Copy link
Collaborator

phalcon commented Aug 22, 2014

@quantum13

use Phalcon\Mvc\Model;

class Robot extends Model
{
    public $name;//not null, default ''
    public $comment;//not null, default ''
}

Model::setup(['notNullValidations' => false]);

$r = Robot::findFirst(1); //(name='robot', comment='')
$r->name = 'robot2';
$r->save();
var_dump($r->getMessages());

Result:

array(0) {
}

@phalcon phalcon locked and limited conversation to collaborators Aug 23, 2014
@andresgutierrez
Copy link
Sponsor Contributor

This is implemented in 2.0.3

<?php

namespace Some;

use Phalcon\DI,
    Phalcon\Db\Column,
    Phalcon\Mvc\Model,
    Phalcon\Events\Manager as EventsManager,
    Phalcon\Db\Adapter\Pdo\MySQL as Connection,
    Phalcon\Mvc\Model\Manager as ModelsManager,
    Phalcon\Mvc\Model\Metadata\Memory as ModelsMetaData;

$eventsManager = new EventsManager();

$di = new DI();

$connection = new Connection(array(
    "host"     => "localhost",
    "username" => "root",
    "password" => "",
    "dbname"   => "phalcon_test",
));

$connection->setEventsManager($eventsManager);

$eventsManager->attach('db',
    function ($event, $connection) {
        switch ($event->getType()) {
            case 'beforeQuery':
                echo $connection->getSqlStatement(), " [", join(', ', $connection->getSqlVariables()), "]\n";  
                break;
        }
    }
);

$modelsManager = new ModelsManager();
$modelsManager->setDi($di);

$metaData = new ModelsMetadata();

$di['db'] = $connection;
$di['modelsManager'] = $modelsManager;
$di['modelsMetadata'] = $metaData;

class Robots extends Model
{
    public function initialize()
    {
        $this->allowEmptyStringValues(['name', 'text', 'datetime']);
    }
}

$robot = new Robots();
$robot->name = "";
$robot->text = "";
$robot->datetime = "";
$robot->save();
print_r($robot->getMessages());

@cottton
Copy link
Contributor

cottton commented Jul 12, 2017

sarcasm : thanks that i had to search to find out what happen with the empty strings /sarcasm

Bad solution imo. And as i read im not the only one.

2017 - Extra search|code|work|time to fix an issue with a model that just should insert an empty string.

Not so well done.

Not only that you force ppl to search for a solution. You force ppl to add code to fix a problem that actually should not be problem.

Adding code may bring more problems

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not a bug Reported issue is not a bug
Projects
None yet
Development

No branches or pull requests