[BUG]: Model->toArray() calls get~() functions even if they are static

[ryomo]

Describe the bug

If I use Model->property, static get~() functions are not invoked. But Model->toArray() calls get~() functions even if they are static.

To Reproduce

Steps to reproduce the behavior:

test.php:

<?php

use Phalcon\Db\Adapter\Pdo\Mysql as DbAdapter;
use Phalcon\DI\FactoryDefault;
use Phalcon\Mvc\Model;

/**
 * Prepare tests
 */

$db_host = 'db';
$db_user = 'root';
$db_pass = 'pass';
$db_name = 'phalcon_test_db';
$table_name = 'test';

$sql = <<<SQL
CREATE DATABASE IF NOT EXISTS {$db_name};
USE {$db_name};
DROP TABLE IF EXISTS {$table_name};
CREATE TABLE {$table_name} (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `title` varchar(255),
  `description` varchar(255),
  PRIMARY KEY (`id`)
);
INSERT INTO {$table_name} (title, description) VALUES ("test title", "test description");
SQL;

echo shell_exec("mysql -u{$db_user} -p{$db_pass} -h {$db_host} -e '{$sql}'");

$di = new FactoryDefault();
$db_conf = [
    'host'          => $db_host,
    'username'      => $db_user,
    'password'      => $db_pass,
    'dbname'        => $db_name,
    'charset'       => 'utf8',
];
$di->set('db', function () use ($db_conf) {
    return new DbAdapter($db_conf);
}, true);

/**
 * Start tests
 */

class Test extends Model
{
    public ?int $id = null;
    public ?string $title = null;
    public ?string $description = null;

    public static function getDescription($title): string
    {
        echo 'static function called' . PHP_EOL;
        return self::findFirstByTitle($title)
            ->description;
    }
}

$test = Test::findFirst();

echo $test->description . PHP_EOL;  # static getDescription() not called
var_dump($test->toArray());  # static getDescription() called (and causing error)

terminal:

$ php test.php
test description
PHP Fatal error:  Uncaught ArgumentCountError: Too few arguments to function Test::getDescription(), 0 passed and exactly 1 expected in /var/www/html/test.php:54
Stack trace:
#0 [internal function]: Test::getDescription()
#1 /var/www/html/test.php(65): Phalcon\Mvc\Model->toArray()
#2 {main}
  thrown in /var/www/html/test.php on line 54

Expected behavior

$ php test.php
test description
array(3) {
  ["id"]=>
  int(1)
  ["title"]=>
  string(10) "test title"
  ["description"]=>
  string(16) "test description"
}

Details

• Phalcon version:
  • 5.4.0 OK
  • 5.5.0 NG
  • 5.6.1 NG
• PHP Version: 8.1.27
• Operating System: Ubuntu
• Installation type: Copied from https://github.com/phalcon/cphalcon/releases
• Zephir version (if any):
• Server: Apache
• Other related info (Database, table schema): MariaDB

[s-ohnishi]

There is no difference in the calling method from Phalcon 5.4, so I don't understand why it will work in 5.4. However, since method_exists() checks for existence regardless of whether it is a static method, implementations that rely on it may not be appropriate. However, I think it is an overkill to use the ReflectionFunction class to make judgments. So I'm reluctant to call it a bug.

On the other hand, I think it is inappropriate to define a "static" method with a name that is confusingly similar to a regular getter.

[ryomo]

@s-ohnishi

There is no difference in the calling method from Phalcon 5.4, so I don't understand why it will work in 5.4.

In 5.4.0, toArray() didn't call getters.

• v5.4.0
• v5.5.0

However, I think it is an overkill to use the ReflectionFunction class to make judgments. So I'm reluctant to call it a bug.

Agree with you. Most appropriate issue template was "bug". The others were "feature" and "security vulnerability".

On the other hand, I think it is inappropriate to define a "static" method with a name that is confusingly similar to a regular getter.

