Support from Acquia helps fund testing for Drupal Acquia logo

Comments

arianek’s picture

Project: Documentation » Drupal core
Version: » 7.x-dev
Component: Missing documentation » documentation

moving to the core docs queue

Anonymous’s picture

Whoops, sorry, thanks!

jhodgdon’s picture

Version: 7.x-dev » 8.x-dev
Category: task » bug
Issue tags: +Needs backport to D7

Good catch, and thanks for reporting! Needs to be fixed in d8, then backported to d7.

Anonymous’s picture

Title: No example implementation for hook_file_copy » No example implementation for hook_file_(copy|update|move)

It turns out that update and move are also not documented.

jhodgdon’s picture

Issue tags: +Novice

Probably a good Novice project (hopefully)...

javi-er’s picture

Status: Active » Needs review
FileSize
2.42 KB

I guess I qualify as a novice contributor :-)

I looked around for an implementation of these hooks and couldn't find anything, so I did an example where the user name is added at the beginning of the file name on these file operations. Plus a log is saved in the database.

Maybe I was a little enthusiastic on the example and it's a bit too long, let me know what do you think. An alternative it's just adding a log on the database of the current operation.

By the way, this is a working example, I tested it locally.

Status: Needs review » Needs work
Issue tags: -Novice, -Needs backport to D7

The last submitted patch, file-api_documentation-1173322-6.patch, failed testing.

javi-er’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Needs backport to D7
jhodgdon’s picture

Status: Needs review » Needs work

Thanks! There are a few creative misspellings of the word "beginning" in the code comments, and, the code doesn't quite conform to our coding standards:

+  if(strpos($file->filename, $file_user->name) !== 0){

should be

+  if (strpos($file->filename, $file_user->name) !== 0) {

there need to be spaces added after if and before {

But... I am not sure these examples will work anyway. It looks like probably these hooks get invoked after the file has already been moved, copied, etc., and they allow a module that is keeping track of files to respond to this event. I don't think that the name will actually be changed by the actions in these proposed hook bodies? You could go to api.drupal.org, look up the hooks, and click on the "1 invocation of hook_*" links to find out for sure... Also, there are a couple of implementations in test modules, so you could look there to get ideas or copy one of those examples.

Thanks!

javi-er’s picture

Hi! I just made those coding standards modifications, please take a look to the attached patch.

About the example, this is a working example. I create a new module and wrote the code there, once I made sure that it was working bug-free I copied and adapted it to the example documentation.

Maybe the example is a bit complex, I can make it so it just write a line in the Drupal log instead.
Bellow is a description of what the code does:

  • On file move, file update or file copy:
    1. Load the file owner user.
    2. If the file name doesn't contains the user name at the beggining (e.g. admin_file.jpg).
    3. Adds the user name at the beginning of the file name and saves the file.
    4. Logs the event with watchdog.
javi-er’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

Great! I think that the code is clear and these are good examples.

The only thing that I think needs attention is the in-code comments:
- Normally comments in code use verbs like "Load the ..." vs. "Loads the...".
- In the comments that say "...the user name is contained at the beginning of the uploaded file." it should say "file name" at the end.
- Say "Check whether..." instead of "Check if..."
- I don't think that the watchdog() line needs a comment at all (it should be obvious what that line is doing without a comment). Same with the user_load() line. Actually, I think maybe just one comment at the beginning saying "Make sure that the file name starts with the owner's user name." would be enough?

javi-er’s picture

Status: Needs work » Needs review
FileSize
1.98 KB

I agree that just a comment should be enough, in general the rule is that any code that's not self-explanatory should be commented and in this case simply stating what the code does should be enough, in the first patch I went with as much comments as possible since some examples I saw before starting seemed to be using as much comments as possible.

Attached is a revised version, let me know what do you think.

Thanks for the grammar corrections, I'm not too good at it since English isn't my primary language :-)

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Patch (to be ported)

Thanks! Looks good, committed to 8.x. The patch doesn't apply to 7.x, so it needs to be ported.

joshf’s picture

Assigned: Unassigned » joshf
dcam’s picture

Status: Patch (to be ported) » Needs review
FileSize
2.67 KB

Backported #13 to D7.

dcam’s picture

Assigned: joshf » Unassigned
jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Status: Needs review » Reviewed & tested by the community

Looks good, thanks!

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I cannot get the patch above to apply to d7 currently.

dcam’s picture

Status: Needs work » Needs review
FileSize
1.81 KB

Rerolled #16.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thanks! I'll get that committed shortly.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks again - committed to 7.x.

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