First of all, why this test
if ($file == NULL)
two lines after declaring it NULL
static $file = NULL; ?

Secondly, this function makes some erronous assumptions about the locations of files. Specifically, I run multiple sites off of one drupal code base using Apache and Alias directives. Currently, _acidfree_lockfile() returns

'C:/drupal/ursimaging/C:\drupal\sites\webdev.ursimaging\files/acidfree/tmp'

Seems to me that $_SERVER['DOCUMENT_ROOT'] and $subdir are totally unnecessary for absolute paths, and in my case, returns incorrect data, anyway.

Seems like we should first get the file path, then test to see if it is absolute or relative. IFF (if and only if) it is relative do we attempt to construct an absolute FILE path. Then, we can construct the path to the lock file.

I'll be working on this and should have a patch shortly.

CommentFileSizeAuthor
#2 _acidfree_lockfile.patch1.47 KBhotgazpacho

Comments

hotgazpacho’s picture

hotgazpacho commented
Title: _acidfree_lockfile() problems » One more thing

variable_get('acidfree_dir', 'acidfree') will always return 'acidfree' because nowhere else in the module, most importantly in its settings, is any reference to 'acidfree_dir' made. I'd like to patch this too, but I am unclear on what the significance of 'acidfree_dir' is.

hotgazpacho’s picture

hotgazpacho commented
Title: One more thing » _acidfree_lockfile() problems
Status: Active » Needs review
StatusFileSize
new _acidfree_lockfile.patch1.47 KB

Here's the patch file. Also included are a fix to make the output XHTML compliant (double quotes instead of single quotes), and the ability to have only 1 column of thumbnails.

vhmauery’s picture

vhmauery commented

The $file variable was static, so the assignment is only executed the first time the function executes. So the null assignment and the null check is not bad. The idea was to store the $file variable so we didn't have to calculate it every time.

The patch looks pretty good. I am glad somebody using windows could help me out here. I haven't used windows since Win2k about 3 years ago, so I am kind of out of touch with how it all works. I took the patch and made some other changes that made sense, considering the changes you made and committed that.

Let me know if this fixes your problems.

hotgazpacho’s picture

hotgazpacho commented

Fixes my problem.

One question. How does declaring a variable static while simultaneously asigning it a null value make any sense? It will always be null, written as it is. You then add some extra opcode overhead by checking to see if it is null.

vhmauery’s picture

vhmauery commented

An assignment of a static variable at declaration time only gets executed once. 

function foo() {
    static $bar = 10;
    if ($bar-- == 0)
        $bar = 10;
    return $bar;
}

echo foo()."\n";
echo foo()."\n";

This will print out:

9
8

So setting it a variable null at declaration time and then checking to see if it is null every time after is just fine. The code inside the if statement only gets executed the first time. This saves some execution time if that function gets called more than once.

vhmauery’s picture

vhmauery commented
Status: Needs review » Closed (fixed)

Since this fixes the problem, I am closing this bug.