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

Tinymce doesn't need to remove the images basepath #25037

Merged
merged 3 commits into from Jun 5, 2019

Conversation

Projects
None yet
9 participants
@HLeithner
Copy link
Member

commented May 29, 2019

Pull Request for Issue #25015 .

Summary of Changes

Removed the deletion of the image base path from the upload path.

Testing Instructions

c/p from #25015 (comment)

Create a sub-directory of "images" under the main images directory.
Create a sub-directory of "testing123" under that directory.

Update the plugin: Editor - TinyMCE
Make sure you are editing tab "Set 0" so Administrator is showing in the groups that use this profile.
Under the "Images Directory" setting, select "images/testing123" (save & close)

please try all variants

the actual path on the server is "images/images/testing123"
Go under content, and articles and either create a new or edit an existing.

Drag and Drop an image into the content window.

Also change the image and file path in com_media options and try it with different pathes

Expected result

You should not receive an error and see the image.

Actual result

You should receive an error the file can not be uploaded, showing the filename as undefined.

@GCLW

This comment has been minimized.

Copy link

commented May 29, 2019

Tested with above listed parameters and this update worked.
Additional tests:

Set TinyMCE image dir to images (images/images) - Drag and Drop worked.
Set TinyMCE to just a sub-directory under the main images directory - Drag and Drop worked.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25037.

@GCLW

This comment has been minimized.

Copy link

commented May 29, 2019

I have tested this item successfully on e2becdf

Tested with above listed parameters and this update worked.
Additional tests:

Set TinyMCE image dir to images (images/images) - Drag and Drop worked.
Set TinyMCE to just a sub-directory under the main images directory - Drag and Drop worked.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25037.

@Quy

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

I have tested this item successfully on e2becdf


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25037.

@Quy

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25037.

@joomla-cms-bot joomla-cms-bot added the RTC label May 30, 2019

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

The test instructions don’t reflect what that part of the code was supposed to cover. You need to test this after setting a different directory for the images in com_media...

@franz-wohlkoenig

This comment has been minimized.

Copy link
Member

commented May 31, 2019

@dgrammatiko should RTC be removed?

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

@dgrammatiko thx for the info but the parameter in the plugin doesn't add the base path to the return value, so it's not needed anyway or do I missing something in the history?

Updated test instructions.

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

As far as I understand the issue is that if you populate the field in tinymce for the final storage path people expect it to be relative to images but in reality it always wasn’t. Actually a better description for that field would do here (eg if you want a folder something relative to images then type images/something). By the way removing this logic in staging is a B/C break for those not using the root/images path for the media manager (I’m one of those few ppl)

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

You can't enter a path in the plugin I think you created a new fieldtype "imagespath" and you have to select it from the dropdown.

@brianteeman

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

@dgrammatiko is referring to this option in the media manager
image

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

thats the path used in the plugin and is added automatically on upload, not if you set the path in the plugin. ymmv

@dgrammatiko

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

@HLeithner quite honestly I thought that I had introduced that piece of code but looking at the initial commit of the drag and drop it seems that isn't the case, eg:

if ($dragdrop && $user->authorise('core.create', 'com_media'))
{
$allowImgPaste = "true";
$isSubDir = '';
$session = JFactory::getSession();
$uploadUrl = JUri::base() . 'index.php?option=com_media&task=file.upload&tmpl=component&'
. $session->getName() . '=' . $session->getId()
. '&' . JSession::getFormToken() . '=1'
. '&asset=image&format=json';
if (JFactory::getApplication()->isSite())
{
$uploadUrl = htmlentities($uploadUrl, null, 'UTF-8', null);
}
// Is Joomla installed in subdirectory
if (JUri::root(true) != '/')
{
$isSubDir = JUri::root(true);
}
// Get specific path
$tempPath = $this->params->get('path', '');
if (!empty($tempPath))
{
$tempPath = rtrim($tempPath, '/');
$tempPath = ltrim($tempPath, '/');
}
$dragDropPlg = 'jdragdrop';
JText::script('PLG_TINY_ERR_UNSUPPORTEDBROWSER');
JFactory::getDocument()->addScriptDeclaration(
"
var setCustomDir = '" . $isSubDir . "';
var mediaUploadPath = '" . $tempPath . "';
var uploadUri = '" . $uploadUrl . "';
"
);
}
the code doesn't have all these preg/replaces there... (#7435 )
Then looking at the github blame #15057 comes up as the bug source.
Anyways by reviewing the code I think this should work fine, all the extra code there is kinda bloat but please check the case when a user has a diferent media path than the standard images folder

@HLeithner

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

@dgrammatiko ok thx, I added the imagepath property to the test instructions and tested it by my self without any problems for the drag and drop upload.

@Quy

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2019

Tested successfully with a different image root folder.

@rdeutz

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

All JSessionTest are failing because of "Warning: A non-numeric value encountered in C:\projects\joomla-cms\libraries\vendor\joomla\string\src\phputf8\utf8.php on line 38"

I can't tell what is wrong and I am sorry but I don't have the time now to dig into this deeper. Just reporting, this fails only on 7.2

@HLeithner HLeithner merged commit d6439d5 into joomla:staging Jun 5, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
Hound No violations found. Woof!
continuous-integration/drone/pr Build is passing
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@HLeithner HLeithner added this to the Joomla 3.9.7 milestone Jun 5, 2019

@joomla-cms-bot joomla-cms-bot removed the RTC label Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.