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

[BUG]: Model - Update - Postgres Bool False/True Not Working Phalcon 4 [temp fix at the end] #14722

Closed
tidytrax opened this issue Jan 17, 2020 · 22 comments

Comments

@tidytrax
Copy link
Contributor

@tidytrax tidytrax commented Jan 17, 2020

When I try to save a boolean value on postgresql, it does not save;

$model->getMessages() "return field1,field2,field3.... is required

Do not work If I use true/false

$find = Model::findFirst('id='.$id);

Return Bool fine
The fields area NOT NULL DEFAULT true/false;

["enabled"]=> | bool(true)
| ["allowed"]=> | bool(true)
["restrict"]=>|  bool(false)


Do not Work

Throw required field allowed and enabled

 $find->label = 'Test';
 $find->save();

Manually.

Do not Work If i want to change the value or auto update the column;

$find->enabled= false
$find->allowed= true.
$find->restrict = false
$find->save();

Work

$find->enabled= 0
$find->allowed= 1....
$find->restrict = 0
$find->save();

Work On Create

Return values empty on object after create.
And i don't need to set all fields values, does not set bool field as required

$find= new Model(); 
$find->enabled= false
$find->allowed= true.
$find ->save();

$find->assign(['test' => false])->update();
Works if I use 0/1
$find->assign(['test' => 0,])->update();

Config
Phalcon 4.0.2 / PHP 7.4.1 / Postgresql 12.1/Nginx 1.16.1

@ruudboon ruudboon added this to Current Sprint (Ends January 24th) in Phalcon Roadmap Jan 17, 2020
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 17, 2020

just that a notice, if ask the values again even with useDynamicUpdate(true)

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 17, 2020

Work $model = new Model();
Return values empty on object.

$model ->scene_nav = true;
$model ->save();
@tidytrax tidytrax changed the title [BUG]: Model - Postgres Bool False/True Not Workig Phalcon 4 [BUG]: Model - Update - Postgres Bool False/True Not Workig Phalcon 4 Jan 18, 2020
@ruudboon ruudboon added the 4.0.3 label Jan 22, 2020
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 22, 2020

Have some way to return all as string, something to turn around ?

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 24, 2020

One more info, the fields does not appear on getChangedFields

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 25, 2020

@tidytrax
Do you have a database schema for me?

@ruudboon ruudboon changed the title [BUG]: Model - Update - Postgres Bool False/True Not Workig Phalcon 4 [BUG]: Model - Update - Postgres Bool False/True Not Working Phalcon 4 Jan 25, 2020
@ruudboon ruudboon moved this from Current Sprint (Ends February 7th) to Working on it in Phalcon Roadmap Jan 25, 2020
@ruudboon ruudboon self-assigned this Jan 25, 2020
@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 25, 2020

I tried this on Postgres 12 but unable to reproduce this behaviour. Can you share your model?

@ruudboon ruudboon removed their assignment Jan 25, 2020
@ruudboon ruudboon moved this from Working on it to Current Sprint (Ends February 7th) in Phalcon Roadmap Jan 25, 2020
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

they came with boolean type if i do a $resultset->toArray();
They came correct, they do not appear on changed fields.

Same result as before.

$model = UserSession::findFirst(['hydration' => Resultset::HYDRATE_RECORDS]);
var_dump($model->toArray());

id: (integer) 2
url: (string) "/logout"
ip: (string) "10.0.2.2"
force_logout: (boolean) false
remember: (boolean) false

$model->id = 1;
$model->save();
var_dump($model->getMessages());

