Posted by CitizenKane on January 1, 2009 at 6:18pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | file system |
| Category: | task |
| Priority: | normal |
| Assigned: | CitizenKane |
| Status: | closed (fixed) |
Issue Summary
Attached patch does the following things:
1. Removes FILE_STATUS_TEMPORARY. Because $file->status is a bitmapped field, having FILE_STATUS_TEMPORARY for the 0 value of this field doesn't make sense.
2. Fixes issues with the use of FILE_STATUS_PERMANENT to ensure that it is treated more uniformly as a binary flag.
3. Fixes a logic issue with $file->status in upload.module.
At present all tests still pass.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| remove-file_status_temp.patch | 5.74 KB | Idle | Unable to apply patch remove-file_status_temp.patch | View details |
Comments
#1
Glad you took this up. The whole temporary constant has bothered me for a while but I'd never gotten around to rolling a patch.
I cleaned up the file.inc docs to make it clear what FILE_STATUS_PERMANENT does since FILE_STATUS_TEMPORARY's comment is being removed.
I fiexed the spacing on
+ $file->status |= FILE_STATUS_PERMANENT;to line up with all the other assignments.I dropped the changes to system_cron() since the current code is fine.
#2
Good clean-up. Committed to CVS HEAD. Kudos to both.
#3
thanks, added the change to the update docs: http://drupal.org/node/224333#remove_FILE_STATUS_TEMPORARY
#4
I have no experience with bitmap flags (or whatever they're called), but removing FILE_STATUS_TEMPORARY seems very bad for readability from my point of view.
#5
it's less readable but the problem with it was that it didn't really do anything and encouraged people to set the status in a way that would preclude other bits from being used.
#6
Automatically closed -- issue fixed for two weeks with no activity.