Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #3139 Attachment: a bug and a mod to allow multi-attachment selection #3180

Closed
wants to merge 2 commits into from
Closed

Conversation

Sama34
Copy link
Contributor

@Sama34 Sama34 commented Apr 30, 2018

Fixes #3139 Attachment: a bug and a mod to allow multi-attachment selection

Please review.

@effone effone added the b:1.8 Branch: 1.8.x label Apr 30, 2018
@SvePu
Copy link
Contributor

SvePu commented May 14, 2018

@Sama34 Thx for this nice fix, but you'll get a MySql syntax error on editpost because $attachwhere is empty

@Eldenroot
Copy link
Contributor

@SvePu - thx for your input!
@Sama34 - good job! Better and cleaner approach than mine was 👍 can you please fix last issue?

@SvePu
Copy link
Contributor

SvePu commented May 18, 2018

@Sama34 - also I get an empty($_POST) error when summary of all selected file sizes is greater than php post_max_size. May be we need a size check before start uploading

Could someone confirm this?

Copy link
Member

@euantorano euantorano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change needed with the template version, and would be good if we could get the edit post issue. I'm not sure about the best way to handle the other issue mentioned by @SvePu - a size check might be easiest.

@@ -9459,7 +9452,7 @@ if(use_xmlhttprequest == "1")
</tr>]]></template>
<template name="post_attachments_new" version="1807"><![CDATA[<tr>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This template needs the version bumping to 1816.

@SvePu
Copy link
Contributor

SvePu commented Jun 2, 2018

@Sama34
I fixed the issue by replacing in editpost.php:

$ret = add_attachments($pid, $forumpermissions, $attachwhere, "editpost");

with:

$ret = add_attachments($pid, $forumpermissions, "pid='{$pid}'", "editpost");

@euantorano
I use some litte JavaScript for this size check - if JavaScript is disabled in browser the multiupload option will be changed into a single upload.

@euantorano
Copy link
Member

@SvePu That sounds like a good option to me.

@SvePu
Copy link
Contributor

SvePu commented Jun 2, 2018

Maybe someone with more knowledge in JavaScript could integrate something like this into jscripts/post.js
The vars for max_upload_size and post_max_size we could get over:

ini_get('upload_max_filesize');

and

ini_get('post_max_size');

@Eldenroot
Copy link
Contributor

@Sama34 - can you please incorporate fixes above? I tested this PR and seems to be fine except these two things. Thank you'

@Ben-MyBB
Copy link
Member

Ben-MyBB commented Jun 5, 2018

@Sama34 ping

@effone
Copy link
Member

effone commented Jun 8, 2018

Can you please look at the PR @Sama34 ? Seems to be the last PR for 1.8.16 so far ...

@Eldenroot
Copy link
Contributor

Or is there anybody who can merge this and add the requested chnages in next PR?

@effone
Copy link
Member

effone commented Jun 8, 2018

May be @SvePu can create an extended part of this and then we can merge both back to back.
He has already worked on it. Studying the whole PR for extending will take time for me.

@SvePu
Copy link
Contributor

SvePu commented Jun 9, 2018

@effone
I use this small JS inline script to check upload size and file count before fire up the upload. Maybe someone could integrate something like this into post.js to call it on click at submit button.

<script type="text/javascript">	
	$(function(){
		document.getElementById("file").setAttribute("multiple","multiple");
		$("input[type='submit']").click(function(){
			var fileUpload = $("input[type='file']");
			if (parseInt(fileUpload.get(0).files.length)>{$php_max_file_uploads}){
				alert("Du kannst maximal {$php_max_file_uploads} Dateien gleichzeitig hochladen - Führe wenn nötig mehrere Durchgänge aus.");
				document.getElementById("file").value="";
				return false;
			}
			var totalSize = 0;
			fileUpload.each(function() {
				for (var i = 0; i < this.files.length; i++) {
					totalSize += this.files[i].size;
				}
			});
			var valid = totalSize <= {$php_max_upload_size};
			if (!valid) {
				alert('Die hochzuladenen Dateien haben eine Gesamtgröße über {$php_max_upload_size_mb} - bitte führe wenn nötig mehrere Durchgänge aus.');
				document.getElementById("file").value="";
				return false;
			}
		});
	});
</script>

Sorry....the alert info is in German language ;)

@Eldenroot
Copy link
Contributor

Is there anybody who can make a new PR with JS script above? I would like to but my JS knowledge is almost zero :(

@euantorano
Copy link
Member

@Eldenroot This PR won't be making it into 1.8.16 unfortunately, it'll have to wait until .17.

@Eldenroot
Copy link
Contributor

I use this "new feature" on my board (without JS part), I applied changed from SvePu, working fine. Is there anybody who can take care of JS part and add it inside post.js? @Sama34

@effone effone added the s:conflict Status: Conflict. Solution incompatible with current code label Jul 22, 2018
@euantorano
Copy link
Member

Superseded by #3373.

@euantorano euantorano closed this Aug 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
b:1.8 Branch: 1.8.x s:conflict Status: Conflict. Solution incompatible with current code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants