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

Fix to allow Zend_Db_Expr as column default #9131

Merged
merged 1 commit into from
Apr 13, 2017

Conversation

scottsb
Copy link
Member

@scottsb scottsb commented Apr 4, 2017

Description

Currently if you attempt to use a Zend_Db_Expr as the default value for a column, it fails because it is accidentally converted to a string too early, which results in a malformed SQL query.

This is due to a check that attempts to avoid double quoting but isn't careful to ensure the passed value is a string, triggering an implicit type conversion.

Manual testing scenarios

Create a column in a setup script similar to this:

    /** @var \Magento\Framework\Setup\SchemaSetupInterface $setup */
    $setup->getConnection()->addColumn(
        'test_table',
        'test_column',
        [
            'type' => Table::TYPE_DATETIME,
            'default' => new \Zend_Db_Expr('CURRENT_TIMESTAMP'),
            'comment' => 'Test column comment'
        ]
    );

Before this PR that fails with the exception:

Syntax error or access violation: 1067 Invalid default value for 'test_column'

After this PR the column is created correctly.

@okorshenko okorshenko self-assigned this Apr 12, 2017
@okorshenko okorshenko added this to the April 2017 milestone Apr 12, 2017
@magento-team magento-team merged commit 2302765 into magento:develop Apr 13, 2017
magento-team pushed a commit that referenced this pull request Apr 13, 2017
 - added integration test to allow Zend_Db_Expr as column default value
@magento-team
Copy link
Contributor

@scottsb thank you for your contribution

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

Successfully merging this pull request may close these issues.

3 participants