Editor Paste / Drag-drop Image Handler #4359

effone · 2021-04-18T12:13:44Z

Resolves #2440, resolves #4078

Overrules #4080

…hardcoded js to js file

effone · 2021-04-19T20:57:09Z

Questions:

~~CHMOD new upload directory?~~ DONE
~~Backend upload errors need to be displayed? (needs addition of language strings)~~ Silently skipped
~~Shall I update SCE to 3.0? Since the PR is already big?~~ DVZ waved head side-by-side
~~Rename the js file to avoid old template conflict?~~ Renamed. Open to discuss further
~~New settings not required to push through upgrade script? (I guess, no...)~~ sync_settings( ); will take care of it

@mybb/reviewers

Eldenroot · 2021-04-25T15:54:52Z

Nice, good job! Briefly tested! Would be nice to get it together with #4195 into 1.8.27 :) SCeditor needs some love!

@laird - could you test it please?

lairdshaw · 2021-05-06T15:21:57Z

Nice, good job! Briefly tested! Would be nice to get it together with #4195 into 1.8.27 :) SCeditor needs some love!

Agreed, @Eldenroot - it's excellent work, and it would be great if we could get this in to 1.8.27 at the last minute.

could you test it please?

Yes, for sure. I was away for a few days (oh, and you left out my surname in the mention - poor old Laird Popkin from Atlanta, Georgia must be feeling quite confused!) but have now tested this PR and have been in contact with @effone about it. There are a few minor niggles to sort out, but generally it seems to work as advertised, which will be a relief to so many of our users who have been caught out by this missing feature. My list of minor niggles is:

There's a missing require_once of inc/functions_post.php in the new code in xmlhttp.php (confirmed by @effone, who I think is going to update the PR).
The existence of the same problems with troublesome treatment of (ambiguity around) absolute versus relative paths for the new postembedpath setting as we tried to solve for the uploadspath setting in PR Fixes #4338 / #3754 - Relative/absolute path #4342 (not yet discussed with @effone). (ETA: Perhaps the easiest way to resolve these problems is simply to stipulate that the setting must be a relative path. This would ensure, too, re my comment below, that image files always can be served out of this directory).
Errors being returned by the new code in xmlhttp.php as boolean true rather than as a human-readable string, and those errors not yet being displayed in any way in the browser - currently, the Javascript code has a comment "jGrowl it?", to which I think the answer is emphatically "Yes, definitely, let's do that".

I'm also not sure whether serving the image files directly out of the postembedpath directory (by default a sub-directory of uploads) is such a good idea, but maybe others can comment on that (not yet discussed with @effone).

@effone also made a good case to me privately for renaming jscripts/bbcodes_sceditor.js, the only unanswered question in his comment above, so perhaps we can quickly get that and the above sorted and then hopefully merge this very nice PR in time for 1.8.27.

dvz · 2021-05-10T22:46:11Z

Would it be better to handle uploaded files as regular attachments? That would make it an interface improvement without introducing new kinds of uploads and locations watch out for. The editor field and attachment section could then be merged in the future.

With that behavior, we can just add server/client-side hooks to allows plugins to intercept such uploads (and insert external [img] links, etc.). This would make it easier to implement new platforms without modifying the core, and MyBB wouldn't include references to any specific platforms.

effone · 2021-05-11T01:28:11Z

As has been clarified before also, embeds are not attachments. They should not be treated as one and should not impact on allotted quota either.
They should not be saved as .attach and they are not supposed to be bound as per admin's choice to get impacted to profile eligibility as attachment or free from restrictions if admin opts for an external API.
My approach is justified as per my opinions.

Regarding other platforms it has been discussed with @euantorano and agreed upon that I can approach towards third party upload APIs

Sama34

Uploads path should be updated to use absolute path.
Imgur and Imgbb API settings should be spliced.
I think this plugin should somehow mask image links, rather than disclose the full path like in [img]http://localhost/mybb/./uploads/postembeds/1_1620719162_blob.png[/img]
For privacy purposes images should only be served if the user has permission to the post being seen. It isn't necessary to ship all these features in, but having a file to serve the images could make it easier for future improvements.
There are no image manipulation settings, like maximum dimensions, size, or resize features.
I think the feature should be disabled by default, or rather well reported in the release blog.

