Redmine

Hongjoo Lee wrote:

After setting maximum attachment size to 0, with expecting unlimited size of attachment (expected behavior), updating issue with attachment seems to upload file, but actually it doesn't make a link to the file.

This is the expected behavior. When setting "Maximum attachment size" is set to 0 (which is actually 0 KB) you deny any bits and bytes to be uploaded.

Here's steps to reproduce.

[...]
4. progress bar shows for awhile (seems file is uploaded)
[...]

The files are not only not-linked but also not uploaded in this case. The progress bar shows only while it is requesting permission to upload (store) the file. When this is denied, as in this case due to setting "Maximum attachment size" to 0, file upload attempt is canceled and Not Found message is rendered.

Mischa The Evil wrote:

Hongjoo Lee wrote:

After setting maximum attachment size to 0, with expecting unlimited size of attachment (expected behavior), updating issue with attachment seems to upload file, but actually it doesn't make a link to the file.

This is the expected behavior. When setting "Maximum attachment size" is set to 0 (which is actually 0 KB) you deny any bits and bytes to be uploaded.

No problem with how expected behavior defined in redmine. However, set to 0 means unlimited in general and there are many cases in the same sense. In this point of view, my expectation was unlimited size of attachment.

Here's steps to reproduce.

[...]
4. progress bar shows for awhile (seems file is uploaded)
[...]

The files are not only not-linked but also not uploaded in this case. The progress bar shows only while it is requesting permission to upload (store) the file. When this is denied, as in this case due to setting "Maximum attachment size" to 0, file upload attempt is canceled and Not Found message is rendered.

presenting clip icon and file name looks as if the file uploaded. Also "NOT FOUND" message after showing process bar causes some misapprehension.

In my opinion, a message showing size limit exceeded should come out in this case and keep consistency. It would be better than unusual behavior.

Hongjoo Lee wrote:

presenting clip icon and file name looks as if the file uploaded. Also "NOT FOUND" message after showing process bar causes some misapprehension.

In my opinion, a message showing size limit exceeded should come out in this case and keep consistency. It would be better than unusual behavior.

You are right. With setting "Maximum attachment size" set to 0 the regular notice of exceeding the max size isn't displayed and file upload is triggered with the following error stack as a result:

Started POST "/uploads.js?attachment_id=1&filename=CPtoRM-List.txt" for xxx.xxx.xxx.xxx at 2013-05-06 05:05:46 +0200
Processing by AttachmentsController#upload as JS
  Parameters: {"attachment_id"=>"1", "filename"=>"CPtoRM-List.txt"}
  Current user: Mischa (id=1)
  Rendered attachments/upload.js.erb (3.9ms)
Completed 500 Internal Server Error in 65ms

ActionController::RoutingError (No route matches {:action=>"show", :controller=>"attachments", :attachment_id=>"1", :format=>"js", :id=>#<Attachment id: nil, container_id: nil, container_type: nil, filename: "CPtoRM-List.txt", disk_filename: "", filesize: 2578, content_type: "", digest: "", downloads: 0, author_id: 1, created_on: nil, description: nil, disk_directory: nil>}):
  app/views/attachments/upload.js.erb:7:in `_app_views_attachments_upload_js_erb__187147715116926503_58328920'
  app/controllers/attachments_controller.rb:90:in `upload'

1. Fix handling of attachment uploads when "Maximum attachment size" is set to 0, and/or
2. Provide a way to set "Maximum attachment size" to unlimited

Regarding 1: I've tried to find the cause of this but haven't succeeded. It seems the filesize-validation should cover the 0 value correctly looking at source:/trunk/app/models/attachment.rb#L66 unless the .to_i.kilobytes is not handled correctly with a 0 value¹:

  def validate_max_file_size
    if @temp_file && self.filesize > Setting.attachment_max_size.to_i.kilobytes
      errors.add(:base, l(:error_attachment_too_big, :max_size => Setting.attachment_max_size.to_i.kilobytes))
    end
  end

¹ though, these methods are provided by the Rails framework instead of by Redmine

I totally agree with you at the point of splitting this issue into two parts.

And I just tried to do it, but seems configuration have change since the last time I made an my old report and cannot produce any sub-issues or related-issues anymore.

Anyway, "fix handling of attachment uploads when "Maximum attachment size" is set to 0" as bug, and "provide a way to set unlimited attachment size" as a new feature will be made.

Just created a new issue #14022 for new feature,
I cannot modify the title of this issue but let's deal this is as a matter of "fix handling of attachment uploads when 'Maximum attachment size' is set to 0".

BTW, the upload field should not even be displayed if the maximum size is set to 0, what do you think?

Jean-Philippe: it depends on #14022, which basically says "0 should mean unlimited size". In this case we shouldn't hide the field. For now 0 equals 0, so we should probably hide the field.

Jean-Baptiste Barth wrote:

Jean-Philippe: it depends on #14022, which basically says "0 should mean unlimited size". In this case we shouldn't hide the field. For now 0 equals 0, so we should probably hide the field.

I'd like to keep the ability to disable uploads by setting the max size to 0. -1 would be an option for unlimited size.

Jean-Philippe Lang wrote:

This is now fixed by r11841 and r11842.

Confirmed. Thanks.

BTW, the upload field should not even be displayed if the maximum size is set to 0, what do you think?

I agree.

I'd like to keep the ability to disable uploads by setting the max size to 0. -1 would be an option for unlimited size.

I think that that is indeed the best way to go when an the unlimited size option is getting implemented (as per #14022).

Jean-Philippe Lang wrote:

BTW, the upload field should not even be displayed if the maximum size is set to 0, what do you think?

I think it is common use in software configuration to set 0 to deactivate a limit, as Hongjoo actually thought it was, -1 is more often used behind the scenes by programmers.

Also, I don't think there exists an use case for totally disabling upload and if there was, this should be done using permissions. IMHO, it's not worth it dealing with extra code for this purpose.

Jean-Baptiste Barth wrote:

Jean-Philippe: it depends on #14022, which basically says "0 should mean unlimited size". In this case we shouldn't hide the field. For now 0 equals 0, so we should probably hide the field.

As I mentioned (thanks for Etienne's agreement), 0 is more generally used for meaning unlimited and -1 means sort of exceptional case having broad meaning comparing to 0. Actually, both cases are not often shown on UI setting but appear in text configuration such as INI or XML files. However, "0 means unlimited" sometimes have seen in UI configuration, having size input form with short guide, "set to 0 for unlimited".

We can take it in this way. The feature, "file attachment" may firstly divided into two cases, activated and deactivated. Then, activated branch can have sub-branches of limited and unlimited, and may have authorization/permission branch in future for the case of deactivating file attachment. And now we are discussing about activated-unlimited-fileattachment case and I claimed that the feature #14022 is suitable for that case. Hence, I think we don't need to deal with deactivating file attachment case here and that should be dealt separately. Of course deactivation can be easily represented by setting the field to -1 or 0 and it make us hesitate to choose better way, but sooner or later it must be conflicted to other sub cases such as permission.

In summary, let's talk about deactivation separately when it is needed and issued. And let's move on to unlimited file attachment with #14022.