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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pwolanin’s picture

The menu router column is the same in Drupal 6, so wow - we suck.

nikgregory’s picture

FileSize
2.96 KB

Here is a patch that addresses menu_router.file and the associated update

pwolanin’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, menu_router.patch, failed testing.

pwolanin’s picture

Looks like a bogus fail (Poll module).

tyabut’s picture

Status: Needs work » Needs review

#2: menu_router.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, menu_router.patch, failed testing.

nikgregory’s picture

Status: Needs work » Needs review
FileSize
2.83 KB

here is the menu_router patch with less crufty goodness

nikgregory’s picture

Status: Needs review » Active
FileSize
11.71 KB

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)

pwolanin’s picture

Status: Active » Needs work
nikgregory’s picture

Status: Needs work » Needs review
FileSize
3.6 KB

fixed the failing poll test (i believe) form.inc makes an assumption on menu_router['file']

Status: Needs review » Needs work

The last submitted patch, menu_router.3.patch, failed testing.

nikgregory’s picture

Status: Needs work » Needs review

#11: menu_router.3.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, menu_router.3.patch, failed testing.

sun’s picture

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
18.1 KB

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.

sun’s picture

+++ 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!

catch’s picture

Status: Needs review » Needs work
bcn’s picture

Status: Needs work » Needs review
FileSize
17.81 KB

Re-roll to get this to apply cleanly to head...

Still doesn't address sun's second part of #17:

The key 'description' can be removed from this update.
andypost’s picture

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

andypost’s picture

+++ 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' ?

sun’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! Looks good to go.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD.

rfay’s picture

Status: Fixed » Active

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`;
andypost’s picture

@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

Damien Tournoud’s picture

Status: Active » Fixed

This falls into the "we don't support 7.x to 7.x updates until the first beta version" category. Back to fixed.

rfay’s picture

@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.

catch’s picture

@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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.