Make the file entity a classed object

File entities are actually owned by system module.

Not sure if it should be part of this patch, but it would be great if we can get language supported for files as well. Same as for nodes, so we can just copy that code hopefully :) That would unify this part of the code / behavior.

Let's please not *add* things as part of this conversion.

Understood.

This would be a good time to move the file entity out of the system module, to the file module where it belongs.
File entity in system.module has always been a WTF to me.

Well the logic actually lives in includes/file.inc and system module and not in the file.module. This would make file.module a required core module which would be a functional change. These are all good followups but out of scope for this issue.

Will give it a shot later today.

Recategorizing per #1346204-11: [meta] Drupal 8 Entity API improvements.

Sorry guys, contrib ate all of my time this weekend.
Unassigning myself in case someone gets to this before I have time again.

Working on this one...

A first attempt... I haven't worked with the new Entity pattern yet and thought files were the easiest one to start with.. They aren't exactly easy either :)

- Added File class
- Ported the save/delete code to FileStorageController
- Changed existing calls and code to the new patttern
- There are some API changes which can't be avoided. E.g. file_save() returned the changed object, file_delete() with all these checks and the $force argument.. see below

This wont' pass all test nor is it complete, I'm just trying to get an overview of how many tests fails there still are, takes quite some time to run even just the File tests locally ;)

Open issues/Problems:
- Saving File objects with variable_set() leads to fatal errors on the following request because the __wakeUp() is called during variable_initalize(), which calls setUp(), which calls entity_get_info()... and that one is not yet available at that time.
- There is also something not yet working in general with file uploads, probably something with validation..
- file_delete() currently returns different stuff but the entity delete pattern doesn't support this. Like returning $references which evaluates to TRUE. Additionally, there is the $force argument, which we IMHO need to drop. I am not exactly sure how yet, I am thinking of the following:
You shouldn't simply call file_delete() which will then maybe delete the file and maybe not..
Instead, modules IMHO need to check if there are remaining references to the file and only delete
it if not. We could wrap this logic inside a file_delete_if_unused() helper function or so. The $force argument should probably be removed completely and instead there should be a possibility to delete all references with a separate api function. Eventually the cases that currently result in the file not being deleted should be converted to Exceptions.

Thoughts/Ideas?

Saving File objects with variable_set() leads to fatal errors on the following request because the __wakeUp() is called during variable_initalize(), which calls setUp(), which calls entity_get_info()... and that one is not yet available at that time.

Where does this happen in core currently?

I would be happy to remove logic from file_delete() about checking references but we can't change that logic here in a "conversion" patch.

Yeah, taxonomy_term_delete() doesn't go and try to check for references to the term before allowing you to actually delete the term.

Similarly, node_delete() doesn't check for noderef or entityref fields that might reference a node before deleting it - and that's not only because those field types are not in core and thus cannot enforce that; I don't think we'd want to go that way even if they were in core and provided by node.module or entity.module.

Delete is delete. A mechanism for "delete only if not referenced", if desired, needs to live on a higher level.

@Dave

The variable_set() of $file objects currently only happens in the file_test.module, from what I've seen. I've changed that to only store the $file->fid, not sure if I got all of them yet, though. But it also happens for $user objects, for example because Simpletest is saving all sent mails including their context in a variable. And the context for user registration mails and the like contain the user object as context information :)

I did hack around that in the user.module patch by making the entity_get_info() call conditional. That prevents any errors but things will fall apart if you try to save or do another action that requires the entity type information. One way around that would to wrap it inside a protected getEntityInfo() method which only loads the necessary information on demand.

Note sure about the "we can't change that in a conversion patch" part. The thing is, there is no way to keep it like it was because entity_delete is not designed to return anything and there are quite a few other API changes. For example file_save() not returning the final object anymore or that file_delete currently expects the $file object as argument and not $fid. So, why not change this? Because the only other option is to stop deleting silently, without any indication to the caller of file_delete().

We should probably postpone this issue until #1401558: Remove the usage handling logic from file_delete() is fixed then.

Ok, getting this back on track.

Changes:

- PSR-0'ified (in namespace Drupal\Core\File). This was actually necessary because the file class lives in /includes. At least it will be necessary once #1320650: Stop scanning /includes with the registry
- Fixed file_save_upload() to stop checking for the return value of file_save().
- Reverted changes in file_test.module, that variable stuff simply works now thanks to the entity info removal.
- Bunch of wrong tests fixed and (object) conversion in file.module replaced with file_load(). This is statically cached so it's cleaner than entity_create() and probably faster as well. And certainly better for memory usage.
- Started with type hints, just a beginning and tons of @params need to be updated and so on.

