system module no longer saves the files and tmp dir

I also noted this. We need a '#post_process' on this form, too.

Let's get used to this: when someone writes down $_POST, God kills a kitten. (Or if he is busy, I do, and because I feel sorry for the kitten, I replace the kitten with you :P)

I can't find any mention of #post_process anywhere except the patch that implemented it.
What on earth does #post_process do and how can it help resolve this bug?

p.s. here's a new version of Bèr's patch that applies against head.

I stand corrected. My cursory look was wrong. You need #valid here and not #post_process.

Let me explain what happened. The validation routine got the default value. No wonder it did not work. The patch feds the validation routine the actual value. Most of the patch is just formatting. The real meat are the the two '#valid' values and the "long" validation function.

I have just tested this patch on a clean install from cvs, apllies cleanly and works as advertised.

I tested this patch and found that errors were no longer presented correctly.

Previous behaviour was to try to validate the directories and present an error on clicking admin>settings (i.e. on creating the form, not processing a form submission). I don't know if it's a design decision to remove this error reporting, but I will say that I found it useful.

Also, on entering new values and submitting the form an error is no longer shown if either the files or temp directory don't exist or couldn't be created.

It also looks like the error would be logged to the file_directory_path form item if the temp directory couldn't be created.

I will have a look at this patch and see if I can improve the error handling.

I have fixed a few problems in the last patch:

- I changed validation function's name to system_view_general_dir_valid.
- Added a phpdoc comment to the validation function.
- Changed the validation function so it adds the error to the appropriate form element.
- Changed the description on the temp directory form element to indicate that a relative path is relative to drupal install, not to files.
- Added back in the test to see if these directories are present when first showing this form.

I am not sure if this last change is done well... I'd hate to be responsible for any kittens being harmed.

To check to see if the form was being shown (i.e. the user has just clicked admin>settings) I put the tests in a if (!isset($_POST['op'])). I couldn't work out any other way to do this that wasn't even worse than looking at $_POST.

I am pretty sure this test should still be done because a similar test is made in system_theme_settings.

Note when testing this patch on a Unix system that the temp directory name isn't being shown in the error message. This is because of another problem: http://drupal.org/node/26249

I made the following tests: changed the tmp dir, it was created automatically. I changed files permissions so that it is not writeable and the error message is shown. Changed back and then I tmp permissions so that it is not writeable and the error message is shown for tmp field.

That last patch was missing the second argument to system_view_general_dir_valid from inside the if (!isset($_POST['op'])).

Apart from that this is a very nice patch.

But that array() is not needed.

If you wrap the other ", '#maxlength' => 255," the patch will be perfect.

:)

Here it is... the perfect patch!

Works for me.

The PHPdoc comment needs work; it's not valid English.

Oh, well spotted!

For consistency, this is probably better.

patch works for me, too

Can't we think of a better name for system_view_general_dir_valid() and isn't $element_id the $directory? If possible, I'd like to see someone improve the readability of the code, and the validation function in particular.

just FYI --
I did a fresh install of drupal-cvs, and created the files directory with full permissions (777)

I created pristine new databases

I applied system_default_files_dir_5.patch, which at least on the surface seems to have been successful (no reported errors, system.module was updated)

And I still get the error message "The directory does not exist."

(OS X 10.4.3, PHP 5.04)

With a clean install of drupal and the _5 patch, going to settings initially gets me this problem; changing the tmp directory works.

Even with this patch, however, the error message is very confusing, since in my system's case, "" is being passed to file_directory_check.

I believe this should be checked against an empty value and a different error message should be generated.

after filling in the tmp directory, everything seems ok. Sorry for any confusion.

The temp directory name isn't being shown in the error message because of another bug which is ready to be committed at: http://drupal.org/node/26249

These two patches conflict, so I'm hoping that other one will be committed soon so I can make sure the error message is OK before this patch is committed.

Here's a new patch that cleans up the validation function. It is only called when the form is submitted now which makes more sense and allows the function to be much simpler.

The directories are now validated when the form is first displayed by calling file_check_directory directly which makes the earlier code more understandable and more efficient.

