PHP 5.4 "Illegal string offset" warning when install

Sorry for the new-bee question... but how do you install the patch?

If it matters, my environment is Windows 7 with Apache 2.2 and PHP 5.4.3.

#1: illegal_string_offset-1542186-1.patch queued for re-testing.

This issue occurs during a fresh installation. I just performed a fresh install and I get this error.

Also, the patch does not completely fix the issue in 7.15. See attached.

#1 actually does work for Drupal 7.15. It reduces the number of errors on installation from 30+ down to 4, and it actually allows the installation to complete versus a aborting the process altogether. Unfortunately the patches in #1 and #5 are for Drupal 8 (e.g. using the core folder), so this version is for Drupal 7.

If we need to have the D8 submission accepted first, so be it... but this is a major ticket for sqllite users on D7 as you essentially cannot install Drupal without this patch. I'd like to push higher to critical but I know that mysql is the dominant database used in the community.

Because this has to be addressed in D8 before being backported to D7, this is a review of patch #1 using the current branch of D8 on an OS X MAMP stack using php 5.4.4 with the standard installation profile and SQLlite. I'm not using the built in php server, but that's not relevant from my end because I was able to demonstrate that the installation fails with the same errors using apache 2.2.x.

After applying #1 with D8 standard profile + sqllite on php 5.4.4, the installation now works with no visible errors to the user. I'm marking RBTC. Patch #7 (re-rolled against D7) will be reviewed after this is successfully committed to the D8 core.

Thanks for the fix! Could someone quickly verify that these errors do not appear on MySQL or some other database platform? In other words, is it correct that this is a SQLite only fix?

Hi @webchick,

I just applied and ran patch #1 against the most up to date version of D8 and can confirm that #1 does not introduce errors into a mysql installation... and a mysql installation works regardless of this patch or not. Also, because the scope of the patch in #1 is limited to a single function for the sqlite drivers, I don't think it would conflict.

When #1 gets accepted, I'd be pumped to see my D7 port in #7 also get accepted (pretty please with a cherry on top?) :) Drush quick-core-install is pretty much useless with this SQLite bug.

Ok, awesome. Thanks for the confirmation. :D We currently have a testbot backlog, so I'm not going to commit this just yet, but will take a look at it tonight. Thanks!

Test env:
PHP 5.4.5
PostgreSQL 9.14

Install perfect with or without #1, RTBC from me.

Looks good to me, just a straight error in the sqlite driver. Committed/pushed to 8.x.

There's already a 7.x patch here..

Committed and pushed to 7.x, too. Thanks!

YAY!!!! Thanks for both the D7 and D8 commits :) Cheers!

I kindly request to roll back this patch in 7.x as well as 8.x and instead go for the solution proposed in #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed.

I'm not convinced that is a better fix, or even related. Please follow-up in the other issue.

I think we really have to revisit this patch. Not only because it blocks the other issue but also because it does not solve the reported issue in the first place. It also does not have anything to do with PHP 5.4 but just with having error_reporting = E_ALL in php.ini.

I have:

$ php -v
PHP 5.3.3-7+squeeze14 with Suhosin-Patch (cli) (built: Aug  6 2012 14:18:06) 
Copyright (c) 1997-2009 The PHP Group
Zend Engine v2.3.0, Copyright (c) 1998-2010 Zend Technologies
    with Xdebug v2.1.0, Copyright (c) 2002-2010, by Derick Rethans
    with Suhosin v0.9.32.1, Copyright (c) 2007-2010, by SektionEins GmbH
$ git checkout 7.x
$ git shortlog | grep 1542186
      Issue #1542186 by pp, redcell, rickmanelius: Fixed PHP 5.4 'Illegal string offset' warning when install.

And nevertheless I get:

$ drush site-install --db-url=sqlite:/dev/shm/test.sqlite
You are about to DROP all tables in your '/dev/shm/test.sqlite' database. Do you want to continue? (y/n): y
Starting Drupal installation. This takes a few seconds ...                                                                                                                                                             [ok]
WD php: Warning: Illegal offset type in unset in UpdateQuery_sqlite->removeFieldsInCondition() (line 80 of /[...]/includes/database/sqlite/query.inc).                                                    [warning]
WD php: Warning: Illegal offset type in unset in UpdateQuery_sqlite->removeFieldsInCondition() (line 80 of /[...]/includes/database/sqlite/query.inc).                                                    [warning]

Please revert this patch and then help me fix the real cause of that headache in the other one.

#5: patch.diff queued for re-testing.

quick fix.

(the sqlite test system is broken: #1787004: [SQLite] Can't run tests )

I don't think that testing this patch on the MySQL testbot is very usefull. The changes apply to the SQLite driver exclusively.

does not seem major to me

@znerol,

You can download the patch and test it.

Like I mentioned in #19, I'd rather advise to revert the changes introduced here and instead remove the entire removeFieldsInCondition method (as suggested by its original author Damien Tournoud). This would resolve this issues as well as #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed.

IMHO it is useless to patch removeFieldsInCondition if it should be removed entirely.

In Drupal 7 at least, we cannot just rip out methods, though.

@webchick: I know that we cannot change API in minor versions. However removeFieldsInCondition is a protected method of the class UpdateQuery_sqlite. This class is only loaded when sqlite is choosen as the database backend. The method was introduced by Damien but he "[... does] not remember why [he] did this in the first place, and it indeed looks very wrong.". As demonstrated in the other issue, this method leads to data corruption when only the primary key of an entity changes. There is also a test case in the patch of the other issue demonstrating the mechanism as well as detailed instructions on how to reproduce the problem.

Regrettably the "fix" introduced in this issue - which does actually not fix the reported warnings, at least not in the version it was commited - conflicts with the solution we propose in #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed.

I suggest that we split up the discussion as follows.

1. We first decide over at #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed if that issue should be fixed or not.
2. After that we decide here if commit 669b268 should be reverted or not.
3. Finally I will reroll the patch in the other issue according to the decisions taken.

This (private) method is completely unnecessary, it should just be removed. Let's move forward on removing it in #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed.

I'm not sure about anything else, but I can guarantee that the Drupal 7 patch was broken:

-      if ($child_condition['field'] instanceof QueryConditionInterface) {
....
+        if ($child_condition['field'] instanceof ConditionInterface) {

This cannot possibly be correct (it looks like a bad copy-paste job from the Drupal 8 patch).

I'm not quite sure what the consequences are, but don't really want to find out in a point release either.

Therefore, I've rolled this patch back in Drupal 7: http://drupalcode.org/project/drupal.git/commit/6b11350d32b753a45f481432...

Tentatively setting this back to needs work (for Drupal 7), though if we're removing the method in Drupal 7 also (rather than trying to fix it again), feel free to close this again as a duplicate.

On the advice in #31 and since nothing is moving forward on either issue I'm closing this one as a dup of #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed

Updated issue summary.