Move file entity info, managed file, and file usage functionality into File module

related: Entity file it extends a lot of the system entity hooks. Perhaps there's a place for it in core?

We'd need to move the file_managed schema definition from system module too. The problem with this is that some parts of system module use that, which would make system module depend on file module.

Overall it makes sense to move anything we possibly can out of system module, but we'll need to be careful here.

Moving more things from system to File module would also make sense for merging File entity into core to enable fields on files and provide the UI as well.

Once #1361226: Make the file entity a classed object is in, we have an even more explicit dependency from includes/file.inc and Drupal\Core\File to entity.module due the entity class and the type-hints.

So the logical conclusion would be to move *everything* that deals with managed files into file.module, including:

- The schema
- The entity_info()
- The entity classes
- Every function from file.inc that deals with managed files, like file_save_upload() and the validate stuff, move/copy, obviously the crud functions and so on. This means file.inc only deals with unmanaged files from then on.

Yes, this would make file.module a required module that both user.module (until $user->picture is moved into a image field and maybe even then) and system.module would depend on, depending itself on entity.module and user.module (validation stuff for example). Still crosss-dependencies, and still a mess, but better than what we have now.

I'm not sure if it would make sense to move the file_field stuff into a separate module to make that part optional. But I think that's all just hook implementations, so can be altered and changed...

We could move the field stuff to filefield.module. However, yched is pushing very hard to revamp Field API to use plugins, which makes it all OO, which means there's no code overhead of happening to have the code in an enabled module. So, I don't really see a need to do so.

I'm unclear on how system.module would then depend on file.module, but that's probably an indication of more stuff that needs to come out of system.module more than anything else. :-)

system.module for example has things like an upload field for the site logo. Not sure if we can move that into file.module, maybe ;)

But yes, many things in there could probably be moved into file.module as well, for example tokens, obviously the tests, the cleanup stuff in system_cron() and so on. system also contains image.gd.inc, no idea if that's a candidate as well, probably at least a separate issue as that is afaics not depending on managed fields.

Can someone draw a picture of the related pieces and their dependencies? I'm kinda lost at this point so it's hard to say what goes where when. :-)

No picture, but:

file.inc includes both unmanaged and managed file APIs.

file module provides the file field, nothing else.

system module implements the following on behalf of file.inc:

system_schema()
system_entity_info()
system_cron()
image.gd.inc
tokens
Possibly other stuff, you never know what you'll find once you start looking in system module.

Additionally, system module provides admin screens that depend on the managed file field, for example the logo upload.

This means that if all managed file stuff moves to file module, then due to those administration screens, both system module and user module will have a dependency on file module. In terms of cross-dependencies, the admin form for the site logo and user pictures are probably the least worst ones to have. The other option would be to move everything to do with managed files to system module, but that only gets around the cross-dependency issues because system module cheats.

Icky.

I would agree that it's better to move most of the code to file module for now, get those two small dependencies, and figure out how to break those later. Once we've got those knots unraveled we can refine from there. That's definitely better than throwing anything new into system.module.

So then:
- Unmanaged files: /core. (May leverage Symfony's Finder component? Not sure.)
- Managed files: file.module
- File fields: file.module
- Related stuff that requires managed files: file.module
- Image fuzting: Um, should this perhaps become its own module, or does it also go in file.module?

Working on this one.

Ok, here is a start. Haven't touched the tests yet as they will be moved to PSR-0 soon. Just trying to see how far this gets us.

Ok. Let's see what happens now.

This patch includes #1598574: Convert file.test to PSR-0, that's one of the reasons for the size. Once that is commited, this one should be *much* smaller.

And the patch went in, so here's a re-roll.

Tagging.

This should fix the fatal errors.

Ok, added both an explicit dependency on file to aggregator and locale and made the file.module required for now as it is needed in the theme settings. We'll see if we can clean that up.

This might pass.

+++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityCrudHookTest.php
@@ -34,7 +34,7 @@ class EntityCrudHookTest extends WebTestBase {
-    parent::setUp('entity_crud_hook_test', 'taxonomy', 'comment');
+    parent::setUp('entity_crud_hook_test', 'taxonomy', 'comment', 'file');

+++ b/core/modules/file/lib/Drupal/file/Tests/FileManagedTestBase.php
@@ -2,19 +2,22 @@
-    parent::setUp('file_test');
+    parent::setUp('file_test', 'file');

Since 'file' is now a required module that should not be necessary. It could be included here, if we think we'll make file.module optional again later in the cycle and don't want to go back and add this everywhere. Wanted to mention it, though.

Otherwise looks very good. I looked through and couldn't find anything to complain.

Right, but making file required was the last thing I did, I first wanted to know where we actually need it. Same for the added dependencies. Not sure, maybe we can move the problematic parts in system.module to file.module and alter it in. So if can go without file.module but then you don't get stuff like an upload for a logo/favicon.

Either an alter from file module, or possibly module_exists() in system module seems like it would be OK for now to avoid adding a new required module.

Ok

- Made the site logo/favicon features optional. The only thing that's actually used there is file_save_upload() and that only for the validation part. It then uses file_unmanaged_copy() in the submit o.0
- Added file.module as a dependency for update.module. Again, it only seems to use file_save_upload().

Not sure what's the best way to deal with those cases. Actually properly use managed files for site logo? Introduce a non-managed file validation API?

Anyway, this should pass the tests.

- Ok, needed to add another if statement in the submit callback obviously. Nice, this means that we now have test coverage for the theme settings form with and without file.module, perfect.
- The second failure was caused because aggregator.module now depends on file.module and the test tried to enable aggregator.module but now we got a intermediary dependency enabling confirmation.

This patch looks great!

I (strongly) agree that the setUp() calls should explicitly specify the modules to install, since that will be required in the future either way, so let's keep that. :)

However, I don't really feel comfortable with requiring the File field module for all managed file operations. That is, because this dependency does not only involve File module, but actually the entire stack of field, field_sql_storage, entity, ... modules.

To clarify:

+++ b/core/modules/aggregator/aggregator.info
+dependencies[] = file

E.g., Aggregator apparently is one of the modules you could potentially use without the entire Field API stack.

+++ b/core/modules/locale/locale.info
+dependencies[] = file

+++ b/core/modules/update/update.info
+dependencies[]=file

Likewise, Locale and Update modules have no business at all with fields, too.

In light of that, I actually feel more comfortable with the current situation; i.e., having the code baked into System module. But obviously, we badly want to untangle that.

It looks like the only reason we're merging this code into File module is that File module has taken the most appropriate namespace already. I wonder whether it's time to rename all field modules into *_field ? (separate issue)

However, to not block progress here, would it be acceptable to call this module (temporarily or not) managed_file.module ? (or perhaps even file_entity.module, in case that is appropriate?)

Related: #1688162: Replace required flag of modules with proper dependencies

field already is a required module, isn't it? We need it for the hardcoded field_attach_*() calls, if nothing else. entity, obviously, is a hard requirement as well, no way around that. In fact, I'm not actually sure why entity was made a module. (Ah, only saw the related issue link now, but that wouldn't really change this, you can't run Drupal without user.module and user.module requires field/entity).

I think renaming the field-providing modules to *field (filefield, imagefield, listfield, textfield, ..) has been discussed before, in the list/options issue as well. That does sound like a good idea to me, not sure in what order we should do that.

I don't really get the file_managed naming and it is a mess (the table is called file_managed but the entity and the API functions are file_*) so I don't really like to introduce a new module here that is called file_managed (with file_*) API functions which is then again renamed back to file when that namespace is free again. Similar for file_entity, that would mean renaming the entity and api functions to file_entity_load(), which would get us into trouble with hook_entity_load(), among other things ;)

These are the options that we have, IMHO:
- Move it to file.module, keep the dependencies and the current module names.
- Rename field modules to *field first, then re-create file.module (not sure if there is anything else other than the file field in there currently)
- Move it now and then split file.module into file.module and filefield.module.

Berdir: Any module that is a hard requirement for Drupal to even boot is a bug. There are no exceptions. That includes user module. Field is only required if you have node and entity enabled, which are optional by design.

system.module is a design flaw that needs to just go away.

Crell: Point taken :) The question is how far can and should this issue go in regards to improving the current situation, which is quite far away from what you are describing.

Current situation:
Hard dependency of /includes, /lib and system.module on entity.module. Files aren't fieldable, so there's no dependency on field.module but since there are also hard dependencies on user.module from the same places (e.g. session handling), which is fieldable, we already have the hard dependency on field.module anyway at the moment.

With this patch:
File entity moved to an optional module, which currently happens to implement a field type as well. No more dependencies on that part. aggregator, locale and update now depend on file module, which implies field.module.

IMHO, that's quite a nice improvement, and the added direct dependency on field.module to those modules *currently* doesn't hurt us because that is indirectly already here through user.module. We don't need to/can solve "all the problems" here I think.

So I'd vote for the third option in #27, which is what this patch does. If we agree on doing that split, then I think that can happen much faster than removing all user dependencies from /includes code so we can have that ready when field.module becomes optional.

I think it makes sense to move it now then try to refine things further later. There are so many interdependencies in core that even if we're only swapping insane dependencies for slightly saner ones that still feels like a step forward. Splitting file module into the managed file API vs. the field doesn't sound too bad to me but definitely feels like its own issue.

