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

DB::require_table type mismatch. #11424

Open
2 tasks done
maxime-rainville opened this issue Oct 14, 2024 · 0 comments
Open
2 tasks done

DB::require_table type mismatch. #11424

maxime-rainville opened this issue Oct 14, 2024 · 0 comments

Comments

@maxime-rainville
Copy link
Contributor

maxime-rainville commented Oct 14, 2024

Module version(s) affected

Basically every recent version.

Description

The PHPDoc for DB::require_table says it expects this method signature:

    /**
     * @param string $table The name of the table
     * @param string $fieldSchema A list of the fields to create, in the same form as DataObject::$db
     * @param string $indexSchema A list of indexes to create.  The keys of the array are the names of the index.
     * @param boolean $hasAutoIncPK A flag indicating that the primary key on this table is an autoincrement type
     * @param string $options SQL statement to append to the CREATE TABLE call.
     * @param array $extensions List of extensions
     */

This is clearly wrong. $fieldSchema, $indexSchema and $options are meant to be arrays, not strings. Trying to pass string to these parameter will lead to crashes and misery.

Looking at actual usage, it clearly expects those parameters to be array:

public function requireTable()
{
// Only build the table if we've actually got fields
$schema = static::getSchema();
$table = $schema->tableName(static::class);
$fields = $schema->databaseFields(static::class, false);
$indexes = $schema->databaseIndexes(static::class, false);
$extensions = DataObject::database_extensions(static::class);
if (empty($table)) {
throw new LogicException(
"Class " . static::class . " not loaded by manifest, or no database table configured"
);
}
if ($fields) {
$hasAutoIncPK = get_parent_class($this ?? '') === DataObject::class;
DB::require_table(
$table,
$fields,
$indexes,
$hasAutoIncPK,
$this->config()->get('create_table_options'),
$extensions
);
} else {
DB::dont_require_table($table);
}

The method itself is just an alias for DB::get_schema()->requireTable which has the correct signature.

How to reproduce

NA

Possible Solution

While changing the PHPDoc signature could arguably be called a "breaking change", there's no way the current stated signature could ever work. So it's clearly a mistake.

I think the only sensible fix here is to take the signature from DBSchemaManager::requireTable and apply it to DB::require_table.

Additional Context

No response

Validations

  • Check that there isn't already an issue that reports the same bug
  • Double check that your reproduction steps work in a fresh installation of silverstripe/installer (with any code examples you've provided)

Pull request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant