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

Update hook 1100 (7100) fails. #6

Open
klonos opened this issue Feb 12, 2017 · 3 comments
Open

Update hook 1100 (7100) fails. #6

klonos opened this issue Feb 12, 2017 · 3 comments
Assignees
Labels

Comments

@klonos
Copy link
Member

klonos commented Feb 12, 2017

This is the respective issue for Update hook 7100 fails [#2846887] in the d.org queue that was fixed with http://cgit.drupalcode.org/oauth/commit/?id=e1558f5

Running the 7100 update hook results in an error:

Update #7100
Failed: PDOException: SQLSTATE[22004]: Null value not allowed: 1138 Invalid use of NULL value: ALTER TABLE {oauth_common_token} CHANGE callback_url callback_url VARCHAR(255) NOT NULL COMMENT 'Callback url.'; Array ( ) in db_add_field() (line 2900 of /mnt/www/html/acquiacomdev/docroot/includes/database/database.inc).

The solution is to add 'default' => '', to the update hook code.

Not sure, but I believe it applies to our codebase too.

@klonos klonos added the bug label Feb 12, 2017
@klonos klonos self-assigned this Feb 12, 2017
klonos added a commit that referenced this issue Feb 12, 2017
@klonos
Copy link
Member Author

klonos commented Feb 12, 2017

PR up for review @Graham-72

@klonos
Copy link
Member Author

klonos commented Feb 12, 2017

The PR was a straight adaptation from the d.org patch, but there seems to be something wrong with it: https://www.drupal.org/node/2846887#comment-11931133

...pending reply from the d.org maintainers.

As for your concerns:

...I am not sure that your PR will always produce the right result. Suppose someone is migrating from Drupal and has already applied the new update 7101. They will have the new Callback URL field correctly added and so we should not delete it in Backdrop's update 1101. So we should only delete it if 7100 has been applied but not 7101 i.e. it is 'wrongly created'. What do you think?

...(once we get a reply from the d.org maintainers) I think that we should be checking whether the default value exists for the callback_url field (in both tables) before deleting the field and adding the field with the updated schema. Something like this perhaps:

function oauth_common_update_1101() {
  $tables = array('oauth_common_provider_consumer', 'oauth_common_token');
  $unprocessed_schema = backdrop_get_schema_unprocessed('oauth_common');
  foreach ($tables as $table) {
    if (db_field_exists($table, 'callback_url')) {
      if (!$unprocessed_schema[$table]['fields']['callback_url']['default']) {
        db_drop_field($table, 'callback_url');
      }
    }
    if (!db_field_exists($table, 'callback_url')) {
      $spec = array(
        'description' => 'Callback URL.',
        'type' => 'varchar',
        'length' => 255,
        'not null' => TRUE,
        'default'     => '',
      );
      db_add_field($table, 'callback_url', $spec);
    }
  }
}

@Graham-72
Copy link
Member

@klonos I think your suggestion above for update 1101 is a good one and I would like to include it in the BD version of the module to make sure that the schema for both tables are correct and updated accordingly. I have now made a commit for issue #2 that gets the code fully updated and would like to include your update 1101 to make sure that any existing installation gets correctly updated.

I note that in D7 there is a more recent issue about this at https://www.drupal.org/node/2852011 and this would seem to support your suggestion there.

I have tried to follow through the D7 repository to understand what amendments have been included because until now I don't seem to have picked them all up in this port to BD. Hopefully this update in BD will correct any omissions from earlier releases.

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

No branches or pull requests

2 participants