Faulty default value for file_private_path

We had a strange problem today. We use CKFinder, and suddenly on one particular website the pictures were uploaded to the Drupal root folder instead of /sites/{site-name}/files. We had to go through the source code of the CKFinder module to finally figure out the cause of this problem. Not inituitively, it was caused by simply opening the file path system settings and saving it (hence: not even modifying it!): /admin/config/media/file-system. Because when you save a system_settings_form, the variables will all be set, and this is the default value (''). This works well as long as all defaults everywhere in the variable_get function are set to the same default value, but CKFinder did use another default for the variable 'file_private_path'.

When you use CKFinder, the file_private_path variable is used to identify the location to store the pictures and files uploaded in CKFinder. In the CKFinder module, if the file_private_path is not set, it defaults to 'conf_path() . '/files'' (see modules/ckeditor/includes/ckeditor.lib.inc). However, the default for file_private_path in the core is simply '' (or FALSE). So after saving the variable we the CKFinder used the empty string as file_private_path and files were stored at the Drupal root. We solved the problem by deleting the variable record from the database (ouch!). Good luck finding out the problem to the average Drupal user!

In theory all occurrences of variable_get('file_private_path', ...) should have the same default value. CKFinder should get the variable, then test if it is not false, and if so use it's default, like this:

$fpp = variable_get('file_private_path', FALSE);
if (!$fpp) $fpp = conf_path() . '/files';

But of course the underlying reason for a lot of problems like these is the fact that you CAN specify different default values when this really leads to undesired behavior, and never to desired behavior (at least not until after opening and saving the configuration form associated with it). Therefor I also posted a message on the Drupal Core about handling variables.

However, fact of the matter is still that CKFinder shouldn't specify another default for file_private_path at all.