Phalcon\MessagesMessage (object) [Object ID #124][5 properties]
code:protected: (integer) 0
field:protected: (string) "force_logout"
message:protected: (string) "force_logout is required"
type:protected: (string) "PresenceOf"
metaData:protected:
(array) [0 elements]
1:
Phalcon\MessagesMessage (object) [Object ID #123][5 properties]
code:protected: (integer) 0
field:protected: (string) "remember"
message:protected: (string) "remember is required"
type:protected: (string) "PresenceOf"
metaData:protected:
(array) [0 elements]

I didn't updated this 2 fields, they are filled, but ask again, and if i try

$model->force_logout = false/true: //**fail**
$model->force_logout = 0/1 //**works**.

This is fine.
var_dump($model->getChangedFields());exit;

(array) [2 elements]
0: (string) "session_id"
1: (string) "updated"

So always when i want to update a record i have to pass all boolean columns as int again, even if they are already filledup, phalcon does not accept to save bool,

How i have to do to update a record on database or save.

$model = UserSession::findFirst(['hydration' => Resultset::HYDRATE_RECORDS]);
$model->id = 1;
$model->force_logout = ( int) $model->force_logout(already has a value);
$model->remember = (int) $model->remember(already has a value);
$model->save();
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

I tried this on Postgres 12 but unable to reproduce this behaviour. Can you share your model?

CREATE TABLE "auth"."user" (
  "id" serial8 NOT NULL,
  "url" text COLLATE "pg_catalog"."default" NOT NULL,
  "ip" varchar(50) COLLATE "pg_catalog"."default" NOT NULL,
  "force_logout" bool NOT NULL DEFAULT false,
  "remember" bool NOT NULL DEFAULT false,
  "status" numeric(2) NOT NULL DEFAULT 1,
  "updated" timestamp(6) NOT NULL,
  "created" timestamp(6) NOT NULL
)

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

Configuration of model, i already try all options mix.

Model::setup([
           'forceCasting' => true,
           'castOnHydrate' => true
       ]);

$this->useDynamicUpdate(true);

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

On phalcon 3.4.4 works fine, i'm trying to migrate an aplication to the Phalcon 4, works fine all, less postges bool

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 25, 2020

The hydrate stuff is not for this issue.

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

I know sorry only saw the wrong tab after reply

@ruudboon ruudboon added 4.0.4 and removed 4.0.3 labels Jan 25, 2020
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

You was able to reproduce the behavior?

It's that a way to return all values as string?

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

$this->modelsMetadata->getDataTypesNumeric($model);

id: (boolean) true
force_logout: (boolean) true
remember: (boolean) true
status: (boolean) true

$this->modelsMetadata->getDataTypes($model);

id: (integer) 14
url: (integer) 6
ip: (integer) 2
force_logout: (integer) 8
remember: (integer) 8
status: (integer) 3
created: (integer) 17

And on cache metadata if enabled,
they came as string on default value 'false'/'true' instead of false/true

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

Could be on Phalcon\MVC\Model.zep

Line 4594, value is returned as string on metadata 'false' !== false
value !== defaultValues[field]
Line 4599 since the field is numeric but the value is not

if !is_numeric(value) {
    let isNull = true;
}
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 25, 2020

@ruudboon Worked if i go to meta-app_models_model.php and removed from int type

array (
'id' => true,
'force_logout' => true, //removed
'remember' => true, //removed
'status' => true,
),

BECAME

array (
'id' => true,
'status' => true,
),

Works

$find = Model::findFirst();
$find->save();

//Works

$find->remember = true;
$find->save();

If i disable models metadata does not work, so the problem isn't on cache, is on the field Type = numeric

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 26, 2020

Fix for who has the same problem, must use cache on model metadata.
just works with use annotation strategy with no annotation on the model.
Stranger, but fix.
And is not more on numeric, but does not bring anymore the default value all = NULL, so to create a new row, have to fill all. the problem is really on set as numericField,

$di->setShared('modelsMetadata', function () {
    $options = [
        'metaDataDir' => PATH_CACHE_METADATA . DS
    ];
    $metadata=new \Phalcon\Mvc\Model\Metadata\Stream($options);
    $metadata->setStrategy(new Annotations()); //set here
    return $metadata;
});

Using this, does not bring on metadata as numeric field.

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Jan 26, 2020

@tidytrax Thnx! This is some great research and really helpful.

@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 26, 2020

Thank you @ruudboon i'm using about 7 years phalcon, i will as help as i can.
And this bug, is really a killing.

But i found the problem, and fix here for me, and is describe below how to fix for who use postgresql

Adapter\Pdo\Postgresql.zep
Line 207 bool : The Problem isNumeric = true because boolean is treat like tinyint
This should just for mysql/mariadb... adapter, postgresql has boolean, you can use tinyint also to do a "boolean" field, but on postgresql column type boolean should be boolean not numeric.

 /**
* BOOL
*/
case memstr(columnType, "boolean"):
 /**
* tinyint(1) is boolean
*/
 let definition["type"] = Column::TYPE_BOOLEAN,
definition["isNumeric"] = true, //HERE IS THE PROBLEM
definition["bindType"] = Column::BIND_PARAM_BOOL;
break;

Tinyint is already treated on Line 248

/**
 * TINYINT
 */
case memstr(columnType, "tinyint"):
/**
 * Smallint/Bigint/Integers/Int are int
 */
let definition["type"] = Column::TYPE_TINYINTEGER,
definition["isNumeric"] = true,
definition["bindType"] = Column::BIND_PARAM_INT;

break;

FIX

So the fix, extends PDO, and change describeColumns.

Also does not annoying asking all fields again when you update, if you want to gain performance, you can rewrite all function or use parent as source and change.

Do not set column as numeric, or will come back to the same problem

use Phalcon\Db\Column;
class Postgresql extends \Phalcon\Db\Adapter\Pdo\Postgresql
{

    public function describeColumns(string $table, string $schema = null): array
    {
        $columns = parent::describeColumns($table, $schema);

        foreach ($columns as $key => $column) {
            if ($column->isNumeric() and $column->getType() === Column::TYPE_BOOLEAN) {
                $columns[$key] = new Column($column->getName(), [
                    'bindType' => Column::BIND_PARAM_BOOL,
                    'after' => $column->getAfterPosition(),
                    'notNull' => $column->isNotNull(),
                    'type' => Column::TYPE_BOOLEAN,
                    'default' => ($column->getDefault() === 'false') ? false : true //Optional
                ]);
            }
        }
        return $columns;
    }
}

Just use it only phalcon team does not fix, after they did come back to original

Service should look like something...
Metadata is not needed;

$di->setShared('modelsMetadata', function () {
    $options = [
        'metaDataDir' => PATH_CACHE_METADATA . DS
    ];
    $metadata=new \Phalcon\Mvc\Model\Metadata\Stream($options);
    return $metadata;
});

$di->setShared('db', function () {
    $config = $this->getConfig();
    $dbConfig = $config->database->toArray();
    $adapter = $dbConfig['adapter'];
    unset($dbConfig['adapter']);

    $class = '\System\Postgres';
    $connection = new $class($dbConfig);
    return $connection;
});

Before F5 and reload to test, clear metadata folder(if use it).

And now you can use again true/false or update a row without passing all values(boolean) again as int.

$model->field = true;
$model->field2 = false;
$model->save();
@tidytrax tidytrax changed the title [BUG]: Model - Update - Postgres Bool False/True Not Working Phalcon 4 [BUG]: Model - Update - Postgres Bool False/True Not Working Phalcon 4 [temp fix at the end] Jan 26, 2020
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 26, 2020

Why the problem does not occur on phalcon 3.4 @ruudboon .

No numeric type is set for smallint(tinyint) or boolean
postgresql.zep#L209

if memstr(columnType, "smallint(1)") {
    /**
     * Smallint(1) is boolean
     */
    let definition["type"] = Column::TYPE_BOOLEAN,
    definition["bindType"] = Column::BIND_PARAM_BOOL;
} elseif memstr(columnType, "bool") {
    /**
     * Boolean
     */
    let definition["type"] = Column::TYPE_BOOLEAN,
    definition["size"] = 0,
    definition["bindType"] = Column::BIND_PARAM_BOOL;
tidytrax added a commit to tidytrax/cphalcon that referenced this issue Jan 26, 2020
Remove is numeric for boolean type
Related to this bug.
phalcon#14722
niden added a commit that referenced this issue Jan 26, 2020
Remove is numeric for boolean type
Related to this bug.
#14722
@niden niden moved this from Current Sprint (Ends February 7th) to Working on it in Phalcon Roadmap Jan 26, 2020
@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Jan 26, 2020

Resolved in #14781

Thank you again @tidytrax for the report, investigation and fix

@niden niden closed this Jan 26, 2020
Phalcon Roadmap automation moved this from Working on it to Implemented Jan 26, 2020
@tidytrax

This comment has been minimized.

Copy link
Contributor Author

@tidytrax tidytrax commented Jan 26, 2020

i will keeping test v4.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Phalcon Roadmap
  
Released
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.