This patch extracts the file handling functions from filefiled.module into filefield.inc. The idea is to commit this and then start porting features from imagefield and multimediafile.inc (from mediafield module), then let those module depend on filefield ... is this ok?

CommentFileSizeAuthor
#19 filefield-split-v7.patch17.98 KBjpetso
#17 filefield-split-v6.patch15.63 KBEgon Bianchet
#12 filefield-split-v5.patch13.23 KBjpetso
#10 filefield-split-v4.patch10.8 KBjpetso
#8 filefield-split-v3.patch10.05 KBjpetso
#4 filefield-split-v2.patch9.66 KBjpetso
filefield-split.patch9.47 KBEgon Bianchet

Comments

azote’s picture

azote commented

I must agree with Egon... I believe this is the way to go... around having the same functions all over the cck modules..

azote’s picture

azote commented

patch works ok... let hope we can get this on head after further testing ....
about the return NULL; on the filefield_file_update (filefield.inc) ... i have no clue....

drewish’s picture

drewish commented
Status: Needs review » Needs work

before this turns into any kind of API, it really needs to start writing records to the {file_revisions} table.

jpetso’s picture

jpetso commented
Status: Needs work » Needs review
StatusFileSize
new filefield-split-v2.patch9.66 KB

Adding file_revision entries, an include_once statement, and making file deletions work again (the first patch forgot to rename a _filefield_file_insert() call from private to public). Please review again.

drewish’s picture

drewish commented

cool, i'll take a look at that. the only thing i'd suggest just by looking at it is to add an filefield_update to add missing file _revisions records.

drewish’s picture

drewish commented
Status: Needs review » Needs work

the patch won't apply. it looks like your patch is against an older version.

jpetso’s picture

jpetso commented

Just noticed that there have been a couple of changes to HEAD, while the original patch obviously had the DRUPAL-5 version as a base.
So it seems that it's a bad idea to apply the patch as is, I'm going to reroll it from HEAD.

jpetso’s picture

jpetso commented
Status: Needs work » Needs review
StatusFileSize
new filefield-split-v3.patch10.05 KB

Rerolled from HEAD. Please review once again :D

jpetso’s picture

jpetso commented

Er, damn, it doesn't work. Let me debug a bit. Sorry for not testing before uploading.

jpetso’s picture

jpetso commented
StatusFileSize
new filefield-split-v4.patch10.8 KB

Ok, so it might actually be a good idea to include the patch with the files that one actually modifies. It should work better now :-P

drewish’s picture

drewish commented

seems to work fine. the only thing is, as we discussed on IRC, adding revisions support causes the files to appear in the upload module's form items. it might be worth doing some form alter to pull out our rows...

jpetso’s picture

jpetso commented
StatusFileSize
new filefield-split-v5.patch13.23 KB

Yeah, I did it! form_alter magic, here we come. Ready to be reviewed.

Egon Bianchet’s picture

Egon Bianchet commented

I'm not sure it's a good idea to use form_alter, shouldn't filefield be an alternative to upload? In that case users should just disable upload for that content type

jpetso’s picture

jpetso commented

Well, it works, it hardly affects performance, and it provides the possibility to use upload nevertheless. What filefield is used for should be up to the user, not predefined by us.

hbfkf’s picture

hbfkf commented

Subscribing.

Egon Bianchet’s picture

Egon Bianchet commented

moved filefield_check_directory and filefield_form_check_directory to the inc file

Egon Bianchet’s picture

Egon Bianchet commented
StatusFileSize
new filefield-split-v6.patch15.63 KB

The file :-P

Egon Bianchet’s picture

Egon Bianchet commented
Status: Needs review » Needs work

Updates and revisions are broken

jpetso’s picture

jpetso commented
Status: Needs work » Needs review
StatusFileSize
new filefield-split-v7.patch17.98 KB

This patch should make revisions work like they're supposed to. Yay. Please review again, and have a look if you can catch dopry so that we actually have a chance to get this applied :-P

Egon Bianchet’s picture

Egon Bianchet commented
Status: Needs review » Needs work

With a single-file field, if I upload a file, edit the node again, upload a new file to replace the previous, and then revert the old revision, when I edit the node again no file is shown in the form.

Also, does anybody know if it's better to include filefield.inc with a hook_init like content.module does?

I tried to contact dopry but he didn't answer yet

Egon Bianchet’s picture

Egon Bianchet commented

dopry said

I will not split it into a .inc and a .module. It will all stay in the
module. I don't want to reinvent dynamic includesand dependencies that
are already provided by the module system. submit patches as they are.
I'm under a pretty heavy workload at the moment, I hop to start catching
up on my modules and core work this weekend.

so I think we could just limit the patch to the revisions support and attach it in this issue http://drupal.org/node/130269

jpetso’s picture

jpetso commented
Status: Needs work » Closed (fixed)

> With a single-file field, if I upload a file, edit the node again, upload a new file
> to replace the previous, and then revert the old revision, when I edit the node again
> no file is shown in the form.

I actually tested that use case, and it worked here. Maybe we've been doing it slightly differently, but in any case, I can't reproduce what you described.

In other news, I merged the reworked functions back to filefield.module, and posted the related patch here. Unfortunately, I've also got AJAX support lying around in my local filefield checkout, and I don't really feel like decoupling the patch just for patch's sake, so it's kind of a kitchensink patch now. (Which is also why I haven't attached it to #130269.) Sorry for not keeping them apart.

Respecting dopry's opinion on file separation, I'll close this issue now. Further comments please go to the appropriate one of the two issues mentioned above.