schema defaults are not quoted and just broken in sqlite

This is a PHP notice, so I don't think that it is caused by the person who registered. Notices are one of the lowest levels of messages that PHP can throw, so this probably just wasn't caught by the developers of Drupal core yet.

I have made the title of this issue contain the actual error, so that it will be more likely to be reviewed by someone who can figure it out.

I'm getting this just viewing a user page, whether logged in or not.

Error messageNotice: unserialize(): Error at offset 5 of 22 bytes in UserController->attachLoad() (line 268 of /{path_to}/user.module).

not using SQLite, so i think that may rule that out.

Can someone reproduce this with the devel module's backtrace error handler on? (or else XDebug?) That way, we can see what is leading up to the error.

Retitling, since it occurs under more general conditions.

Reassigning and escalating, since I think #7 implies a schema update will be necessary.

MySQL's createFieldSql

    // $spec['default'] can be NULL, so we explicitly check for the key here.
    if (array_key_exists('default', $spec)) {
      if (is_string($spec['default'])) {
        $spec['default'] = "'" . $spec['default'] . "'";
      }
      elseif (!isset($spec['default'])) {
        $spec['default'] = 'NULL';
      }
      $sql .= ' DEFAULT ' . $spec['default'];
    }

vs. SQLite's createFieldSql

      if (isset($spec['default'])) {
        if (is_string($spec['default'])) {
          $spec['default'] = "'" . $spec['default'] . "'";
        }
        $sql .= ' DEFAULT ' . $spec['default'];
      }

vs. pgsql

    if (isset($spec['default'])) {
      $default = is_string($spec['default']) ? "'" . $spec['default'] . "'" : $spec['default'];
      $sql .= " default $default";
    }

Moving to sqlite queue.

Please leave the version at 7.x-dev, otherwise it doesn't show up in the right lists.

INSERT INTO "users" VALUES(5,'REDACTED','REDACTED','','',0,0,0,1,'Europe/London','',0,'REDACTED','NULL','2');

The last NULL value (for the data column) is actually a string 'NULL' instead of the constant NULL, which I think is the cause of the error. Now the real problem is 'NULL' being inserted as a string of course...

edit: The default for the data column is the string 'NULL' as well, which I think is caused by an error in introspectSchema():

$schema['fields'][$row->name] = array(
 	  	  	'type' => $type,
 	  	  	'size' => $size,
 	  	  	'not null' => !empty($row->notnull),
 	  	  	'default' => trim($row->dflt_value, "'"),
 	  	  	); 

This sets $schema['fields'][$row->name]['default'] to 'NULL' (string) instead of NULL (constant) in case of the {users}.data field.

Changing the title to reflect the underlying bug.

The same error occurs for the default of the INTEGER columns being set to '0' (string) instead of 0 (int), which is not so much a problem because of re-casting on insert.

Ew. I see.