Still needs the referenced issue above (so please please review and comment there!) to be fixed first, but this should actually pass most tests. Yay for #1512564: Remove entity info from the Entity classes.

#22: much_better_now.patch queued for re-testing.

retesting because the referenced patch was committed.

The entity system is now a contrib module, therefore core (/includes and /lib) should define no entities. That would create a circular dependency. Vis, File entities should live in the file module, not core.

A *contrib* module? user.module has hard dependency on entity.module, it's not like you can disable it or something. :)

Right now, file.module is about a file *field*. Moving file entity into a file.module would add a dependency from user.module to that, and user is referenced in /includes all over the place. Back to status quo. Simply more indirection.

Moving into system.module is just as pointless imho.

So all those possible changes don't really change the fact that the file entity is pretty hard-wired. And I feel that moving file entity into a (current or new) file.module, including all those functions that work with a $file object in file.inc, which is most of them, untangling the cross-dependencies between user and file (which would e.g involve replacing user->picture with a field that is added in standard.profile) are way outside of the purpose of this issue.

This issue does not introduce the dependency on entity.module, it just makes it a bit more obvious. We already have entity_load() calls in file.inc. I would really argue for getting this in first and then try to untangle includes/lib/modules. This allows to unblock further improvements on the entity side as well.

I agree with Berdir on this, the weird cross dependencies with the file object already exist, so we don't need to fix them here. But it'd be good to open a follow-up since this is quite confusing at the moment - for example system module defines the file entity too.

Er, not contrib module. Core module. That's what I get for posting after midnight. :-) Either way the circular dependency problem still stands.

I can deal with a follow-up issue if catch posts one. (I don't see a link yet.) But we have to be really careful to not continue to make these circular assumptions that always come back to bite us.

I wouldn't let this in if it introduced the circular dependency, but since it doesn't it seems a shame to hold it up. #1468328: Move file entity info, managed file, and file usage functionality into File module is already open, and while we might want to move things around to other places, seems like a good place to discuss this.

OK. Following that other issue, and I withdraw my objections from #26.

Ok, let's see how this one goes.

I've fixed the last test failures and added tons of type hints and documentation updates (class and object -> entity). I might have gone too far in some instances, for example by adding it to gettext.inc. We could easily define that those do not need file entities but simple file objects with a number of properties defined, like file objects returned by file_scan_directory().

Unless I've introduced new failures with those type hints, this one should pass.

Ok, two more object casts replaced with entity_create().

1)

+++ b/core/lib/Drupal/Core/File/File.phpundefined
@@ -0,0 +1,94 @@
+  /**
+   * The comment language code.
+   *
+   * @var string
+   */
+  public $langcode = LANGUAGE_NOT_SPECIFIED;

Comment langcode? And isn't this part of the extended entity alrdy?

2)
I though the namespace suff should be Drupal\Core\file\File and not Drupal\Core\File\File

1) That is obviously wrong :)

2) Note the "Core" part in there. This is currently a core component so it has to live in lib/Drupal/Core. That's why it's File and not file. There is a follow-up issue to move all the managed file handling to file.module.

Updated issue summary.

Thank you. The patch looks pretty clean. When reviewing it I only found these nitpicky points (and questions):

