Posted by Damien Tournoud on March 13, 2010 at 5:20pm
12 followers
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | file.module |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
According to our own list of reserved words, FILE is a reserved word and we shouldn't be using it.
This is used in two places:
- the new {file} table
- the menu_router.file column
Comments
#1
The menu router column is the same in Drupal 6, so wow - we suck.
#2
Here is a patch that addresses menu_router.file and the associated update
#3
#4
The last submitted patch, menu_router.patch, failed testing.
#5
Looks like a bogus fail (Poll module).
#6
#2: menu_router.patch queued for re-testing.
#7
The last submitted patch, menu_router.patch, failed testing.
#8
here is the menu_router patch with less crufty goodness
#9
A start on the file table. file-> fileinfo.
This needs work, I cannot get the install to complete successfully after applying the patch, I get past the
site configuration screen and end up on an error screen with the following
Warning: array_keys(): The first argument should be an array in drupal_schema_fields_sql() (line 5679 of /Users/nik/sandbox/d7/drupal/includes/common.inc).
Recoverable fatal error: Argument 2 passed to SelectQuery::fields() must be an array, null given, called in /Users/nik/sandbox/d7/drupal/includes/entity.inc on line 210 and defined in SelectQuery->fields() (line 1124 of /Users/nik/sandbox/d7/drupal/includes/database/select.inc)
#10
#11
fixed the failing poll test (i believe) form.inc makes an assumption on menu_router['file']
#12
The last submitted patch, menu_router.3.patch, failed testing.
#13
#11: menu_router.3.patch queued for re-testing.
#14
The last submitted patch, menu_router.3.patch, failed testing.
#15
Regarding {file}, a more consistent name would be {file_managed}. See http://api.drupal.org/api/search/7/file_managed
+++ modules/system/system.install 18 Mar 2010 14:27:17 -0000@@ -983,13 +983,13 @@ function system_schema() {
- 'indexes' => array(
+ 'Indexes' => array(
Unintended change, I guess.
Powered by Dreditor.
#16
Ok, removed the case change and renamed file to file_managed. Installing does work, lets see what the testbot says.
What I'm not certain about is we should rename the entity type to filed_managed or just rename the base table. I just renamed the base table now, but renaming the entity would probably be better.
#17
+++ modules/system/system.install@@ -2390,6 +2390,16 @@ function system_update_7051() {
+function system_update_7052() {
+ db_change_field('menu_router', 'file', 'include_file', array(
+ 'description' => 'The file to include for this element, usually the page callback function lives in this file.',
+ 'type' => 'text',
+ 'size' => 'medium'
+ ));
+}
Wrong indentation + missing trailing comma in multi-line array here.
The key 'description' can be removed from this update.
+++ modules/user/user.install
@@ -192,7 +192,7 @@ function user_schema() {
- 'description' => "Foreign key: {file}.fid of user's picture.",
+ 'description' => "Foreign key: {file_managed}.fid of user's picture.",
@@ -472,7 +473,7 @@ function user_update_7004(&$sandbox) {
- 'description' => t("Foreign key: {file}.fid of user's picture."),
+ 'description' => t("Foreign key: {file_managed}.fid of user's picture."),
Interesting. Only seeing changes for 'description' keys in this patch probably means that we are missing some proper 'foreign keys' definitions for these tables (probably better to do in a separate issue, bonus points for creating it ;).
139 critical left. Go review some!
#18
#19
Re-roll to get this to apply cleanly to head...
Still doesn't address sun's second part of #17:
#20
Same patch with fixed system_update_7052()
+++ modules/system/system.install 1 Apr 2010 22:32:51 -0000@@ -2391,6 +2391,16 @@
+function system_update_7052() {
+ db_change_field('menu_router', 'file', 'include_file', array(
+ 'description' => 'The file to include for this element, usually the page callback function lives in this file.',
+ 'type' => 'text',
+ 'size' => 'medium',
+ ));
Description is useless when changing field
+++ modules/user/user.install 1 Apr 2010 22:32:52 -0000@@ -201,7 +201,7 @@
- 'description' => "Foreign key: {file}.fid of user's picture.",
+ 'description' => "Foreign key: {file_managed}.fid of user's picture.",
{users}.picture should be different issue, because {file}.uid already have foreign key to {users}.uid
So we could get circular reference
#21
+++ modules/system/system.install 5 Apr 2010 00:05:36 -0000@@ -2391,6 +2391,12 @@ function system_update_7051() {
+ * Rename file to include_file in menu_router
Fixed doc-block to conform standard.
EDIT: should we allow FieldAPI to create field with name 'file' ?
#22
Thanks! Looks good to go.
#23
Committed to CVS HEAD.
#24
On all (I think) of my sites the hook_update_N() has failed to create the file_managed table. I haven't caught the errors exactly, but it leaves the site in a half-functional state, with PDO errors on every page load.
The resultant error is predictably (here to help search engine):
* Warning: Cannot modify header information - headers already sent by (output started at /home/randyfay/d7.drupalexamples.info/includes/common.inc:2426) in drupal_send_headers() (line 1007 of /home/randyfay/d7.drupalexamples.info/includes/bootstrap.inc).* PDOException: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'examplesd7.file_managed' doesn't exist: SELECT fid FROM {file_managed} WHERE status & :permanent1 <> :permanent2 AND timestamp < :timestamp; Array ( [:permanent1] => 1 [:permanent2] => 1 [:timestamp] => 1270974763 ) in system_cron() (line 2789 of /home/randyfay/d7.drupalexamples.info/modules/system/system.module).
A workaround that seems to get a site sane again is:
ALTER TABLE `d7git`.`file` RENAME TO `d7git`.`file_managed`;#25
@rfay do you upgrading from D6? if not so this hook should not be run. I've tested this on clean install (today's HEAD) and it works without any warnings
#26
This falls into the "we don't support 7.x to 7.x updates until the first beta version" category. Back to fixed.
#27
@damien, #26: that's fine... But then why implement hook_update_N() with an update that doesn't work? Or doesn't work most of the time?
@andypost, #25: No, of course not, D6 upgrades are still impossible (or they were last week). This is a simple update.php on an existing D7 site after updating the code.
#28
@rfay, there's no upgrade for file->file_managed that would run on HEAD - the patch only changes the schema and updates an older D7 update to use the correct name. The new update is for the {menu_router}.file column, which worked fine on my HEAD install.
#29
Automatically closed -- issue fixed for 2 weeks with no activity.