Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Letharion’s picture

Have yet to port the variable_{del,get,set}.

Letharion’s picture

Status: Active » Needs review
FileSize
9.28 KB

Patch now includes a new file_entity.variable.inc file that mimics the same functionality from the media module.

Status: Needs review » Needs work

The last submitted patch, 1552988_2_add_file_add_to_file_entity_module.patch, failed testing.

Letharion’s picture

Now actually includes the new file. ;)

Letharion’s picture

Status: Needs work » Needs review
fabsor’s picture

Status: Needs review » Needs work

Since we are introducing file_entity.variable.inc here and functions for setting variables through file_entity_variable_*, we probably want to switch over to that pattern completely, or drop it altogether. There are several places were we still use the generic pattern, it's probably a good idea to not mix them.

Dave Reid’s picture

I don't wish to continue to using the bad variable pattern from media. It's not what core does and we shouldn't start using it here if we're trying to merge this module into core.

Letharion’s picture

Status: Needs work » Needs review

In that case, the patch in #1 needs review, instead of #4 needing work.

Letharion’s picture

Two questions that would be helpful to answer:

Should we simply copy or move the defaults from the media module into their respective, variable_get()'s? Or set them on file_entity install?

Do we support an upgrade path? That would mean renaming the variable prefix.

Dave Reid’s picture

I'm wondering why we need the variables at all? Core already provides us with a default extensions ('jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp' from file_save_upload()) and file size (file_upload_max_size()) that we can use, I don't see a need to add anything new here at all. Media can add it's own default in by using the $params parameter for the form.

Dave Reid’s picture

Issue tags: +sprint, +Media Initiative

Adding sprint tags.

dgorton’s picture

Status: Needs review » Reviewed & tested by the community

#1 works for me (in conjunction w/ #6 from #1552920: Move file/add from media to file.

mtift’s picture

I mentioned it in the other issue as well, but #1 works for me (in conjunction w/ #6 from #1552920: Move file/add from media to file.) works for me, too.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work

A couple of quick updates then this is ready!

+++ b/file_entity.moduleundefined
@@ -867,3 +882,16 @@ function file_entity_file_is_local($file) {
+/**
+ * Sets the status to FILE_STATUS_PERMANENT.
+ *
+ * @param $file
+ *  A file object.
+ */
+function _file_entity_save_file_permenently(&$file) {
+  if ($file->status < FILE_STATUS_PERMANENT) {
+    $file->status = FILE_STATUS_PERMANENT;
+    file_save($file);
+  }
+}

Let's remove this function. To change a file to permanent, it only requires two lines. :)

+++ b/file_entity.pages.incundefined
@@ -31,6 +31,190 @@ function file_entity_view_page($file) {
+  elseif ($tmp = variable_get('file_extensions')) {
+    $validators['file_validate_extensions'] = array($tmp);
+  }

Let's replace this with:

else {
$validators['file_validate_extensions'] = array('jpg jpeg gif png txt doc xls pdf ppt pps odt ods odp');
}

+++ b/file_entity.pages.incundefined
@@ -31,6 +31,190 @@ function file_entity_view_page($file) {
+  elseif (($tmp = variable_get('max_filesize')) && $tmp < $max_filesize) {
+    $validators['file_validate_size'] = array(parse_size($tmp));
+  }

Let's remove this condition.

+++ b/file_entity.pages.incundefined
@@ -31,6 +31,190 @@ function file_entity_view_page($file) {
+  // Change the file status from temporary to permanent.
+  _file_entity_save_file_permenently($file);

Replace this function call with:

$file->status = FILE_STATUS_PERMANENT;

+++ b/file_entity.pages.incundefined
@@ -31,6 +31,190 @@ function file_entity_view_page($file) {
+      file_save($file);
+      _file_entity_save_file_permenently($file);

Change this to:

$file->status = FILE_STATUS_PERMANENT;
file_save($file);

dgorton’s picture

Once the changes in #14 are complete, the patch in #16 from #1394826: Add upload progress to the Upload tab will need to be rerolled against file_entity.

mtift’s picture

I've incorporated Dave's suggestions and re-rolled the patch

mtift’s picture

Status: Needs work » Needs review
dgorton’s picture

Status: Needs review » Reviewed & tested by the community

#16 works for me

Dave Reid’s picture

Status: Reviewed & tested by the community » Fixed

Thanks all! I've committed this to 7.x-2.x with http://drupalcode.org/project/file_entity.git/commit/f050bd7. Nice work! Going to commit #1552920: Move file/add from media to file. to Media now.

Dave Reid’s picture

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