file_save_upload() is in file.inc, but we moved the file entity to file.module, so this function should be there too.

Similarly, there's a lot of token-related code in system_token_info() and system_tokens(), probably test coverage too. That also needs to move to file.module.

CommentFileSizeAuthor
#14 moving-file-code-2045189-14.patch30.07 KBjlindsey15
#12 moving-file-code-2045189-12.patch30.06 KBjlindsey15
#8 moving-file-code-2045189-8.patch30.06 KBjlindsey15
#4 moving-file-code-2045189-4.patch29.69 KBjlindsey15
#5 moving-file-code-2045189-5.patch29.96 KBjlindsey15
#2 moving-file-code-2045189-2.patch40.72 KBjlindsey15
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
Assigned: Unassigned » jlindsey15

I'll tackle this.

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
Status: Active » Needs review
FileSize
moving-file-code-2045189-2.patch40.72 KB

Here goes...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Thanks for working on this!

+++ b/core/modules/file/file.moduleundefined
@@ -603,6 +603,281 @@ function file_theme() {
+function drupal_move_uploaded_file($filename, $uri) {

Not sure if this needs to be moved, doesn't seem to depend on file entity?

+++ b/core/modules/file/file.moduleundefined
@@ -1660,3 +1935,264 @@ function file_library_info() {
+function system_token_info() {

This is a hook, so the function name prefix needs to change to file.

Also, don't move the whole function, just the part that's about files. date/site needs to stay in system_token_info()/system.module.

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
FileSize
moving-file-code-2045189-4.patch29.69 KB

Thanks for the feedback - I think this should work now (or at least it's a lot better than the last one :)

EDIT: see the next comment for the version with proper documentation.

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
Status: Needs work » Needs review
FileSize
moving-file-code-2045189-5.patch29.96 KB

I forgot to change the comments to reflect the changes in that last patch, but it's good now.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: documentation » file.module

Not just documentation.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work

Close :)

+++ b/core/modules/file/file.moduleundefined
@@ -848,6 +1086,126 @@ function file_file_predelete(File $file) {
+    'type' => 'date',
+  );
+  $file['owner'] = array(
+    'name' => t("Owner"),
+    'description' => t("The user who originally uploaded the file."),
+    'type' => 'user',
+  );

This is missing a return of the types and tokens.

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
Status: Needs work » Needs review
FileSize
moving-file-code-2045189-8.patch30.06 KB

Okay another try!

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
Status: Needs work » Needs review

I'm without access to git for the next week and a half for not-worth-explaining reasons - that last patch was an attempt to manually fix the patch file, which not surprisingly failed. I'll finish it up when I get back, unless someone else does first.

Status: Needs review » Needs work

The last submitted patch, moving-file-code-2045189-8.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Assigned: jlindsey15 » Unassigned
Status: Needs review » Needs work

Unassigning then, thanks for working on it, let's see if someone picks it up.

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
Assigned: Unassigned » jlindsey15
Status: Needs work » Needs review
FileSize
moving-file-code-2045189-12.patch30.06 KB

Managed to finish it up anyway! (hopefully...)

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Needs work 
+++ b/core/modules/file/file.moduleundefined
@@ -848,6 +1086,133 @@ function file_file_predelete(File $file) {
+  $types['file'] = array(
+  'name' => t("Files"),
+  'description' => t("Tokens related to uploaded files."),
+  'needs-data' => 'file',

Indentation here is a bit off, missed that before.

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
FileSize
moving-file-code-2045189-14.patch30.07 KB

Indentation issue fixed:

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented
Status: Needs work » Needs review
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Great, looks good to me.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Needs work

System module calls file_save_upload() unconditionally with the patch applied - that'll be a fatal error with the patch applied and file module disabled no?


      // Check for a new uploaded favicon.
      $file = file_save_upload('favicon_upload', $validators, FALSE, 0);
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Reviewed & tested by the community

That code is wrapped in a module exists check. It would already fail right now (due to the missing entity type), that's why we added the module exists there when originally moving the entity type code from system.module/file.inc to file.module. Quite sure that we have tests for this that explode without that module exists check.

So I think we're fine. Yes, maybe that should be a form alter, but we discussed that back then and for some reason, didn't do that.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Arghh missed the module exists check.

That's ugly but it's not introduced here, so committed/pushed to 8.x.

jlindsey15’s picture

jlindsey15 CreditAttribution: jlindsey15 commented

I'm just curious, since this is the first patch I've made to be committed - is there somewhere on your account that tells you if an issue you worked on got committed, or lists ones that did? The "Your Commits" tab seemed promising but didn't show anything. I don't really care much if there isn't, and apologies if this is a stupid/out-of-place question, but if I was missing something I wanted to know!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented

@jlindsey15: Congrats on your first core commit! Sadly, there is currently no such listing in core.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Congrats! There is no official list, the reason is that the profile only shows commits you commit yourself and those where the committer used --author, which is not used for core as it only works for a single author.

There are inofficial lists, which parse the git commit messages, but they might contain errors. See http://ericduran.github.io/drupalcores/, for example.

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