I mostly agree with that. But static getters are commonly used. ref Model->property and Model->toArray() having different behaviors cause confusion.

[s-ohnishi]

In 5.4.0, toArray() didn't call getters.

I'm sorry. It seems I was watching an unintended version.

Model->property and Model->toArray() having different behaviors cause confusion.

toArray() also has an option that does not use a getter, so if you want to collect raw data, you can specify that. The role of the getter is to retrieve the data after making it readable, formatting, and other adjustments, rather than the raw data, so I think it makes sense to retrieve it through it by default. And again, I think what's causing the confusion is that you gave the "static" method the same name as the regular getter. In the Phalcon model, getProperty() is reserved for getters, so you should think that you cannot define a method with the same name (static or not).

But static getters are commonly used.

This is a case where the conditions of use are permitted, and it does not apply in all cases.

[ryomo]

And again, I think what's causing the confusion is that you gave the "static" method the same name as the regular getter. In the Phalcon model, getProperty() is reserved for getters, so you should think that you cannot define a method with the same name (static or not).

Really? Is it reserved?

My thought was like below. Phalcon's get~ is just using PHP's __get(). And PHP's __get() "will not be triggered in static context." ref So, we wrote some static getters to retrieve data like static findFirstBy~() methods.

Anyway, there are lots of workaround, and this problem is an edge case.

[ryomo]

Here are my workarounds.

Workaround 1

Rename the static getter. Easy but it doesn't apply to my situation.

Workaround 2

Override toArray param.

public function toArray($columns = null, $useGetter = false): array
{
    return parent::toArray($columns, $useGetter);
}

Workaround 3

Override toArray.

Most of lines are from phalcon toArray and added !(new ReflectionMethod($this, $method))->isStatic().

code

```php public function toArray($columns = null, $useGetter = true): array { $metaData = $this->getModelsMetaData(); $columnMap = $metaData->getColumnMap($this); $data = []; foreach ($metaData->getAttributes($this) as $attribute) { /** * Check if the columns must be renamed */ if (is_array($columnMap)) { // Try to find case-insensitive key variant if (!isset($columnMap[$attribute]) && $this->getDI()->get('orm')->get('case_insensitive_column_map')) { $attribute = self::caseInsensitiveColumnMap( $columnMap, $attribute ); } if (!isset($columnMap[$attribute])) { if (!$this->getDI()->get('orm')->get('ignore_unknown_columns')) { throw new Exception( "Column '" . $attribute . "' doesn't make part of the column map in '" . get_class($this) . "'" ); } continue; } $attributeField = $columnMap[$attribute]; } else { $attributeField = $attribute; } if (is_array($columns)) { if (!in_array($attributeField, $columns)) { continue; } } /** * Check if there is a getter for this property */ $method = "get" . (new HelperFactory())->camelize($attributeField); /** * Do not use the getter if the method is static or the field name is `source` (getSource) */ if (true === $useGetter && "getSource" !== $method && method_exists($this, $method) && !(new ReflectionMethod($this, $method))->isStatic()) { $data[$attributeField] = $this->{$method}(); } elseif (isset($this->{$attributeField})) { $data[$attributeField] = $this->{$attributeField}; } else { $data[$attributeField] = null; } } return $data; } ```

[s-ohnishi]

So, we wrote some static getters to retrieve data like static findFirstBy~() methods.

I think the confusion is caused by giving the method the same name as a regular getter (for example, getProperty()). I think there is a problem with names that cannot be inferred, such as that it is a static method or that it returns a value after identifying it with a specified key.

From these things, I think there is a mistake in the way it is used, not a bug on Phalcon's side.

It may be necessary to refactor as the framework advances. Even if there are many changes, I would choose this option.

It is up to you to override existing methods with the default useGetter flag set to false. That kind of usage is common. I think it's unnecessary, but that's just my personal opinion.

Normally, you would think that the role of toArray() is to obtain the properties held by that instance. Therefore, it would be overkill to prepare a countermeasure in case you create a static method with a misleading name. ​