Uncovered while working on #930122: Regression: temp directory handling broken by confusion between file_directory_temp and file_temporary_path...

There's nothing in D7 that migrates your D6 temp directory setting (that lives in a variable called 'file_directory_temp') to the D7 equivalent (called 'file_temporary_path').

CommentFileSizeAuthor
#16 931736-16.file_directory_temp.patch955 bytesdww
#12 931736-12.file_directory_temp.patch4.05 KBdww
#8 931736-4.file_directory_temp.patch3.61 KBdww
#4 931736-4.file_directory_temp.patch3.61 KBdww
#3 931736-3.file_directory_temp.test-only.patch2.47 KBdww

Comments

dww’s picture

dww
we/he/they
commented
Title: No upgrade path for the D6 'file_directory_temp' setting » No upgrade path for the D6 file directory settings

Actually, it's not just the temp dir setting. There's also 'file_directory_path'. So, we need:

A) Migrate 'file_directory_temp' to 'file_temporary_path'. Delete 'file_directory_temp'

B) Based on 'file_downloads' migrate 'file_directory_path' to 'file_public_path' or 'file_private_path' as appropriate. Delete 'file_directory_path'.

C) Migrate 'file_downloads' to 'file_default_scheme'. Delete 'file_downloads'.

dww’s picture

dww
we/he/they
commented
Title: No upgrade path for the D6 file directory settings » No upgrade path for the D6 'file_directory_temp' setting.
Assigned: Unassigned » dww

Nope, I was right the first time. ;) system_update_7034() handles B and C. I'll just add A to that. One sec, I'll roll a patch.

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 931736-3.file_directory_temp.test-only.patch2.47 KB

Here's a test for the bug. This should have 2 failures for these two assertions:

  $this->assertNull(variable_get('file_directory_temp', NULL), "The 'file_directory_temp' variable was properly removed.");
  $this->assertEqual(variable_get('file_temporary_path', 'drupal-7-bogus'), $d6_file_directory_temp, "The 'file_temporary_path' setting was properly migrated.");

Patch with the same test and a fix coming up next. Just want to demo that the test finds the bug currently in HEAD.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 931736-4.file_directory_temp.patch3.61 KB

This one *should* pass, except for the bug at #930122: Regression: temp directory handling broken by confusion between file_directory_temp and file_temporary_path. ;) B/c of that, the 'file_directory_temp' variable is automatically set to the system default (e.g. /tmp) during the upgrade process, so we can't actually see that the upgrade path removed the bogus variable.

Once #930122 lands, this patch should pass all tests.

Status: Needs review » Needs work
Issue tags: -D7 upgrade path

The last submitted patch, 931736-4.file_directory_temp.patch, failed testing.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
Issue tags: +D7 upgrade path

#4: 931736-4.file_directory_temp.patch queued for re-testing.

dww’s picture

dww
we/he/they
commented

retesting now that #930122: Regression: temp directory handling broken by confusion between file_directory_temp and file_temporary_path is in...

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 931736-4.file_directory_temp.patch3.61 KB

Damn bot. http://qa.drupal.org/pifr/test/95039 says the patch passed all tests, but it's not updating the issue. :(
Here's #4 again in the vain hope that the bot actually shows us green in here.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

Drupal 7 bogus, eh? Nice test, good fix.

tom_o_t’s picture

tom_o_t commented

#8: 931736-4.file_directory_temp.patch queued for re-testing.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Needs work

We need to pull this into a separate update function now that we support HEAD-to-HEAD upgrades.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 931736-12.file_directory_temp.patch4.05 KB
tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented

please remove

debug(variable_get('file_directory_temp', NULL));

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Needs review » Fixed

Thanks. Committed to HEAD, minus the debug() :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Fixed » Needs work

This pretty much overwrites whatever freshly installed Drupal 7 sites had in their temporary directory setup. Drupal was supposed to provide update functions "like a stable version" since it became beta, so I think it would have been better to form this update as:

$d7_temp_path = variable_get('file_temporary_path', NULL);
$d6_temp_path = variable_get('file_directory_temp', NULL);
if (empty($d7_temp_path) && !empty($d6_temp_path)) {
  variable_set('file_temporary_path', $d6_temp_path);
  variable_del('file_directory_temp');
}

Otherwise as the current code stands, it overwrites the existing D7 setting with a default even if the D6 setting was never there. I think this needs a fix.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 931736-16.file_directory_temp.patch955 bytes

Yeah, I guess that's a good move. Except we always want to delete the dead variable.

Status: Needs review » Needs work

The last submitted patch, 931736-16.file_directory_temp.patch, failed testing.

dww’s picture

dww
we/he/they
commented
Assigned: dww » Unassigned

Ahh right, hurray for the test I wrote. I believe the proposal in #15 will fail because by the time we're hitting this code, the D7 variable is already set due to the slightly wonky way it's being initialized in the code. So, there's always going to be a D7 value (even if it wasn't set by a site admin), which means that what Gabor proposes in #15 is (I believe) doomed. We'll never migrate the D6 value that way.

It might be possible to duplicate the code that sets the default value, and compare the D7 variable to what we think is the default, and only clobber the D7 value if:

- The D6 value exists
- The D7 value is the same as the default

But even that is questionable...

Anyway, I have no time for this now. If someone else wants to take a stab, knock yourselves out. ;)

p.s. This update already shipped in 7.0-rc1. So the people Gabor is concerned about, the HEAD 2 HEAD folks, have probably already been nailed by it. I'd strongly suggest moving this back to fixed and being done with it.

  • webchick committed fe7e01c on 8.3.x 
    #931736 by dww: Fixed No upgrade path for the D6 'file_directory_temp'...

  • webchick committed fe7e01c on 8.3.x 
    #931736 by dww: Fixed No upgrade path for the D6 'file_directory_temp'...

  • webchick committed fe7e01c on 8.4.x 
    #931736 by dww: Fixed No upgrade path for the D6 'file_directory_temp'...

  • webchick committed fe7e01c on 8.4.x 
    #931736 by dww: Fixed No upgrade path for the D6 'file_directory_temp'...

  • webchick committed fe7e01c on 9.1.x 
    #931736 by dww: Fixed No upgrade path for the D6 'file_directory_temp'...

Status: Needs work » Closed (outdated)

Automatically closed because Drupal 7 security and bugfix support has ended as of 5 January 2025. If the issue verifiably applies to later versions, please reopen with details and update the version.