1. +++ b/core/includes/file.incundefined
   @@ -855,7 +817,7 @@ function file_copy(stdClass $source, $destination = NULL, $replace = FILE_EXISTS
   -    $file = file_save($file);
   +    file_save($file);
   

   In the other make X a classed entity patches, this would be $file->save(). In case we do that, there are more instances to convert.

2. +++ b/core/includes/file.incundefined
   @@ -1638,7 +1573,8 @@ function file_save_upload($source, $validators = array(), $destination = FALSE,
   -  if ($file = file_save($file)) {
   +  file_save($file);
   +  if ($file) {
   

   file_save() is now equivalent to $file->save(). So it makes no sense to check if $file is true-ish afterwards, anymore.

3. +++ b/core/lib/Drupal/Core/File/File.phpundefined
   @@ -0,0 +1,94 @@
   +/**
   + * Defines the comment entity class.
   + */
   +class File extends Entity {
   

   Stale copied code :)

4. +++ b/core/lib/Drupal/Core/File/File.phpundefined
   @@ -0,0 +1,94 @@
   +   * The comment language code.
   +   *
   +   * @var string
   +   */
   +  public $langcode = LANGUAGE_NOT_SPECIFIED;
   

   Is this really about comments or copy&pasted, too? (Is that what is "obviously wrong", or is the objection wrong?)

5. +++ b/core/lib/Drupal/Core/File/File.phpundefined
   @@ -0,0 +1,94 @@
   +  }
   +
   

   Maybe remove the empty line.

6. +++ b/core/modules/file/file.moduleundefined
   @@ -346,7 +348,7 @@ function file_progress_implementation() {
      // TODO: Remove references to a file that is in-use.
   

   Is this TODO required for the patch, or a follow-up?

   That TODO is from the context.

This is the last patch remaining for us to be able to close critical task #1368394: Convert all core entities to classed objects

I increased the priotity of #1401558: Remove the usage handling logic from file_delete(), which is blocking this issue.

Promoting to critical task, per #1368394-8: Convert all core entities to classed objects.

Ok, re-rolled based on #1401558: Remove the usage handling logic from file_delete(), includes those changes as well for the moment to get this moving again.

Removed file_save(), as it has changed anyway, fixed the docblocks in File.php, used full namespace in file hook documentation.

+++ b/core/lib/Drupal/Core/File/FileStorageController.phpundefined
@@ -0,0 +1,51 @@
+   * Overrides EntityDatabaseStorageController::presave().

This need to contain the full path

+++ b/core/modules/user/user.entity.incundefined
@@ -0,0 +1,52 @@
+ * This extends the DrupalDefaultEntityController class, adding required

I think this needs a full path

+++ b/core/modules/user/user.entity.incundefined
@@ -0,0 +1,52 @@
+class UserController extends DrupalDefaultEntityController {

And why isn't this in the user namespace?

9 days to next Drupal core point release.

Fixed the entity references and removed user.entity.inc, that was a merge left-over.

Uh, this is not good. We're getting the following error here

Fatal error: Class 'Drupal\entity\Entity' not found in /home/berdir/Projekte/d8/core/lib/Drupal/Core/File/File.php on line 19

Backtrace:

#0  require() called at [/home/berdir/Projekte/d8/core/vendor/Symfony/Component/ClassLoader/UniversalClassLoader.php:249]
#1  Symfony\Component\ClassLoader\UniversalClassLoader->loadClass()
#2  spl_autoload_call()
#3  unserialize()
#4  array_map() called at [/home/berdir/Projekte/d8/core/includes/bootstrap.inc:841]
#5  variable_initialize() called at [/home/berdir/Projekte/d8/core/includes/bootstrap.inc:2293]
#6  _drupal_bootstrap_variables() called at [/home/berdir/Projekte/d8/core/includes/bootstrap.inc:2067]
#7  drupal_bootstrap() called at [/home/berdir/Projekte/d8/core/includes/bootstrap.inc:2191]
#8  _drupal_bootstrap_page_cache() called at [/home/berdir/Projekte/d8/core/includes/bootstrap.inc:2059]
#9  drupal_bootstrap() called at [/home/berdir/Projekte/d8/index.php:20]

So it fails because it has a File object in the variables cache and then loads the definition when unserializing it, before the module namespaces have been registered. I'm not sure if this is a page cache specific problem now, because otherwise, we should have seen it already when doing the entity psr-0 conversion due to similar user tests (mail related). Weird. And very much not good.

Once we'd move the file entity to file.module, it would already fail when trying to load the File because it would be in a file then as well.

I don't think we're storing an actual file object in a variable somewhere in regular runtime code.

However, as usual, tests are doing unholy things, so I wouldn't be surprised if a test or test support module takes a file object and saves it as a variable (in order to read that variable from within the test).

E.g., modules/field/tests/field_test.storage.inc looks suspicious.

Yes, we do this, this was also one of the reasons to remove the entity_get_info() calls from the Entity class. It looks like we re-introduced the problem now.

Another case where we are storing entities in variables is Drupal\Core\Mail\VariableLog, when sending mails which have a user object in the mail context.

Arggh that is full of evil. #1175054: Add a storage (API) for persistent non-configuration state would get around this - we'd abuse that and load individual keys instead of the early bootstrap variable_init(), but still nasty and that's nowhere near ready yet.

For VariableLog maybe we can add a file log and use that instead?

For the tests that are putting entities in variables and reading them out again, I don't see a way around this except refactoring them to not use variables for this.

Yeah, given that variable_*() is on it's way out, that's actually not much of a problem.

The attached patch only saves the file id, it's possible that there is a test that will now fails in case those are checked somewhere but the tests that failed above should now pass.

Weird double post.

+++ b/core/includes/file.inc
@@ -6,6 +6,7 @@
+use Drupal\Core\File\File;

I think that namespace is somewhat strange. To be inline with our other entities I would have expected Drupal\system\File. Since you need to implement hooks and have a schema to take part in the entity system I think it makes sense to have this in a module namespace.

I guess this is because #1468328: Move file entity info, managed file, and file usage functionality into File module will move it to Drupal\file\File anyway?!

Exactly, that will happen over there. Both Drupal\Core and Drupal\system are wrong because that implies a dependency on entity.module in system.module. So it doesn't matter. That system.module defines hooks and hook documentation for things invoked in /includes is nothing new.

Yes, I guess that's true, that it isn't making this worse than now.

+++ b/core/includes/file.incundefined
@@ -574,7 +575,7 @@ function file_save_htaccess($directory, $private = TRUE) {
+ * Loads File entities from the database.

File with capital

+++ b/core/includes/file.incundefined
@@ -586,7 +587,7 @@ function file_save_htaccess($directory, $private = TRUE) {
+ *   An array of File entities, indexed by fid.

Capital

+++ b/core/includes/file.incundefined
@@ -600,13 +601,13 @@ function file_load_multiple($fids = array(), array $conditions = array()) {
+ * Loads a single file entity from the database.

no capital

==> in those cases we are not using capitals, grep for "node entity" for examples.

+++ b/core/includes/file.incundefined
@@ -1295,30 +1245,31 @@ function file_create_filename($basename, $directory) {
+ * Delete files.

Deletes

+++ b/core/includes/file.incundefined
@@ -1295,30 +1245,31 @@ function file_create_filename($basename, $directory) {
+ * file_usage_list() will be called to
+ * determine if the file is being used by any modules. If the file is being
+ * used the delete will be canceled.

Why is the spread over 3 lines. We didn't reach 80 chars on the first line.

+++ b/core/includes/file.incundefined
@@ -1810,8 +1759,8 @@ function file_validate_is_image(stdClass $file) {
+ *   A file entity. This function may resize the file affecting its
  *   size.

Can be written on 1 line now.

I tried hard but I couldn't find any other problems...

8 days to next Drupal core point release.

Thanks.

- Fixed uppercase File
- That docblock was outdated, replaced it with the same one that's on file_delete().
- Moved the last one to a single line.

+++ b/core/includes/file.inc
@@ -1498,15 +1449,15 @@ function file_save_upload($source, $validators = array(), $destination = FALSE,
   $file->filemime = file_get_mimetype($file->filename);

Is there a reason this isn't passed into entity_create() like the rest of the info?

+++ b/core/modules/file/tests/file.test
@@ -41,7 +41,7 @@ class FileFieldTestCase extends WebTestBase {
+    return entity_create('file', (array)$file);

+++ b/core/modules/image/image.test
@@ -1290,8 +1290,8 @@ class ImageFieldDefaultImagesTestCase extends ImageFieldTestCase {
+      $file = entity_create('file', (array)array_pop($files));

+++ b/core/modules/system/tests/file.test
@@ -238,11 +239,11 @@ class FileTestCase extends WebTestBase {
+    return entity_create('file', (array)$file);

@@ -612,7 +610,7 @@ class FileSaveUploadTest extends FileHookTestCase {
+    $this->image = entity_create('file', (array)current($image_files));

+++ b/core/modules/system/tests/image.test
@@ -486,7 +486,7 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
+    $file = entity_create('file', (array)current($this->drupalGetTestFiles('image')));

There should be a space after the cast.

+++ b/core/modules/system/tests/file.test
@@ -238,11 +239,11 @@ class FileTestCase extends WebTestBase {
+    // Write the record directly rather than using the API so we don't/ invoke
+    // the hooks.

Wraps a bit early I think, but more importantly there is a stray "/".

+++ b/core/modules/system/tests/file.test
@@ -388,11 +389,11 @@ class FileValidatorTest extends WebTestBase {
+    $this->image = entity_create('file', array());
     $this->image->uri = 'core/misc/druplicon.png';
     $this->image->filename = drupal_basename($this->image->uri);
...
+    $this->non_image = entity_create('file', array());
     $this->non_image->uri = 'core/misc/jquery.js';
     $this->non_image->filename = drupal_basename($this->non_image->uri);

@@ -471,8 +471,8 @@ class FileValidatorTest extends WebTestBase {
+    $file = entity_create('file', array());
 
     // Add a filename with an allowed length and test it.
     $file->filename = str_repeat('x', 240);

It seems this could use entity_create() as well. I guess that's a matter of taste, though.

> file_get_mimetype() access $file->filename, which is relatively complex operation with multiple function calls. we'd need to repeat that.

Will fix the other stuff later.

- The first two entity_create()'s could only have the first property as an array argument because the second again depends on the first. Could be hardcoded, sure, but I actually quite like it like that. The third has three blocks that each sets a separate filename and does some validation on it. Makes sense to keep that as well IMHO.

Fixed the missing spaces the and removed the "\" in the comment, the length is actually ok, just looks wide in dreditor.

Reroll

for $^é# sake, what does that comment part does in there

Maybe this one...

Looks awesome. Marking RTBC. Bot will mark it needs work if it fails.

+++ b/core/includes/file.inc
@@ -1498,15 +1449,15 @@ function file_save_upload($source, $validators = array(), $destination = FALSE,
+  // Begin building file entity.
+  $file = entity_create('file', array(
...
   $file->filemime = file_get_mimetype($file->filename);

File::$filemime is declared as property on File, so I do not understand why it is kept separate here.

Also, should we open a follow-up issue to add a ->get/setMimeType() method to the File class?

+++ b/core/includes/file.inc
@@ -1898,8 +1846,8 @@ function file_save_data($data, $destination = NULL, $replace = FILE_EXISTS_RENAM
+    // Create a file entity.
+    $file = entity_create('file', array());
     $file->fid = NULL;
     $file->uri = $uri;
     $file->filename = drupal_basename($uri);

Looks like this code could be cleaned up in a follow-up patch to properly leverage entity_create().

+++ b/core/lib/Drupal/Core/File/File.php
@@ -0,0 +1,94 @@
+use Drupal\entity\Entity;

+++ b/core/lib/Drupal/Core/File/FileStorageController.php
@@ -0,0 +1,51 @@
+use Drupal\entity\EntityDatabaseStorageController;
+use Drupal\entity\EntityInterface;

+++ b/core/modules/system/system.module
@@ -269,6 +269,8 @@ function system_entity_info() {
     'file' => array(
...
+      'controller class' => 'Drupal\Core\File\FileStorageController',
+      'entity class' => 'Drupal\Core\File\File',

This effectively adds a dependency on Entity module. To System module.

That's fine if we choose so, but I'd prefer that to be explicit; i.e., dependencies[] = entity in system.info.

+++ b/core/lib/Drupal/Core/File/FileStorageController.php
@@ -0,0 +1,51 @@
+class FileStorageController extends EntityDatabaseStorageController {
...
+  /**
+   * Overrides Drupal\entity\EntityDatabaseStorageController::delete().
+   *
+   * file_usage_list() is called to determine if the file is being used by any
+   * modules. If the file is being used the delete will be canceled.
+   */
+  public function delete($ids) {
...
+      if (file_unmanaged_delete($file->uri)) {

I don't see where file_usage_list() is called in this delete() method.

Did you intend to call file_usage_delete() here?

+++ b/core/modules/file/tests/file.test
@@ -41,7 +41,7 @@ class FileFieldTestCase extends WebTestBase {
     // Add a filesize property to files as would be read by file_load().
     $file->filesize = filesize($file->uri);
 
-    return $file;
+    return entity_create('file', (array) $file);

+++ b/core/modules/image/image.test
@@ -1290,8 +1290,8 @@ class ImageFieldDefaultImagesTestCase extends ImageFieldTestCase {
     $files = $this->drupalGetTestFiles('image');
     $default_images = array();
     foreach (array('field', 'instance', 'instance2', 'field_new', 'instance_new') as $image_target) {
-      $file = array_pop($files);
-      $file = file_save($file);
+      $file = entity_create('file', (array) array_pop($files));
+      $file->save();

+++ b/core/modules/system/tests/image.test
@@ -486,7 +486,7 @@ class ImageFileMoveTest extends ImageToolkitTestCase {
-    $file = current($this->drupalGetTestFiles('image'));
+    $file = entity_create('file', (array) current($this->drupalGetTestFiles('image')));

1) It looks like we're converting from object to array to File object here?

2) Shouldn't WebTestBase::drupalGetTestFiles() return File objects?

+++ b/core/modules/system/system.api.php
@@ -6,6 +6,7 @@
+use Drupal\Core\File\File;

@@ -2369,15 +2370,15 @@ function hook_file_load($files) {
+ * @param Drupal\Core\File\File $file
...
+function hook_file_validate(Drupal\Core\File\File $file) {

@@ -2397,12 +2398,10 @@ function hook_file_validate($file) {
+function hook_file_presave(Drupal\Core\File\File $file) {

@@ -2414,12 +2413,10 @@ function hook_file_presave($file) {
+function hook_file_insert(Drupal\Core\File\File $file) {

@@ -2430,59 +2427,57 @@ function hook_file_insert($file) {
+function hook_file_update(Drupal\Core\File\File $file) {
...
+function hook_file_copy(Drupal\Core\File\File $file, Drupal\Core\File\File $source) {
...
+function hook_file_move(Drupal\Core\File\File $file, Drupal\Core\File\File $source) {
...
+function hook_file_predelete(Drupal\Core\File\File $file) {

@@ -2490,16 +2485,16 @@ function hook_file_predelete($file) {
+function hook_file_delete(Drupal\Core\File\File $file) {

The PHP file imports the File namespace/class already, so it's sufficient that the phpDoc contains the fully qualified namespace already. These API docs function arguments should use just "File" as type hint. (which also simplifies copying of example hook implementations)

+++ b/core/modules/system/tests/file.test
@@ -2006,73 +2004,72 @@ class FileLoadTest extends FileHookTestCase {
     // Save it, inserting a new record.
-    $saved_file = file_save($file);
+    $file->save();
...
-    $this->assertNotNull($saved_file, t("Saving the file should give us back a file object."), 'File');
...
+    $this->assertNotNull($file, t("Saving the file should give us back a file entity."), 'File');
...
-    $resaved_file = file_save($saved_file);
...
+    $file->save();

$file is never ever NULL.

You need to clone $file into $saved_file before doing $saved_file->save().

This will allow you to revert most of the test changes here.

+++ b/core/modules/system/tests/file.test
@@ -2006,73 +2004,72 @@ class FileLoadTest extends FileHookTestCase {
-    $loaded_file = db_query('SELECT * FROM {file_managed} f WHERE f.fid = :fid', array(':fid' => $saved_file->fid))->fetch(PDO::FETCH_OBJ);
...
+    $loaded_file = db_query('SELECT * FROM {file_managed} f WHERE f.fid = :fid', array(':fid' => $file->fid))->fetch(PDO::FETCH_OBJ);
...
+    $loaded_file = db_query('SELECT * FROM {file_managed} f WHERE f.fid = :fid', array(':fid' => $file->fid))->fetch(PDO::FETCH_OBJ);

If I'm not mistaken, then this query needs to be replaced with either file_load(), or alternatively, use ->fetchObject('Drupal\Core\File\File') instead of ->fetch().

This effectively adds a dependency on Entity module. To System module.

That's fine if we choose so, but I'd prefer that to be explicit; i.e., dependencies[] = entity in system.info.

No let's not do that. System module already depends on user module afaik, and we have no explicit dependency there. I'm not against trying to do this if we want, but this isn't the issue to start - we can open another one then gape in awe at the cross-dependencies there.

- As mentioned in #58, $file->mimetype depends on $file->filename, which itself is dynamic. Changing that would require temporary variables or copy & paste the line above. The attached patch contains a slightly different approach that automatically sets filename and filemime if not set in FileStorageController, what do you think?

- Cleaned up and simplified the other call thanks to above changes.

- As discussed, the dependency already exists. We already call entity_load() in file_load(). This will be tackled in #1468328: Move file entity info, managed file, and file usage functionality into File module.

- Removed the left-over file_usage_delete() comment.

- Re the back and forth conversion. Yes, it might make sense to return File entities there. However, this method is called 30+ times and all these calls would need to be checked and possibly updated. Not sure if we want to do this here or in a follow-up. Also, the results in there are generated with file_scan_directory() which means that we can't get around the double conversion.

- api.php: Yes, I know, but our current coding standards mandate the use of fully qualified names in hook documentations according to http://drupal.org/node/1353118, exactly to simplify copy and paste. I don't really agree with it, but that's another story.

- Removed the unecessary NotNull() assertion. cloning makes no sense IMHO, $saved_file was only there because file_save() previously returned the saved object instead of changing it by reference. This is no longer the case.

- I think the direct db_query()'s there are to avoid messing with the hooks and confuse the related assertions. Changed to fetchObject() as suggested.

Let's see if the testbot likes the changes.

The create function obviously needs to be public. Thanks testbot ;)

And now with 100% more return statements.

Ok, this should pass again. Ignore the patches in #67 - #71, this one should be good.

- Fixed another typo in the create function.
- Removed the fetchObject() into the typed class, doesn't work because of the required argument. Just fetchObject() now.

The interdiff is against #63.

Thanks!

+++ b/core/modules/file/file.field.incundefined
@@ -926,7 +926,7 @@ function file_field_formatter_view($entity_type, $entity, $field, $instance, $la
+          '#file' => file_load($item['fid']),

I believe this change will actually prevent any kind of alt or description text from file or image fields from being used at all. This information would be lost with a file_load(). If you inspect $item it contains not only the file object but also keys 'display' and 'description' for file fields, and 'alt', 'title', 'height', 'width' for image fields.

+++ b/core/modules/file/file.field.incundefined
@@ -965,7 +965,7 @@ function theme_file_formatter_table($variables) {
+      theme('file_link', array('file' => file_load($item['fid']))),

Same here.

+++ b/core/modules/file/tests/file.testundefined
@@ -809,7 +809,7 @@ class FileFieldDisplayTestCase extends FileFieldTestCase {
+    $node_file = file_load($node->{$field_name}[LANGUAGE_NOT_SPECIFIED][0]['fid']);

Same here.

I'm also a little confused that we're converting everywhere we build a stdClass object into a File object, but we're not converting somethings like file_scan_directory() to return an array of true File entities rather than regular objects.

Related:
#1446464: Get rid of file_field_load() - shouldn't load all file object data with field load - only file ID
#1448124: Image dimensions should be available from file_load() for images, and not stored in field data

This also means we don't have any tests to confirm the alternate/title text behavior in file and image field widgets.

Ok, reverted those changes and instead making sure that we're passing valid file entity to theme_file_icon(), which is the only one that requires it. To be revisited at a later stage.

I'm also a little confused that we're converting everywhere we build a stdClass object into a File object, but we're not converting somethings like file_scan_directory() to return an array of true File entities rather than regular objects.

Because these aren't *managed* files. functions like file_scan_directory() are *way* too low-level to rely on the entity system. This function is used to detect module files, used during installation and so on. Yes, I've been wondering about naming the entity file_managed and the class FileManaged, same as the table, but that would IMHO imply file_managed_*() functions instead of file_*() which would be a way too big change or this patch.

Then I guess my confusion then is why the locale.module is converted to use File entities when it doesn't seem to use managed files at all. It just wants a file object with ->filename and ->uri just like is used with file_scan_directory().

Yep, that's wrong as discussed. Removed all changes from gettext.inc.

+++ b/core/includes/file.incundefined
@@ -1498,15 +1449,14 @@ function file_save_upload($source, $validators = array(), $destination = FALSE,
-  // Begin building file object.
-  $file = new stdClass();
-  $file->uid      = $user->uid;
-  $file->status   = 0;
-  $file->filename = trim(drupal_basename($_FILES['files']['name'][$source]), '.');
-  $file->uri      = $_FILES['files']['tmp_name'][$source];
-  $file->filemime = file_get_mimetype($file->filename);
-  $file->filesize = $_FILES['files']['size'][$source];
+  // Begin building file entity.
+  $file = entity_create('file', array(
+    'uid' => $user->uid,
+    'status' => 0,
+    'filename' => trim(drupal_basename($_FILES['files']['name'][$source]), '.'),
+    'uri' => $_FILES['files']['tmp_name'][$source],
+    'filesize' => $_FILES['files']['size'][$source],
+  ));

Here we need to set filemime manually since it sets it based of filename and not URI.

+++ b/core/includes/file.incundefined
@@ -1684,7 +1632,7 @@ function drupal_move_uploaded_file($filename, $uri) {
+function file_validate(File &$file, $validators = array()) {

Possibly a follow-up, but the & is unnecessary here.

+++ b/core/lib/Drupal/Core/File/FileStorageController.phpundefined
@@ -0,0 +1,65 @@
+    // Automatically detect filemime if not set.
+    if (!isset($values['filemime']) && isset($values['filename'])) {
+      $values['filemime'] = file_get_mimetype($values['filename']);
+    }

This should actually be calling file_get_mimetype on $values['uri'], not filename. That function expects an URI.

+++ b/core/modules/file/file.moduleundefined
@@ -724,7 +725,7 @@ function file_managed_file_pre_render($element) {
- *   - file: A file object to which the link will be created.
+ *   - file: A file entity to which the link will be created.

I don't believe this change is valid anymore for theme_file_link().

+++ b/core/modules/file/file.moduleundefined
@@ -735,7 +736,8 @@ function theme_file_link($variables) {
+  $icon = theme('file_icon', array('file' => ($file instanceof File) ? $file : file_load($file->fid), 'icon_directory' => $icon_directory));

Eeek ternary, but I guess I can live with that.

Ok, what's about this?

#81: file-entity-1361226-81.patch queued for re-testing.

Adding 'avoid commit conflicts' tag.

Conflicted tests:
image module tests
file module tests
entity module tests

system.test

+++ b/core/lib/Drupal/Core/File/File.phpundefined
@@ -0,0 +1,94 @@
+  /**
+   * The file language code.
+   *
+   * @var string
+   */
+  public $langcode = LANGUAGE_NOT_SPECIFIED;

As far as I know this is introducing a new property that is not really supported by file entities since there is no storage for {file_managed}.langcode. Should this be removed?

+++ b/core/lib/Drupal/Core/File/File.phpundefined
@@ -0,0 +1,94 @@
+  /**
+   * The size of the file in bytes.
+   *
+   * @var string
+   */
+  public $filesize;

Should be @var integer

+++ b/core/lib/Drupal/Core/File/File.phpundefined
@@ -0,0 +1,94 @@
+  /**
+   * UNIX timestamp for when the file was added.
+   *
+   * @var integer
+   */
+  public $timestamp;

Should be "for when the file was last saved." $file->timestamp is constantly updated.

+++ b/core/lib/Drupal/Core/File/FileStorageController.phpundefined
@@ -0,0 +1,65 @@
+  public function create(array $values) {
+
+    // Automatically detect filename if not set.

Extra blank line here. Sorry I'm being nitpicky.

+++ b/core/includes/file.incundefined
@@ -617,60 +618,10 @@ function file_load($fid) {
- if (!isset($file->langcode)) {
- // Default the file's language code to none, because files are language
- // neutral more often than language dependent. Until we have better flexible
- // settings.
- // @todo See http://drupal.org/node/258785 and followups.
- $file->langcode = LANGUAGE_NOT_SPECIFIED;
- }

Ok so ignore what I said about langcode, but this bring up do we need to move this logic to the presave() method in FileStorageController?

Ok, fixed the docblocks and re-added the language check, looks like that one got lost during an earlier re-roll.

Ok I've seen this enough times that I think this is ready to fly. I'll RTBC pending testbot confirmation.

Passed the bot and I chance to do a click-through on a D8 sandbox and didn't encounter anything unusual.

I just confirmed that this patch does conflict with the kernel patch, which is AFAIK going to be commited tomorrow, so it will need a re-roll after that and also shouldn't be commited before.

I'm going to be mostly out until Monday, but if it's RTBC at that point I'll commit it asap assuming I've got time (and this shouldn't put Dries off committing it sooner).

Thanks for the review. I'll wait with a re-roll until the kernel patch is.

Discussed the delete() method with fago in IRC. We would like to use the default delete() implementation and move the special stuff to pre/postDelete(). The thing is that there is still that conditional delete in there, which is problematic on it's own and would not be compatible with the default implementation (silent abort).

Note that this is only relevant for *invalid* files (this means it's either a directory or something weird like a symlink, non-existing files are logged but count as a success).

So we have two options:

- Ignore failures, delete anyway. This means you *can* optionally check your watchdog and clean up manually afterwards (e.g. by deleting the symlink).

- Throw an exception for invalid files, abort deletion. This means you *must* check watchdog/exception message and clean up manually before you can delete the file in the UI.

Note that there aren't many remaining places where we delete directly. It's mostly system_cron() now and a few special places, like moving files and deleting temporary files (which should *not* have this problem, as we just uploaded the file). Still, I currently prefer ignoring failures, keep the watchdog log and continue deleting the file entity.

Ok.

- Re-rolled for the kernel-patch. Was trivial, git rebase++.
- Fixed the 80 chars commentt.
- Changed delete() to preDelete() using the the "ignore failures" approach outlined above. I'm also deleting file_usage entries there, as it makes sense to delete them first anyway, as they reference the file_managed table.
- Also fixed the hook documentation.

.... :)

Re-rolled.

Reviewed this patch in depth, and was ready to commit it. However, when I tried to apply the patch in #97, it didn't work.

vortex:drupal-head dries$ git apply ../f.patch 
error: patch failed: core/modules/user/user.module:1
error: core/modules/user/user.module: patch does not apply

#97: file-entity-1361226-97.patch queued for re-testing.

That's odd, rerolled:

Yep, looks like it conflicted with #1587850: Replace drupal_not_found() with throw NotFoundHttpException, that's a problem with the namespace/use stuff, that conflicts all the time.

The patch in #102 seems to be missing two small changes in user.module, here's another re-roll.

Looks good now. I had already reviewed this twice. Committed to 8.x. Thanks!

I updated the entity change notice. I think it's done now...

AWESOME http://drupal.org/node/1400186

YAY! Great job, folks! :D

removing tag

Updated issue summary.

Swap to the right media tag.