Refactor file.inc to make it simpler and prevent PDO exceptions

It looks like file_create_filename() is also going to have this problem too.

I think the bug is real, but to make progress, we should find a nonbuggy way of triggering it. Which means, we either need a plausible scenario under which a public:// (or other non-temporary) file can be in {file_managed} but not on disk, or else a plausible scenario under which temporary:// files can exist (even if briefly) in {file_managed} (given a multiple web node scenario, we should always assume that temporary:// files might not be on disk at any particular time).

As per #2, a regular file field upload does not save an intermediary temporary:// file to {file_managed}, but possibly user module, media module, and other modules do, but the question is, is doing so ever legitimate, or is that a bug in itself?

I can confirm that #5 happens. Retitling and downgrading to normal, because I don't think it's major unless there's a way to reproduce it from Drupal interaction exclusively.

Considering this is the first I've heard of this with over a year of Drupal 7, I'm inclined to say it's more of a un-typical server configuration or unintentionally malicious admin removing files, which to me says this is normal.

How about this? This does #1 from the issue summary. I think randomizing suffix for race conditions belongs in a separate issue.

This patch looks good so I'm tempted to mark it RTBC.

The only question that came to mind is whether we need all these options? Can't we always check for the file to exist in both the database and on the file system? It seems odd that we would want to support inconsistent database tables.

The reason for FILE_CHECK_EXISTS_FILE is to make drupal_file_exists() by default be BC compatible with file_exists(), because:
1) I think we generally try to do that for drupal_*() versions of native PHP functions
2) So this patch can be backported to D7
3) So that when file_destination() is called from file_unmanaged_copy(), it works (unmanaged files are ones that don't exist in the {file_managed}).

The reason for FILE_CHECK_EXISTS_FILE_OR_ENTITY is to support the goal of this issue, which is to rename an upload to not clash with either something on disk, or something in the db. While we don't want to encourage inconsistency between db and disk, there are conditions in which it can happen (e.g., someone deletes a file from disk via the command line, but leaves the db record around, or an unmanaged file is copied to disk, and by definition isn't meant to have a db entry), and if we're saving an upload, we just want a name that's free both with respect to disk and with respect to db.

FILE_CHECK_EXISTS_FILE_AND_ENTITY is used by the test in this patch for asserting a file exists in both.

FILE_CHECK_EXISTS_ENTITY has no use-case so far, but is just there for symmetry.

The const keyword is not available in PHP 5.2.4/5.2.5 which is the minimim requirement for Drupal 7 as per http://drupal.org/drupal-7.0 and http://drupal.org/requirements

Wups, sorry, the issue is still on D8. Here is an updated D7 patch though. This should not be committed to D8. I'm setting back to RTBC for the D8 patch. I did not test either patch.