I approve this because it seems to work as is, but please consider my suggestions above.

Sama34 · 2021-05-11T07:57:21Z

inc/tasks/postembedscleanup.php

+	global $mybb, $db, $lang, $plugins;
+
+	$dir = trim($mybb->settings['postembedpath'], './');
+	$files = glob(MYBB_ROOT."{$dir}/*.{jpg,jpeg,png}", GLOB_BRACE);


Won't this be problematic for large amount of files? Will the task always fulfill properly regardless of the amount of images? I'm a little surprised by the lack of a database table to control uploaded images, which limits the addition of some ACP interface to manage these unless, for example, being somehow merged with the current attachments tools later on.

The concern is correct at I have not done a performance benchmarking.
Am open to accept any suggestion to improve this. I also felt the necessity of a dedicated table for upload entries. Didn't follow that way as that can be considered as Big Change leading to rejection of this PR.

effone · 2021-05-11T11:11:00Z

Uploads path should be updated to use absolute path.

That will be requiring a proxy (see comment below)

Imgur and Imgbb API settings should be spliced.

Hmm. OK. But why it is problematic as what it is now? Admins gonna use either of the services anyway

I think this plugin should somehow mask image links, rather than disclose the full path like in [img]http://localhost/mybb/./uploads/postembeds/1_1620719162_blob.png[/img]

A considerable point from the privacy point of view.

For privacy purposes images should only be served if the user has permission to the post being seen. It isn't necessary to ship all these features in, but having a file to serve the images could make it easier for future improvements.

A good point, and that file can proxy the images as well, supporting absolute paths. But this will exhaust this PR further, anyway.

There are no image manipulation settings, like maximum dimensions, size, or resize features.

As said, this is the basic PR. Further possibilities are there to enhance it. Can be done in this PR as well, if agreed upon.

I think the feature should be disabled by default, or rather well reported in the release blog.

So you propose an additional setting for the behavior introduced? Well, that can be done as well. I guess @dvz was also asking for something similar...

Sama34 · 2021-05-11T20:47:22Z

I think you need to add some hooks where possible. Also, I did try to disable the forum setting Yes, allow [img] code in posts (requires MyCode to be turned on) to se what happened, but apparently the editor stopped working. Is the following piece of code correct?

<script type="text/javascript">
var editorElm = "#message",
	sourceMode = 0,
	partialMode = 0,
	editorLang = (function ($) {
$.sceditor.locale["mybblang"] = {
	"Bold": "Bold",
	"Italic": "Italic",
	"Underline": "Underline",
	"Strikethrough": "Strikethrough",
	"Subscript": "Subscript",
	"Superscript": "Superscript",
	"Align left": "Align left",
	"Center": "Center",
	"Align right": "Align right",
	"Justify": "Justify",
	"Font Name": "Font Name",
	"Font Size": "Font Size",
	"Font Color": "Font Color",
	"Remove Formatting": "Remove Formatting",
	"Cut": "Cut",
	"Your browser does not allow the cut command. Please use the keyboard shortcut Ctrl/Cmd-X": "Your browser does not allow the cut command. Please use the keyboard shortcut Ctrl/Cmd-X",
	"Copy": "Copy",
	"Your browser does not allow the copy command. Please use the keyboard shortcut Ctrl/Cmd-C": "Your browser does not allow the copy command. Please use the keyboard shortcut Ctrl/Cmd-C",
	"Paste": "Paste",
	"Your browser does not allow the paste command. Please use the keyboard shortcut Ctrl/Cmd-V": "Your browser does not allow the paste command. Please use the keyboard shortcut Ctrl/Cmd-V",
	"Paste your text inside the following box:": "Paste your text inside the following box:",
	"PasteText": "Paste Text",
	"Numbered list": "Numbered list",
	"Bullet list": "Bullet list",
	"Undo": "Undo",
	"Redo": "Redo",
	"Rows:": "Rows:",
	"Cols:": "Cols:",
	"Insert a table": "Insert a table",
	"Insert a horizontal rule": "Insert a horizontal rule",
	"Code": "Code",
	"Width (optional):": "Width (optional):",
	"Height (optional):": "Height (optional):",
	"Insert an image": "Insert an image",
	"E-mail:": "E-mail:",
	"Insert an email": "Insert an email",
	"URL:": "URL:",
	"Insert a link": "Insert a link",
	"Unlink": "Unlink",
	"More": "More",
	"Insert an emoticon": "Insert an emoticon",
	"Video URL:": "Video URL:",
	"Video Type:": "Video Type:",
	"Insert": "Insert",
	"Insert a YouTube video": "Insert a YouTube video",
	"Insert current date": "Insert current date",
	"Insert current time": "Insert current time",
	"Print": "Print",
	"View source": "View source",
	"Description (optional):": "Description (optional):",
	"Enter the image URL:": "Enter the image URL:",
	"Enter the e-mail address:": "Enter the e-mail address:",
	"Enter the displayed text:": "Enter the displayed text:",
	"Enter URL:": "Enter URL:",
	"Enter the YouTube video URL or ID:": "Enter the YouTube video URL or ID:",
	"Insert a Quote": "Insert a Quote",
	"Invalid YouTube video": "Invalid YouTube video",
	"Dailymotion": "Dailymotion",
	"MetaCafe": "MetaCafe",
	"Mixer": "Mixer",
	"Vimeo": "Vimeo",
	"Youtube": "Youtube",
	"Facebook": "Facebook",
	"LiveLeak": "LiveLeak",
	"Insert a video": "Insert a video",
	"PHP": "PHP",
	"Maximize": "Maximize"
}})(jQuery);,
	postEmbed = { enabled: 1, host: "local", clientKey: "" },
	optEditor = {
		style: "http://localhost/mybb/jscripts/sceditor/styles/jquery.sceditor.mybb.css?ver=1823",
		rtl: 0,
		emoticonsEnabled: true,
		emoticons: {
			dropdown: { ":)": "http://localhost/mybb/images/smilies/smile.png",";)": "http://localhost/mybb/images/smilies/wink.png",":cool:": "http://localhost/mybb/images/smilies/cool.png",":D": "http://localhost/mybb/images/smilies/biggrin.png",":P": "http://localhost/mybb/images/smilies/tongue.png",":rolleyes:": "http://localhost/mybb/images/smilies/rolleyes.png",":shy:": "http://localhost/mybb/images/smilies/shy.png",":(": "http://localhost/mybb/images/smilies/sad.png",":at:": "http://localhost/mybb/images/smilies/at.png",":angel:": "http://localhost/mybb/images/smilies/angel.png",":@": "http://localhost/mybb/images/smilies/angry.png",":blush:": "http://localhost/mybb/images/smilies/blush.png",":s": "http://localhost/mybb/images/smilies/confused.png",":dodgy:": "http://localhost/mybb/images/smilies/dodgy.png",":exclamation:": "http://localhost/mybb/images/smilies/exclamation.png",":heart:": "http://localhost/mybb/images/smilies/heart.png",":huh:": "http://localhost/mybb/images/smilies/huh.png",":idea:": "http://localhost/mybb/images/smilies/lightbulb.png",":sleepy:": "http://localhost/mybb/images/smilies/sleepy.png",":-/": "http://localhost/mybb/images/smilies/undecided.png", }, // Emoticons to be included in the dropdown
			more: { ":cry:": "http://localhost/mybb/images/smilies/cry.png",":sick:": "http://localhost/mybb/images/smilies/sick.png",":arrow:": "http://localhost/mybb/images/smilies/arrow.png",":my:": "http://localhost/mybb/images/smilies/my.png", }, // Emoticons to be included in the more section
			hidden: {  } // Emoticons that are not shown in the dropdown but will still be converted. Can be used for things like aliases
		},
		toolbar: "bold,italic,underline,strike|left,center,right,justify|font,size,color,removeformat|horizontalrule,image,email,link,unlink|video,emoticon|bulletlist,orderedlist|code,php,quote|maximize,source"
	};
</script>
<script type="text/javascript" src="http://localhost/mybb/jscripts/editor.js?ver=1827"></script>

I think (jQuery);, is wrong.

Sama34 · 2021-05-11T20:54:05Z

Hmm. OK. But why it is problematic as what it is now? Admins gonna use either of the services anyway

