There is a workaround in place in the sqlite database driver which is supposed to fix an issue for sqlite reporting matched rows instead of affected rows for an update-query (also discussed in #805858: Affected rows inconsistent across database engines). However this workaround introduces problems in the situation when only the key (e.g. machine_name, tid, nid) of a row should be updated but all the other values remain the same. In this situation an update to the record will be suppressed.

It is easy to reproduce this problem by editing the machine_name of a content_type. If only the machine_name is changed, the change will not be written to the db. If however another field like the human readable name is changed during the same edit, the record gets written to the db properly.

The relevant code resides in includes/database/sqlite/query.inc:52 and is introduced by the following comment:

/**
 * SQLite specific implementation of UpdateQuery.
 *
 * SQLite counts all the rows that match the conditions as modified, even if they
 * will not be affected by the query. We workaround this by ensuring that
 * we don't select those rows.
 *
 * A query like this one:
 *   UPDATE test SET name = 'newname' WHERE tid = 1
 * will become:
 *   UPDATE test SET name = 'newname' WHERE tid = 1 AND name <> 'newname'
 */
class UpdateQuery_sqlite extends UpdateQuery {
[...]

Would it be sufficient to rewrite the code such that all fields get a name<>'newvalue' clause, even those which also appear in the condition? Imagine we'd like to update tid from 1 to 2 but leave name unchanched, in my opinion the query would have to translate to:

 UPDATE test SET tid = 2, name = 'oldvalue' WHERE tid = 1 AND tid <> 2 AND name <> 'oldvalue'
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Status: Active » Needs review

Hi there, and thanks for your report.

The current logic will only add additional conditions to the query if the updated fields are not already in the condition. I don't remember why I did this in the first place, and it indeed looks very wrong.

I'm currently run the test suite the fix sandbox/damz/1185354.git@31c6e2f.

znerol’s picture

FileSize
980 bytes

Attached is a patch which tests for the misbehaviour. The test succeeds with your fix from the sandbox and fails without it.

Damien Tournoud’s picture

This causes a lot of breakage everywhere, I'm not completely sure why. See the results.

znerol’s picture

That's strange, I was not able to reproduce those test results on my local machine. Neither with your branch nor with vanilla 7.x. Also I was not able to locate changeset 412ef3d4e6687c712c470d53f4be175a42d03d3d which apparently is checked out on the Jenkins server. Where does the code come from on the continous integration platform?

Damien Tournoud’s picture

Indeed, it seems that it was just an instability due to the test server being overloaded. This patch passed since then.

znerol’s picture

IMHO this patch should be committed soon because it fixes a problem which can cause inconsistencies in the database. As an example consider the case I've mentioned in the original post. When the machine_name of a content type is changed, attached field instances will be updated to point to the new bundle. However because the type column in the node_type table is not updated due to the problem I've pointed out above, the content type will loose all attached fields.

znerol’s picture

Version: 7.7 » 8.x-dev
FileSize
3.53 KB

Update patch and test case for drupal 8.x.

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Let's get this in.

znerol’s picture

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs backport to D7

OK the fix looks good but please remove the link back to this issue, git blame works fine.

+   * Bug: http://drupal.org/node/1266572
znerol’s picture

The patch does not apply anymore due to #1542186: PHP 5.4 "Illegal string offset" warning when install. Over there a fix for removeFieldsInCondition was introduced while I propose to drop this method entirely. What is the right procedure to continue here. IMHO it would be cleaner to revert the changes introduced by the other fix. But I also may just reroll my patch on top of them.

droplet’s picture

Issue tags: +Novice

Novice: reroll patch

znerol’s picture

Issue tags: -Novice

From #1542186-29: PHP 5.4 "Illegal string offset" warning when install

I suggest that we split up the discussion as follows.

1. We first decide if this issue should be fixed or not.
2. After that we decide over at #1542186: PHP 5.4 "Illegal string offset" warning when install if commit 669b268 should be reverted or not.
3. Finally I will reroll the patch according to the decisions taken.

Before anyone fires up his editor I'd like some comments from one or two core developrs/commiters whether the solution proposed in #8 (d8) and #10 (d7) is acceptible.

Damien Tournoud’s picture

This method is completely unnecessary, it should just be removed. Let's move forward with this.

znerol’s picture

Rerolled patch on top of current 8.x. Also removed the issue url from the comment in the test-case as suggested by catch (#11)

znerol’s picture

Status: Needs work » Needs review
FileSize
3.29 KB

Same for 7.x

dcrocks’s picture

I've been getting the following message for the last 2 weeks running recent 7.x dev versions, including today's latest:
Warning: Illegal offset type in unset in UpdateQuery_sqlite->removeFieldsInCondition() (line 80 of /Users/rocks/Sites/drudev/includes/database/sqlite/query.inc).
I haven't tried this fix yet as it isn't quite convenient for me but my question is 'is my database borked'?

znerol’s picture

fenstrat’s picture

I can confirm that the D7 version in #20 fixes errors with PHP 5.4 and drush quick-drupal #1547526: drush qd doesn't work with PHP 5.4.

dcrocks’s picture

Priority: Normal » Major

I can't install drupal 7 dev on a system with php 5.4 without getting this error. Drupal 8 dev installs ok.

czigor’s picture

I could not install drupal7 with php 5.4 and a tmpfs+sqlite3 db. It was trying to write to the watchdog table when it did not exist yet.

Applying the patch in #20 solved it for me.

dcrocks’s picture

Priority: Major » Critical

Bumping this because I cannot run drupal v7 on a php 5.4 system. The aforementioned patch #1542186: PHP 5.4 "Illegal string offset" warning when install was apparently only rolled back on drupal 7. On my rhel 6.3 system running php 5.4 drupal 8 works but drupal 7 doesn't. Drupal 7 works after applying the patch in #20.
As far as I can tell there is no published note prohibiting running drupal 7 with php 5.4 and I don't want to work on a patched system. This issue has been open for more that a year. If a better patch than # 20 is needed, please someone provide one soon. Therefore I think it appropriate to bump this. Fill free to correct me.

fenstrat’s picture

fenstrat’s picture

Status: Needs review » Reviewed & tested by the community

#20 is still green. Given recent feedback I'm going to tentatively mark this as RTBC.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Priority: Critical » Major
Status: Reviewed & tested by the community » Patch (to be ported)

Breaking SQLite isn't a critical, but it does qualify as major.

Committed and pushed #20 to 8.x, since the only thing catch seemed to find in #11 was fixed.

Moving down to 7.x. We'll need the patch in #20 re-uploaded without the do-not-test.patch suffix to make sure that testbot still passes (I would just do that but don't want to get false commit credit).

znerol’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.99 KB

Thanks webchick. Attached patch from #20 with do-not-test suffix removed.

fenstrat’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, tests are passing, back to RTBC for D7.

Heine’s picture

> Breaking SQLite isn't a critical, but it does qualify as major.

If SQLite isn't guaranteed to work from one minor to another lets just drag the driver out to the fields to shoot it in the back of the head (aka "move to contrib") and be done with it.

dcrocks’s picture

I would just comment that from my viewpoint it is Drupal that is broken, whatever sub-component actually fails. Can we please get this in?

joshf’s picture

Assigned: Unassigned » joshf
znerol’s picture

Assigned: joshf » Unassigned

Assigned to: Anonymous » joshf

I guess this was an accident.

joshf’s picture

Assigned: Unassigned » joshf
FileSize
2.77 KB

Backported to D7; patch fixed the issue for me and I was able to install without problems.

joshf’s picture

Status: Reviewed & tested by the community » Needs review
znerol’s picture

@joshf: Diffing #34 against #28 does not show any code-changes. What is the reason for reuploading the patch and demoting the issue from RTBC to needs review?

dcrocks’s picture

Status: Needs review » Reviewed & tested by the community

When is anyone going to process the the 7.x RTBC queue? Issues have been waiting more than a month.

joshf’s picture

@znerol Nothing, I guess. Sorry, I was following the instructions at http://drupal.org/node/1538380 for the first time when I encountered this issue. I didn't realize that rdbc meant it didn't need to be backported. I thought it needed a separate patch based on 7.x and that I was helping it along. I'll be more careful next time.

znerol’s picture

Assigned: joshf » Unassigned

@joshf: No problem. As far as I understand the status-docs you need to look for issues where status is "patch (to be ported)". Thanks for your contribution anyway. I think it is a very good sign that both of us came up with the exact same patch :)

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/bfaa7b4. I also removed t() from the test assertion messages on commit (since that's recently been fixed throughout the database tests). Someone can forward-port that cleanup to D8 if they want to.

@dcrocks, Drupal 7 point releases only happen on a defined schedule, so why does it matter if the RTBC queue builds up a bit in the interim?

dcrocks’s picture

I was under the impression things were committed in between point releases. I don't like working with patched code, but I'm ok with dev. Thanks for putting this in.

David_Rothstein’s picture

Yeah, I do usually try not to bunch them all at the end, but sometimes it happens that way :) The reason I don't think it matters much is that the point releases are the only things that are "officially" supported, so we just need to make sure those are as solid as possible when they come out.

(And for what it's worth, I actually believe it's preferable to run a point release with a couple well-tested patches applied, rather than a dev version. In a dev version you are running a much larger amount of unsupported code. Often it won't matter - and hopefully it never does - but if a security issue or upgrade path problem comes up that only affects dev versions, you could find yourself out of luck without a fix.)

dcrocks’s picture

I appreciate all your comments. Of course I only use dev for dev and have shot myself in the foot a couple of times. I realize that in those cases the burden is on me but I just feel that I want to encounter something that might break what I'm doing before a point release and its possible production implementation.
Again, thanx for your work.

David_Rothstein’s picture

Issue tags: +7.22 release notes

Fixing tags since Drupal 7.21 only contained a fix to deal with fallout from the Drupal 7.20 security release. Hopefully this is really the last time I do this and this will be released in Drupal 7.22 for real :)

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

Issue summary: View changes

Change

 to