I also reintroduced the $directory_path and $directory_temp variables both to reduce the amount of change from the original code and to increase legibilty.

Richard: http://drupal.org/node/26249 has been committed now.

IMO directory checking belongs to file.inc. I moved the function there and now it's name is really nice. Also, I am not calling directly file_check_directory when there is no POST, this seems more future proof. I think the solution I choosen is pretty nice.

Some whitespace made the patch bigger...

This is so confusing. Each time chx posts a patch he improves some things but other things I had fixed are missing from his patch.

Since this issue is now assigned to chx I think I'll stop posting patches and just post comments instead :)

So... some comments:

The description of file_directory_temp is incorrect. I fixed and documented this in #7 and it was fixed again in the #23 patch.

file_check_directory is called twice on first showing the form. And is still called with the original value on submitting the form (which is the behaviour this patch is trying to correct).

Those two things are the real show-stoppers for this patch. But a couple more comments on the structure, basically justifying the the way I had this set up in patch _6. I found that code easier to comprehend than the latest patch.

I think calling file_directory_valid with $key is a complex way of implementing this functionality. The only reason I can see for adding the $key to the validation function is so $form['files']['file_directory_temp']['#default_value'] can be used instead of the temporary variable. I don't like this for a few reasons: 1. the name of the new temp variable is a real monster, 2. using $form as a temp variable is a bit confusing, 3. it requires extra complexity in the validation function and 4. it is more efficient to directly call file_check_directory.

I think the form validation function should be used for validating the form and not try to have other functionality added. Especially since file_check_directory already exists with exactly the functionality required.

I'm also not sure that having a validation function for a system.module form in file.inc is particularly clean. The way it was set up, with the validation function acting as the point of coupling between these two files was easier to comprehend than having the forms API act as the coupling. I don't think form.inc should need to collaborate directly with file.inc to process this form.

There are several ways to skin a cat. I said #post_process in the very beginning, right? So... let's see that.

I fixed the description, thanks Richard.

Deleted POST op cruft. Tested, yes.

Now that's a nice clean way of doing it. I see now how #post_process can be used. It's causes the nominated function to be called when the form is built. I was assuming the "post" in post_process related to method="POST", not post as in "after". That's a confusing name for that attribute.

And it's tricky that the form_value is set from either #default_value or the submitted value as appropriate. I'm not sure I much like hiding all this pre- and post- validation behind such an obscure forms API element, but it does seem to work nicely and I guess we're building a form, so the forms API should be leveraged to the fullest.

Now, some more comments :)

The two calls to file_check_directory need to be deleted. This function is being called twice on first loading the form because the #post_process is calling it too.

Then the $directory_path and $directory_temp variables serve no purpose and can be deleted and inlined into the array definition.

The function name system_file_directory no longer captures the essence of what this function does. Perhaps system_check_directory would be better?

The phpdoc comment needs to have $form_element_name replaced with "$form['#parents'][0]". This sentence would also read OK if "$form_element_name" was simply removed.

The parameter $form to system_file_directory is not the form but rather the form element containing the directory name. This parameter should be renamed $form_element and the phpdoc comment updated.

It's also a nuisance that $form_id is passed to post_process functions as the first parameter when it's hardly likely to ever be used. I'll post a bug and patch to the forms API to address that (after this patch has been committed, because otherwise the two patches will conflict).

Good work Karoly... I'm sure we'll get this patch finished and committed before 4.7 :)

phpdoc, function name updated. pesky remaining calls deleted.

Yes we wil finish this patch because it is a blocker...

Looking good.

The functionality of this patch is rock solid. Well done!

Sorry, I missed a couple of things last review:

All the other @param tags in this system.module have full stops at the end of the sentence. So I guess these two new ones should too.

I think the second @param tag would read better as: "The form element containing the name of the directory to check."

More phpdoc fix. Now ready to commit.

Yes, this patch is spot on.

Good work :)

Thanks for the extensive reviews and patch ping-pong. Great job.
Patch committed to HEAD. Thanks.