We have followed this reasoning before, mainly to avoid confusion and keep both data fields stored in case the admin wants to toggle the service being used. It is not a big deal, if somebody else thinks it isn't worth it.

So you propose an additional setting for the behavior introduced?

I don't think @dvz did mean this in his comment above (which I also brought before), what I meant is that this feature being enabled by default could be problematic for admins when they update their boards, as some even disable the attachment system.

Also, although you seem to be using the core upload functions, I'm unsure your code will work with CDN settings, as at least I noticed you use $mybb->settings['bburl'] directly completely ignoring the possibility of $mybb->asset_url.

Sama34 · 2021-05-11T21:06:50Z

Just recalled, I think this should be shipped along the docs update required, if not for the feature description indeed for the new uploads path in the install and configuration section (permissions doc, etc).

Temporarily prevent drag-n-drop of images into the editor until PR mybb#4359 is updated and included in 1.9.

effone added 7 commits April 18, 2021 17:39

Include editor event handler addon

256edc8

Create settings

348001a

Simplify codebuttons template, var naming standard, move templated / …

17bae90

…hardcoded js to js file

Add paste / drag-drop event handler

74d32d3

Adhere settings

7119e63

Backend upload handler

6c827a5

Syntax error correction: 5.3 compatibility

bc8b4e2

effone marked this pull request as ready for review April 19, 2021 20:36

effone marked this pull request as draft April 20, 2021 16:22

effone added 5 commits April 20, 2021 22:27

Postembed image upload directory and setting

9127839

CHMOD checks

c6a3e62

Path correction

7293031

Typo

07d8609

Create task to cleanup orphan post embed images

d8584e1

effone marked this pull request as ready for review April 21, 2021 20:08

andrewjs18 mentioned this pull request May 4, 2021

Added post about MyBB 1.8.27 mybb/blog.mybb.com-drafts#25

Merged

effone added 4 commits May 11, 2021 02:07

Restore Editor to global scope

fa36856

Include missing functions script

e93e579

Rename editor bridge to avoid upgrade conflicts

586101f

Relative path requirement in description

d6709f6

Sama34 approved these changes May 11, 2021

View reviewed changes

lairdshaw added a commit to lairdshaw/mybb that referenced this pull request May 12, 2021

Inhibit drag-n-drop in the editor

12d0be6

Temporarily prevent drag-n-drop of images into the editor until PR mybb#4359 is updated and included in 1.9.

lairdshaw mentioned this pull request May 12, 2021

Inhibit drag-n-drop in the editor #4368

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor Paste / Drag-drop Image Handler #4359

Editor Paste / Drag-drop Image Handler #4359

effone commented Apr 18, 2021 •

edited

effone commented Apr 19, 2021 •

edited

Eldenroot commented Apr 25, 2021 •

edited

lairdshaw commented May 6, 2021 •

edited

dvz commented May 10, 2021

effone commented May 11, 2021 •

edited

Sama34 left a comment •

edited

Sama34 May 11, 2021

effone May 11, 2021 •

edited

effone commented May 11, 2021

Sama34 commented May 11, 2021

Sama34 commented May 11, 2021

Sama34 commented May 11, 2021

Editor Paste / Drag-drop Image Handler #4359

Are you sure you want to change the base?

Editor Paste / Drag-drop Image Handler #4359

Conversation

effone commented Apr 18, 2021 • edited

effone commented Apr 19, 2021 • edited

Eldenroot commented Apr 25, 2021 • edited

lairdshaw commented May 6, 2021 • edited

dvz commented May 10, 2021

effone commented May 11, 2021 • edited

Sama34 left a comment • edited

Choose a reason for hiding this comment

Sama34 May 11, 2021

Choose a reason for hiding this comment

effone May 11, 2021 • edited

Choose a reason for hiding this comment

effone commented May 11, 2021

Sama34 commented May 11, 2021

Sama34 commented May 11, 2021

Sama34 commented May 11, 2021

effone commented Apr 18, 2021 •

edited

effone commented Apr 19, 2021 •

edited

Eldenroot commented Apr 25, 2021 •

edited

lairdshaw commented May 6, 2021 •

edited

effone commented May 11, 2021 •

edited

Sama34 left a comment •

edited

effone May 11, 2021 •

edited