Raising to critical because we might need a schema update for this :(

Small debug session:

sqlite> CREATE TABLE "test" (
   ...> field_null VARCHAR(10) NULL DEFAULT NULL,
   ...> field_null_string VARCHAR(10) NULL DEFAULT 'NULL',
   ...> field_int INTEGER NOT NULL DEFAULT 0,
   ...> field_int_string INTEGER NOT NULL DEFAULT '0',
   ...> field_text TEXT NULL DEFAULT NULL,
   ...> field_text_string TEXT NULL DEFAULT 'NULL',
   ...> field_float FLOAT NOT NULL DEFAULT 0.45,
   ...> field_float_string FLOAT NOT NULL DEFAULT '0.45');
sqlite> PRAGMA table_info('test');
0|field_null|VARCHAR(10)|0|NULL|0
1|field_null_string|VARCHAR(10)|0|'NULL'|0
2|field_int|INTEGER|1|0|0
3|field_int_string|INTEGER|1|'0'|0
4|field_text|TEXT|0|NULL|0
5|field_text_string|TEXT|0|'NULL'|0
6|field_float|FLOAT|1|0.45|0
7|field_float_string|FLOAT|1|'0.45'|0

This means that when the default value is defined as a string, it is returned enclosed in single quotes. When it is NULL, an integer, a float... it is returned without quotes.

edit: does this mean we need to alter each and every field that has been altered in a previous update function again after the introspection is fixed? Or maybe only those after beta 1 is released, since sqlite wasn't included in D6?

Yes, the real issue is just that 'default' => trim($row->dflt_value, "'") is too naive.

Yup. Write tests for this please. Note that trim is wrong, it rips out valid apostrophes, if the default starts or end with one.

note that even this might not be enough, i did not check that default value does not come out the string N-U-L-L instead of just being NULL. In the latter case we need some truly ugly else cause to my patch.

Note that trim is wrong, it rips out valid apostrophes, if the default starts or end with one.

+        if (isset($row->dflt_value[0]) && $row->dflt_value[0] == "'") {
+          $row->dflt_value = substr($row->dflt_value, 1, -1);
+        }

Isn't this the same as trim($row->dflt_value, "'")? I don't see what the patch accomplishes.

I'm new to the drupal community, but is there something like unquote in the core?

carlos8f's suggestion is OK, but will break a string like:

$row->dflt_value = "'oy there mate!";

How about an unquote function like:

/**
 * Unquote a string and optionally return the quote removed.
 *
 * @param string $s A string to unquote
 * @param string $quotes A list of quote pairs to unquote
 * @param string $left_quote Returns the quotes removed
 * @return Unquoted string, or same string if quotes not found
 */
function unquote($s, $quotes = "''\"\"", &$left_quote = null) {
	if (is_array($s)) {
		$result = array();
		foreach ($s as $k => $ss) {
			$result[$k] = unquote($ss, $quotes, $left_quote);
		}
		return $result;
	}
	if (strlen($s) < 2) {
		$left_quote = false;
		return $s;
	}
	$q = substr($s, 0, 1);
	$qleft = strpos($quotes, $q);
	if ($qleft === false) {
		$left_quote = false;
		return $s;
	}
	$qright = $quotes[$qleft + 1];
	if (substr($s, -1) === $qright) {
		$left_quote = $quotes[$qleft];
		return substr($s, 1, -1);
	}
	return $s;
}

I see now why trim() is wrong, it removes multiple instances of single quotes, whereas we only want to remove the outer instances.

How about:

if ($row->dflt_value == 'NULL') {
  $default = NULL;
}
elseif (is_string($row->dflt_value) && $row->dflt_value[0] == "'") {
  $default = substr($row->dflt_value, 1, -1);
}
else {
  $default = $row->dflt_value; // Cast to proper type?
}

Are we really sure that we get a quoted string here? That sounds very very doubtful to me.

Re: #24, yes we do get a quoted string.

And #25 looks correct because as we've found out, we should also be unescaping literal single quotes here, by un-doubling them.

So who wants to write the test? :P

+++ includes/database/sqlite/schema.inc	2010-12-10 23:28:35 +0000
@@ -380,11 +379,28 @@ class DatabaseSchema_sqlite extends Data
+        // The default is always a PHP string, if the default is a string in
+        // SQLite too then it arrives quotes, remove them.

This is not proper English.

+++ includes/database/sqlite/schema.inc	2010-12-10 23:28:35 +0000
@@ -380,11 +379,28 @@ class DatabaseSchema_sqlite extends Data
+          // Single quotes are escaped by storing them doubled-up. De-escape
+          // them to avoid the driver double escaping them.
+          $default = str_replace("''", "'", substr($default, 1, -1));

I don't know what is meant with 'doubled-up'. Do you mean they are escaped twice? It is not clear why there are 2 quotes.

"doubled up" means literally that -- SQLite requires you to escape single quotes by doubling them, i.e. 'Dries''s comment'

Ups, that was meant to be a comment saying "this patch fixes the issues Dries pointed out with the documentation"

+ // are discarted using substr.

Should be "discarded".

Leaving at "needs review" for review of actual code.

Working on tests.

Here we go.

identical to #33 w/o the whitespace issue

+      if (!empty($field_spec['default'])) {
+        $this->assertEqual($field_value, $field_spec['default'], t('Default value registered.'));
+      }
+      else {
+        $this->assertIdentical($field_value, $field_spec['default'], t('Default value registered.'));
+      }

I guess this is intended to be a isset() instead of a !empty()?

No, I wanted to avoid the usual 0-null-'' equivalence.

This seems to affect primarily SQLite and Postgres, so I will let chx and Damien make the final RTBC. However, reading through the patch in #34 it looks good to me. +1 when chx and Damien are happy with it.

The remaining problem here: if someone installed a site with RC2 on SQLite they *might* have a broken site. We might need to go over the tables and fix defaults from quoted-null to just null. Not sure, needs testing. As for #37, consider setting the default to NULL or 0 and getting back something else, assertEqual will give you a pass. There might be better ways but this is good enough. You can't always do assertIdentical because we stringify fetches so setting a default of 7 and reading it back is '7'

Sounds good to me.

wth, no, this is not ready, what about the upgrade path I mentioned?

Confirming the upgrade issue. The test fails both before and after the fix is applied to a 7.0-rc2 installation.

While the usage of quote is more appropriate (and I am sure semicolons in defaults would present a huge problem), I am unable to reproduce the issue -- note that there are no clear reproduction instructions. I have tried beta 3 and RC2 both, created a user, visited the profile, no probs. Tried field_revision_comment_body (edited for brevity):

sqlite>  insert into field_revision_comment_body (etid,entity_id,revision_id,delta) values (1,1,1,1);
sqlite> select * from field_revision_comment_body;
1||0|1|1||1||
sqlite> .dump field_revision_comment_body
CREATE TABLE field_revision_comment_body (
....
comment_body_format VARCHAR(255) NULL DEFAULT NULL, 
...
INSERT INTO "field_revision_comment_body" VALUES(1,'',0,1,1,'',1,NULL,NULL);

not a 'NULL' in sight.

Now, I can prove that indeed 'NULL' was sent to SQLite but the above proves that SQLite somehow converts that to NULL (once again edited for brevity):

sqlite> .mode insert foo
sqlite> pragma table_info(field_revision_comment_body);
INSERT INTO foo VALUES(0,'etid','INTEGER',1,NULL,1);
...
INSERT INTO foo VALUES(7,'comment_body_value','TEXT',0,'NULL',0);
INSERT INTO foo VALUES(8,'comment_body_format','VARCHAR(255)',0,'NULL',0);

See how the 'NULL' became a NULL both in INSERT and CREATE TABLE? (Edit: i tested and the 'NULL' is not an artifact of the mode insert. It can use and display NULL just fine.)

And there is nothing to upgrade.

I tried to replicate the initial problem, but wasn't able to.

Installed the latest dev on sqlite, created a new user, created content (with body empty), browsed and clicked, didn't see anything.
It's very possible that I missed something, but I just wanted to report this...

I gave this a look by checking out 7.x-beta2 and doing an install. I also cannot replicate the problem of quoted NULLs showing up in the schema, as in #43.

The patch in #34 no longer cleanly applies; modules/simpletest/tests/schema.test has changed enough that only one hunk works. I'm not sure where the tests it's trying to change have wandered off to; git grepping around didn't find them.

I think we still want to quote things the right way, so this patch is still in the running, but someone in the know about where the tests are nowadays needs to update that part of it. Reducing priority, however, as it appears that the problem is not as dire as it ever seemed.

I'm getting this error too, using Drupal 7.2

Notice: unserialize() [function.unserialize]: Error at offset 0 of 4 bytes in UserController->attachLoad() (line 287 of /var/www/docs/www.mysite.com/htdocs/modules/user/user.module).

I'm still setting up my site, and hadn't noticed it before until after I deleted some users. This error shows up on all user profile pages, except for the Admin's profile.

I can't patch; I have tried before, and fail every time. Will this be fixed in a future version of Drupal?

Triaging.

Looks like this is still an issue in Drupal 9 in terms of not casting the default value:

'default' => trim($row->dflt_value, "'"),

The default does seem to be escaped by the driver, at least.

This was a bugsmash triage target. lendude suggested that this was fixed now that everything is quoted. He also pointed out that this was reported not reproducible in #45. Also, the code around the line from #52 was modified in #3232699: SQLite schema introspection implementation struggles with NULL default values.

I think it is time to close this as outdated.

If you are experiencing this problem on a supported version of Drupal reopen the issue, by setting the status to 'Active', and provide complete steps to reproduce the issue (starting from "Install Drupal core").