Switch registry back to using hashes not file mtime and ctime [#819388]

In registry.inc we have:

      $modified_file = !isset($file['filectime']) || !isset($file['filemtime'])
                  || $filectime != $file['filectime'] || $filemtime != $file['filemtime'];

as our method for detecting changed files, which may cause weird problems with multiple web nodes with separate file systems, or when transferring DB dumps and code to and from development and produciton.

Let's swithc back to using more robust hashing of files for this.

Comment	File	Size	Author
#3	819388-registry-file-hashes-3.patch	6.39 KB	pwolanin
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Anonymous (not verified) CreditAttribution: Anonymous commented 6 June 2010 at 06:10

for reference, here's the issue where we changed from md5 to file times: #513592: Simpletest 7 much slower than 6.

i'd support moving back, as the current checks don't handle the case of different contents, same ctime/mtime. that's clearly an edge case, but given that a registry error can hose your entire site, probably worth covering.

Comment #2

catch

he/him

English

CreditAttribution: catch commented 7 June 2010 at 03:07

Subscribing. #797024: Moving a class between files can cause SQL errors on registry rebuild just doubled the time it takes to install / run simpletests, reverting #513592 looks like it'd add an extra 1/5 (1/5 of what I have not idea), and of course we'll be required to use the more expensive drupal_hash_64() here since md5() isn't cool enough for government bureaucrats.

However the file check already caused one fatal error at #605374: Dont add nonexisting files to the registry so I'd agree with trying to go back to some form of hashing if we can do so without making Drupal uninstallable.

Comment #3

pwolanin CreditAttribution: pwolanin commented 6 July 2010 at 03:03

Status:

Active

» Needs review

File	Size
819388-registry-file-hashes-3.patch	6.39 KB

Here's a first pass at it.

Comment #4

Anonymous (not verified) CreditAttribution: Anonymous commented 8 July 2010 at 03:27

Status:

Needs review

» Reviewed & tested by the community

looks good to me, very simple patch, and makes the logic a bit simpler. RTBC.

#2, we've fixed the install speed regression bug.

Comment #5

Dries CreditAttribution: Dries commented 10 July 2010 at 01:57

Project:	Drupal core	» HEAD to HEAD
Version:	7.x-dev	»
Component:	base system	» Code

Committed to CVS HEAD.

Moving this to the HEAD to HEAD project.

Comment #6

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 10 July 2010 at 02:43

Project:	HEAD to HEAD	» Drupal core
Version:		» 7.x-dev
Component:	Code	» base system
Status:	Reviewed & tested by the community	» Active

IMO it distorts the history quite a lot to take an issue which is a Drupal issue (fix something) and after it's run its course, to assign it to another project with a different purpose (fix what we broke by fixing the other thing).

Can we just open new Head2Head issues for these and leave these things closed?

Our issue queues are hard enough to understand without things hopping projects.

I'm taking the liberty of doing that, if you don't mind. If you disagree, I don't mind.

I took the liberty of creating #850070: Fix: Switch registry back to using hashes not file mtime and ctime

Comment #7

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 10 July 2010 at 02:43

Status:

Active

» Fixed

Comment #8

pwolanin CreditAttribution: pwolanin commented 10 July 2010 at 02:48

Thanks Dries.

I agree with rfay though - nodes are cheap - let's not move issues around. if you note the need, one of us will happily open a relevant head2head issue.