OS: Windows Server 2012
Database: SQLSRV
Web Server: IIS 8
PHP 5.6.13
Drupal 7.29
Message from Update.php:

Update #7425
Failed: PDOException: SQLSTATE[23000]: [Microsoft][ODBC Driver 11 for SQL Server][SQL Server]Cannot insert the value NULL into column 'exclude_empty', table 'D_openeye.dbo.webform_emails'; column does not allow nulls. UPDATE fails.: ALTER TABLE webform_emails ALTER COLUMN [exclude_empty] smallint NOT NULL; Array ( ) in db_add_field() (line 2844 of C:\inetpub\wwwroot\openeye.drupal\includes\database\database.inc).

//UPDATE
This may be an issue with SQLSRV driver.

CommentFileSizeAuthor
#17 sqlsrv-fixnull-apply-on-empty-default-2581349-17.patch1.07 KBmattlc
#10 2581349-not-null-default-value-0-10.patch1.22 KBdobe
#6 2581349-not-null-initial-value.patch5.04 KBdobe

Comments

dobe created an issue. See original summary.

dobe’s picture

dobe commented
Issue summary: View changes
dobe’s picture

dobe commented
Project: Webform » Drupal driver for SQL Server and SQL Azure
Version: 7.x-4.11 » 7.x-2.0

I am not seeing an issue with Webform's code here. I think it is a SQLSRV driver issue.

dobe’s picture

dobe commented
Project: Drupal driver for SQL Server and SQL Azure » Webform
Version: 7.x-2.0 » 7.x-4.11

Back to webform.

According to: https://api.drupal.org/api/drupal/includes!database!database.inc/functio...

I believe We need to set an "initial" value in the specifications. There are places in the webform.install but for some reason it was not being done anymore. Patch shortly to address this.

dobe’s picture

dobe commented
Version: 7.x-4.11 » 7.x-4.x-dev
dobe’s picture

dobe commented
StatusFileSize
new 2581349-not-null-initial-value.patch5.04 KB
dobe’s picture

dobe commented
Status: Active » Needs review
danchadwick’s picture

danchadwick commented
Status: Needs review » Active

This doesn't look right to me. It looks like a bug is the MSSQL driver.

Here's the MySQL version:
https://api.drupal.org/api/drupal/includes%21database%21mysql%21schema.i...

From db_add_field (emphasis mine):

$spec: The field specification array, as taken from a schema definition. The specification may also contain the key 'initial'; the newly-created field will be set to the value of the key in all rows. This is most useful for creating NOT NULL columns with no default value in existing tables.

This should only be needed when there is NOT a default, but all the instances changed in the patch do have a default. What's the MS code look like?

It would be a performance hit to add initial unnecessarily.

dobe’s picture

dobe commented

I was leaning towards that at the beginning. The code in the sqlsrv is slightly different to mysql as it does this:

    // Switch to NOT NULL now.
    if ($fixnull === TRUE) {
      // There is no warranty that the old data did not have NULL values, we need to populate
      // nulls with the default value because this won't be done by MSSQL by default.
      if (!empty($spec['default'])) { <--- This is where I am guessing the issue is.  Since the default value is 0 maybe it is reading them empty?
        $default_expression = $this->defaultValueExpression($spec['sqlsrv_type'], $spec['default']);
        $this->connection->query_direct("UPDATE [{{$table}}] SET [{$field}] = {$default_expression} WHERE [{$field}] IS NULL");
      }
      // Now it's time to make this non-nullable.
      $spec['not null'] = TRUE;
      $this->connection->query_direct('ALTER TABLE {' . $table . '} ALTER COLUMN ' . $this->createFieldSql($table, $field, $spec, TRUE));
    }

But your right. I feel there is kind of multiple ways to take the statement on the docs: $spec: The field specification array, as taken from a schema definition. The specification may also contain the key 'initial', the newly created field will be set to the value of the key in all rows. This is most useful for creating NOT NULL columns with no default value in existing tables.

dobe’s picture

dobe commented
Project: Webform » Drupal driver for SQL Server and SQL Azure
Version: 7.x-4.x-dev » 7.x-2.x-dev
StatusFileSize
new 2581349-not-null-default-value-0-10.patch1.22 KB

SQLSRV seems to double up it's effort's in these functions. I am guessing the problem would exist in both scenarios. The mysql version consolidates the last part of addField and runs changeField on it so I am not sure exactly why there is all the comments within this function about 

// There is no warranty that the old data did not have NULL values, we need to populate
// nulls with the default value because this won't be done by MSSQL by default.

As it appears based on the mysql code that it has the same issue hence passing the data to changeField function.

dobe’s picture

dobe commented
Status: Active » Needs review
danchadwick’s picture

danchadwick commented

Bingo. The PHP function empty() does not do what many developers think it does. Unfortunately, it is frequently used within Drupal when the value is very unlikely to be "0", such as in a user-entered name. Drupal core is filled with these small bugs.

http://php.net/manual/en/function.empty.php

Your patch will also address the bug when the default is the empty string, as well as a float 0.0.

mattlc’s picture

mattlc commented