+++ b/core/includes/file.incundefined
@@ -2275,6 +2299,52 @@ function drupal_unlink($uri, $context = NULL) {
+function _drupal_file_entity_exists($uri) {
+  $query = new EntityFieldQuery();
+  return (bool) $query
+    ->entityCondition('entity_type', 'file')
+    ->propertyCondition('uri', $uri)
+    ->count()
+    ->execute();

Rather than having an internal helper function, we should convert this to file_load_by_uri() so that it actually returs a file object like user_load_by_name() does and therefore can be re-used by core and other modules. I think the use of EFQ is unnecessary here too. Just use a normal entity_load() because file entities are always in the database.

Hrm, could we maybe get more information about what exactly was causing the fails since it wasn't a drastic number of errors?

Tagging.

#21: 1556396-zombies-21.patch queued for re-testing.

Rerolled for PSR-0 tests.

complete sentence

This patch is for the new issue summary and contains the zombie test from #9.

Issue summary

Added some logic and tests for file_copy() / file_move() / file_save_data() so that filename of resultant entity is updated to the basename of the desired destination (provided it isn't a directory).The problem was identified here: #1415858: file_move() doesn't rename files correctly when the name of the file changes.

Fix typo

add info about entity filename

Possibly an issue on it's own, please let me know if so:

Is there any reason why file_unmanaged_copy uses file_exists when it could do fopen($source, "r") instead and get remote file support as well?

Edit: Spun of into #1736312: Let file_unmanaged_copy read remote files as well.

#33: refactor-check-files-exist.patch queued for re-testing.

#33: refactor-check-files-exist.patch queued for re-testing.

reroll

This is going to conflict heavily with #1468328: Move file entity info, managed file, and file usage functionality into File module, which is now RTBC. Sorry about that :) I guess you want to wait with further re-rolls until that one is in, and then the easiest way to re-roll might be to copy the new functions over to file.module.

Yeah - I know ;-). I'll reroll again when the other one gets in.

Marking related issue - #1163740: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '' for key 2: INSERT INTO {file_managed} - I'm not sure if it's a duplicate, but worth pointing out here. This is definitely the master thread.

That isn't a duplicate. There are many ways that "Integrity constraint violation" can occur.

This sounds like more a task than a bug to me, so moving it there to put us back under thresholds.

reroll

minor

#53: refactor-check-files-exist.patch queued for re-testing.

@jbrown thanks for looking after this issue so well.

To aid getting reviews, it's helpful if you post a sentence describing what changes you made, and an interdiff. If you want a hand with making the interdiff, let me know or ask in irc.

I've just been re-rolling the patch since #33 (August 15, 2012) - there aren't any changes.

OH! no wonder I was having trouble figuring out the differences. well, thanks again. :)

clarify

Fixed a test.

Clarify summary.

Summary clarification.

this has been being re-rolled for quite a while now.

is there any previous feedback that needs to be resolved?

Does this need a total re-review, or just a quick check to rtbc it?
I'm not sure what the next steps are to stop jbrown from re-roll-perpetuality. :)

reroll

Changed the entity query in drupal_file_exists() to use count().

#67: refactor-check-files-exist.patch queued for re-testing.

Reroll.

Update summary.

Removing tag.

Who is going to do this (D8 re-rolling)? Since it is quite a hassle to have outdated entries in the table file_managed which refer to deleted files (manually from the file system).

72: refactor-check-files-exist.patch queued for re-testing.

72: refactor-check-files-exist.patch queued for re-testing.

Needs a serious re-roll.

I have started the re-roll but I need time. Let me continue and add the patch.

Anything on this for Drupal 7 as I'm getting this when editing and changing nothing, then saving an Article:

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '32' for key 'vid': UPDATE {node} SET vid=:db_update_placeholder_0 WHERE (nid = :db_condition_placeholder_0) ; Array ( [:db_update_placeholder_0] => 32 [:db_condition_placeholder_0] => 61 ) in node_save() (line 1167 of /.../modules/node/node.module).

Unassigning since it's been quite a while. This would be quite a challenging reroll and it may be easier to redo the patch with the most recent patch in mind.

make testbot angry.

fixed some stuff.

makin progress.

more progress

I think I got all 8 testing green locally.
testbot, I know I've been hard on you lately, but it was for the greater good.
please be kind.

I just want to point out that the last time that patch was green was 2 years ago, and it's a 100kb patch.
Not to toot my own horn, but can I get a high five from someone?
That was some mean, nasty, thankless grunt work.

Where's my next "Needs reroll" tagged issue?

Thanks so much Sidney for taking the time to get this passing again - I spent the best part of a week in the summer of 2012 working on this.

The issue summary explains everything. I think the patch satisfies what @Dries asked for in #11.

The key point is that with this patch Drupal does not assume that what is on the filesystem necessarily matches what is in the database. They can diverge very easily, e.g. due to staging issues. Drupal should be able to handle this in a graceful manner instead of throwing a PDO exception that requires the developer to perform surgery in the database to resolve.

I was able to make the file API considerably simpler while solving this issue. From Sidney's latest patch:
28 files changed, 391 insertions(+), 607 deletions(-)

That's 216 fewer lines of code in core with a vastly more robust file API.

I have reviewed the patch and everything is in order. Because I am the original author of the patch I can't mark RTBC.

I not sure what the policy is at this stage, but this is a major bug that can only be resolved my fixing the API. my recommendation is for this too be committed. It makes the whole sub-system considerably simpler.

Can someone please review?

Yes, awesome work with that reroll.

I can't say if this can be committed to 8.0.x still. But to find that out, we need to make sure that the issue summary is up to date, prepare a change record and beta evaluation to argue why the API break would be worth it.

Another thing would be to check if we can find ways to make the API change smaller or keep a BC layer. Parts of file.inc recently moved to a FileSystem service. Maybe we can add the new API's there or make a new service for them, and keep the old functions ans depreated wrappers that still work as they did before.

See #2050759: Move drupal_chmod and other code in file.inc to a Drupal\Core\File\FileSystem class and the meta issue #2229865: [meta] Modernize File/StreamWrapper API.

I think having a BC layer is a bad idea because the old API is fundamentally broken and this patch simplifies everything.

I'm happy to move these functions into a service, but this can happen in a follow up.

I'm going to go ahead and great a change record / beta evaluation.

All that is above my paygrade.
I'm not familiar enough with the API to be able to weigh in. I've only been in Drupal for a couple weeks, and am just trolling the "Needs reroll" tickets, cause it doesn't actually require very much familiarity with Drupal, and lets me leverage my general php skills. I'll leave all the weighty discussion to my betters and move on to the next ticket.
At least you people have a good base to work off of, now. I'll be curious to see what comes of it.
Cheers.

Okay - I've added the beta phase evaluation and created the draft change record: https://www.drupal.org/node/2426725

Can someone review them?

We have an Elite customer at Acquia who may be experiencing this same PDOException using Drupal 7.32. What are the prospects of someone re-rolling this patch for Drupal 7? We could immediately test the rerolled patch on a live site (if it indeed solves their issue) and help to move the patch toward RTBC status.

@crotown I rolled #17 (the last D7 version of the patch that I could see) against 7.32 and ran the full test suite against it locally overnight, came up with 51 fails. Gonna keep working on it. I should be able to get something posted today.

That straight reroll of #17 against 7.32. Have to upload the patch here, so I can have a link to feed to simplytest.me.
Trying to setup a sandbox there.

sorry about the noise. I did ask in #drupal-infrastructure if there is an alternative way for my to use testbot outside of the issue, but haven't heard back. I'll change the version back to 8.0-dev soon.

changing version back to 8-0-dev
requeing D8 patch to top of deck. interdiff 94-108 was 0 bytes.

The patch #107 seems to have solved the issue for our D7 client, I'll report after it gets serious live use but it looks good so far.
@sidharrell: I cannot thank you enough! But thanks much, even though it is not enough.
The Drupal community is a wonder to behold!

Patch no longer applies.

I'll work on it and reroll the patch.

I'm a mentor. Helping @pektinasen to work on the issue.

This reroll seems too big. Too many lines of code changed and file conflicts. I stop working on it.
If anyone wants to help me with this issue I'm at the re-roll table in the mentored sprint room.

I don't know what happened to:
core/modules/migrate/src/Plugin/migrate/destination/EntityFile.php

Mostly just a reroll though.

File core/modules/migrate/src/Plugin/migrate/destination/EntityFile.php was deleted in #2534012: Move module-specific migration support into the file module.

Just rerolled to apply patch successfully.

Starting reroll.

…or not, looks like it's already done.

Is D7 patch stalled?

Hello.

I have an updated D7 patch for the one in #107.

I originally created it back in Oct/Nov of last year, but I wasn't sure it would actually work for the problems we were seeing. It turned out that it did, and it's been in production since mid-Nov. However, I forgot to submit it to this issue after realizing it was a keeper.

With the recent security update, the patch for this issue needs some work, since they both operate in some of the same functions.

So, attached is the updated patch with the security release accounted for.

I could not get an interdiff created, so I did not include one.
I kept getting this error:

1 out of 10 hunks FAILED -- saving rejects to file /tmp/interdiff-1.wMZiJE.rej
interdiff: Error applying patch1 to reconstructed file

I'm going to go ahead and adjust the issue for testing the D7 patch. Feel free to revert back to D8 afterwards.

Thanks for the updated D7 patch. Verified manually that it's still essentially the same as the one in #15 - #107.

Back to 8.x.

I think this issue could be closed as outdated, or at a minimum it needs an issue summary update. The original issue is no longer relevant as there is no unique constraint on file_managed.uri since #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols); and all functions in file.inc are deprecated.

Agree that

as there is no unique constraint on file_managed.uri

means this is no longer relevant