I agree - let's move from system to file and then split out the field later.

ok. I've created #1696946: Rename field modules to [type]_field and thus think we can move forward here.

On these grounds, this patch looks actually RTBC to me. Marking as such, but feel free to disagree. ;)

#25: file-entity-module-1468328-25.patch queued for re-testing.

Re-rolled.

Re-roll, let's see if all went well.

#37: file-file-download-context-1245220-37.patch queued for re-testing.

Re-rolled.

Review anyone? ;)

Uh. Patch would help.

This looks really good but the patch chunk adding perf.php probably needs to be removed?

Uh, yes. Sorry about that.

It would be nice if we could have a file uploader that could save to temporary only and didn't have to work with file entity objects at all, but that is definitely a follow-up.

Pretty much a straight transfer of code, passes locally and the bot, so I believe this is ready to go.

Re-roll of the patch. No changes other than copying the new versions of the functions from file.inc to file.module, you can see the actual changes in the patch in the attached interdiff (it's actually a normal diff of the patch files, interdiff can't handle that stuff).

Leaving at RTBC.

Ok, I had to convert the new field download access alter hook stuff and noticed that the test class has not been converted properly. Posted a patch to #1468328: Move file entity info, managed file, and file usage functionality into File module and also included it here. Will need a re-roll anyway after that quickfix is in. Unless this would get in first, which is doubtful.

Wow, no idea what happened there, trying to upload the patch with my slow connection.

Wow, no idea what happened there, trying to upload the patch with my slow connection.

Restoring tags. Sorry for the spam.

Re-rolled now that the file download access hook was fixed and also fixed the still wrong use in node.module. This really really should pass.

+++ b/core/modules/update/update.infoundefined
@@ -4,3 +4,4 @@ version = VERSION
+dependencies[]=file

Really nit-picky. Should have spaces around the = character.

Fixed! :)

Yay, looks great and passed locally and on the bot.

#54: file-entity-module-1468328-54.patch queued for re-testing.

Rerolled.

s/EntityInterface/StorableInterface

Looks like git rebase didn't have any issues at all to rebase this. Let's see if this works.

@timplunkett: Our double posts are starting to worry me :)

Let's see if my patch will work better :)

The patch in #60 is a reroll of a previously RTBC patch.

The patch applied cleanly. I tested a number of operations on managed and unmanaged files including creating a number of file fields, uploading theme logos and shortcuts, etc and didn't encounter any problems.

RTBC +1

#60: file-entity-module-1468328-58.patch queued for re-testing.

Please commit this as it is holding up many other issues for the file system component.

Here's a re-roll, let's see if this works.

@webchick: system.module is the only module that's allowed to implement hooks for code that lives in /includes. And only because we have no other way to do that at the moment. If we move hook_entity_info(), then we also need to move the schema of those tables, the entity classes and all API functions which depend on the file entity. And the tests.

Yes, a number of modules now depend on file.module which didn't do so before. On the other side, there's now quite a chunk of code and two tables that don't need to be parsed/installed anymore when not actually required. During tests, for example. To simplify the dependency chain, the goal is to move the file field (which is the main part of the current file.module) to a new filefield module.

Someone up for extending this and make an issue summary out of it? Already 2am here and I'm not sure if I'll find the time to do this tomorrow. Also ,this will conflict with the registry patch which is due to be committed tomorrow but I can do a quick re-roll once that has been done.

un-tagging since this has been committed.

This change did not properly adjust the installer:

When installing with Minimal profile and keeping the "Check for updates" option on the last installer page, then the installer automatically enables Update module.

Update module depends on File module with this patch, but does not get enabled in that step. When visiting the Modules page to enable further (other) modules, I suddenly get a confirmation form that tells me that I also need to enable File module:

Hah, I knew it, I was waiting the whole day for someone to show up with an installer issue! :p

Yeah, the installer explicitly does not enable dependencies, which I doesn't work at that point yet.

Should be as simple as this? Nobody is hurt if file.module is already enabled...

Thanks! ;) I'm actually not sure why we're explicitly not checking for dependencies there, but I guess that's for another issue to fix.

Created #1765848: Installer does not resolve all dependencies of Update module

I don't know what more to write than http://drupal.org/node/1770168. I also updated the namespace in http://drupal.org/node/1400186.

All you need to do in a module that uses files is to make sure that you have a dependency on file.module, there are no API changes specific to this issue other than than the namespace rename, which is not relevant if you're upgrading from 7.x, you just have to change it to a different one now, which is documented in the entity change record.

The change notices looks good to me.

No two change notices are not on fire.

Fix media tag.