Hi,
tested #10 : It does the job.
Before this patch (only #10), i could not manage to install various critical modules such as ds, block_class and even UUID.
Hope this will be released soon.

It would be great if the patch could be applied to root dir instead of includes/database, but I can wait for release...

Thx.

dobe’s picture

dobe commented

Great if you could mark as Reviewed and Tested that would be great. The patch is applied from a cloned version of the module. So I am not sure about the remark:

It would be great if the patch could be applied to root dir instead of includes/database, but I can wait for release...

Thank you!

mattlc’s picture

mattlc commented
Status: Needs review » Reviewed & tested by the community

See #13.
@dobe, don't worry about that misunderstanding.

david_garcia’s picture

david_garcia commented
Status: Reviewed & tested by the community » Needs work

This thing needs tests. I can't believe those default values were broken!

mattlc’s picture

mattlc commented
StatusFileSize
new sqlsrv-fixnull-apply-on-empty-default-2581349-17.patch1.07 KB

Hope this help.
@DanChadwick seems to be right. "empty" cause problematic behavior with non empty tables when adding column not null with empty default value.

I tried all available versions and ended with this "stronger" test :
"!empty($spec['not null']) " becomes "isset($spec['not null']) && $spec['not null']==TRUE"

With default collation set to recommanded "Latin1_General_CI_AI".
It seems to be OK.

mattlc’s picture

mattlc commented
Status: Needs work » Needs review
danchadwick’s picture

danchadwick commented

@mattlc -- what distinction are you making between:
$spec['not null'] and $spec['not null']==TRUE?

While the PHP rules for == are convoluted, I'm pretty sure that not_null will always be a boolean or a truthy value. I think the best code for checking a boolean such as this is !empty($spec['not_null']). This will be true for TRUE, any integer other than 0, any string other than "" or "0".

For example, your code would not be TRUE for 1, and I'm pretty sure if you set non_null to 1, you mean for the field to be not_null.

mattlc’s picture

mattlc commented

@DanChadwick
You are right.

I just tested this code : 

$test = array('test1'=>TRUE, 'test2'=>FALSE, 'test3'=>'', 'test4'=>1, 'test5'=>0, 'test6'=>true, 'test7'=>false, 'test8'=>'text');

foreach($test as $k=>$v){
  if(!empty($test[$k])){
    echo '!empty($test['.$k.'])<br/>';
  } else {
    echo 'empty($test['.$k.'])<br/>';
  }
}

if(!empty($test['nokey'])){
  echo '!empty($test[nokey])<br/>';
} else {
  echo 'empty($test[nokey])<br/>';
}

And it gives

!empty($test[test1])
empty($test[test2])
empty($test[test3])
!empty($test[test4])
empty($test[test5])
!empty($test[test6])
empty($test[test7])
!empty($test[test8])
empty($test[nokey])

So why UUID (and other modules defining shema with empty not null colmuns in non empty tables) can be installed with $spec['not null']==TRUE and not with !empty($spec['not null']) ?

virusakos’s picture

virusakos commented

I have tried patch #10 with the following modules (I'm not sure which of them are altering the tables with a not null column)

dependencies[] = l10n_update
dependencies[] = admin_menu
dependencies[] = admin_devel
dependencies[] = admin_menu_toolbar
dependencies[] = token
dependencies[] = pathauto
dependencies[] = transliteration
dependencies[] = ctools
dependencies[] = views
dependencies[] = views_ui
dependencies[] = libraries
dependencies[] = views_slideshow
dependencies[] = views_slideshow_cycle
dependencies[] = backup_migrate
dependencies[] = advanced_help
dependencies[] = languageicons
dependencies[] = variable
dependencies[] = entity_translation
dependencies[] = title
dependencies[] = i18n
dependencies[] = features
dependencies[] = strongarm
dependencies[] = uuid
dependencies[] = node_export
dependencies[] = ckeditor
dependencies[] = file_entity
dependencies[] = multiform
dependencies[] = plupload
dependencies[] = mediafield
dependencies[] = media_wysiwyg_view_mode
dependencies[] = media_wysiwyg
dependencies[] = media_internet
dependencies[] = media_bulk_upload
dependencies[] = media
dependencies[] = deploy
dependencies[] = deploy_ui
dependencies[] = services
dependencies[] = rest_server
dependencies[] = uuid_services
dependencies[] = entity
dependencies[] = entity_dependency

Everything works ok up to now.

david_garcia’s picture

david_garcia commented
Status: Needs review » Needs work
Issue tags: +Badly needs tests
joseph.olstad’s picture

joseph.olstad
Primary language French
commented

We've been successfully using patch 10 in our dev environment since Oct 23 2015. Applied over the latest 2.x dev version of sqlsrv.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Issue tags: -Badly needs tests +Needs tests

  • david_garcia committed 041bf7f on 7.x-2.x authored by mattlc 
    Issue #2581349 by dobe, mattlc: Update #7425: Failed: PDOException:...
david_garcia’s picture

david_garcia commented
Status: Needs work » Closed (fixed)
amleth2’s picture

amleth2 commented

With default collation set to recommanded "Latin1_General_CI_AI".
It seems to be OK.

Is the recommended collation for the SQL server database now Latin1_General_CI_AI, and not SQL_Latin1_General_CP1_CI_AS as per http://www.drupalonwindows.com/en/blog/installing-drupal-windows-and-sql...?

david_garcia’s picture

david_garcia commented

@amleth CI_AI is the recommended, because binary comparison (CS_AS) is an explicit option at the column level in Drupal.