Ticket #454 (closed New Feature: fixed)

Opened 7 years ago

Last modified 7 years ago

Unify the browser and upload connectors

Reported by: alfonsoml Owned by: alfonsoml
Priority: Normal Milestone: FCKeditor 2.5 Beta
Component: File Browser Version: SVN (FCKeditor) - Retired
Keywords: Cc:

Description

Now there are two connectors folders for each server language, one for the quick upload and another one for the filemanager.

That means duplicated code and confusion for users because they have enabled only one of them and the other part doesn't work. Not taking into account the problems when a change is done only in one set of folders and the other one isn't updated.

So I propose to use just one set of files and so provide in their configuration files the option to enable or disable now the basic functionality like Upload, Browse files, and then it can be further expanded to enable deletion if the user wants to.

I'll try to work in my branch to make the proper changes to the asp and php connectors and let's see if I'm able to understand other languages.

Change History

comment:1 follow-up: ↓ 11 Changed 7 years ago by alfonsoml

  • Status changed from new to assigned

I've checked [286] that does the unification for the asp connector and at the same time it does bring other features:

Theres a new setting in the config file so the user can choose the possible commands enabled for the connector (for example only browse, but not upload or viceversa)

dim ConfigAllowedCommands
ConfigAllowedCommands = "FileUpload|GetFolders|GetFoldersAndFiles|CreateFolder"

There's a new setting to easily set where should each file type go:

Set ConfigFileTypesPath	= CreateObject( "Scripting.Dictionary" )

ConfigFileTypesPath.Add "File", ConfigUserFilesPath & "file/"

ConfigFileTypesPath.Add "Image", ConfigUserFilesPath & "image/"

ConfigFileTypesPath.Add "Flash", ConfigUserFilesPath & "flash/"

ConfigFileTypesPath.Add "Media", ConfigUserFilesPath & "media/"

That way if a user wants to put everything in the same folder then he just needs to change those four lines, or he can make Files go to "/files/mine/", Image to "/userFiles/images/" and Media to "/userFiles/media/", etc...

When all the changes are complete this tracker will fix several Feature request about this issues.

comment:2 Changed 7 years ago by alfonsoml

  • Component changed from General to File Browser

I've added some other little improvements from SF Patch 1315722 (avoid getting a cached response) and SF Patch 1662181 (bugfix if the form upload has more data).

This is very similar to the ideas proposed in https://sourceforge.net/tracker/index.php?func=detail&aid=1312834&group_id=75348&atid=543655 so I might get the coldfusion changes done more easily and also the java code from https://sourceforge.net/tracker/index.php?func=detail&aid=1462144&group_id=75348&atid=543655

comment:3 Changed 7 years ago by alfonsoml

I've checked in [290] the new php connector, so if anyone else can test these changes that would be great.

I'll try to work on the rest of the connectors, but I won't be able to test them (so if anyone else steps up with the corresponding changes it would be very helpful)

comment:4 Changed 7 years ago by tallyce

NB See also http://dev.fckeditor.net/ticket/306 which I think overlaps with a lot of this stuff. Can you confirm?

comment:5 Changed 7 years ago by alfonsoml

This will cover the Configurability of subdirectories, I'm also checking existing bug reports to try to fix existing problems, but I don't plan to add new features with this bug.

The reason for the new configuration settings are to make it easier the transition for the people that used only the quick upload feature with one common directory: they can disable the get* commands as well as the createFolder, so end users aren't able to do anything that they didn't want. And instead of porting the useFileType configuration from the upload, I thought that it would be better to bring a single solution to all the complains about were to store the files.

Also this way it's quite easy to bring new features like the ability to delete or rename folders and/or files .

Now most of the changes are finished for the asp and php connectors, although I'll add a new command: GetEnabledCommands so in the fileManager the "create folder" and "upload new file" options can be hidden if they have been disabled.

For the php connector I have to check again the utf-8 thing as I was getting weird results yesterday

comment:6 Changed 7 years ago by alfonsoml

#454 and #465 have been marked as dups.

comment:7 Changed 7 years ago by alfonsoml

make that #458

comment:8 follow-up: ↓ 12 Changed 7 years ago by alfonsoml

I've checked in [300] the unified ColdFusion connector, as it didn't have the option to choose in the upload the ability to use or not the file type I didn't mess too much with it (also because my coldfusion skills are non-existent)

This connector seems to allow passing the configuration data from the request string, and that could be a very dangerous thing and I think it should be removed.

comment:9 Changed 7 years ago by alfonsoml

The Perl connector has been checked in [301].

Notes: I've changed the line endings in connector.cgi from PC to Unix as it was generating a 500 error for me, and I've added the upload.cgi to be able to output the javascript messages properly for the quickupload.

There isn't still a config.pl file to change the settings without touching the files all the way.

comment:10 follow-up: ↓ 14 Changed 7 years ago by fredck

You have done a great job Alfonso.

In my previous thoughts about it, I've been thinking about creating a new command called "QuickUpload" for this kind of upload, instead of reusing "FileUpload", which would be reserved for the File Browser. Maybe this separation could bring us ways to handle the upload accordingly, by, for example, handling special configurations for the quick upload. Of course, the upload code would be shared between those commands.

One of the reasons for the command separation it that I was wondering that, for backward compatibility, shouldn't we provide a configuration similar to "UseFileType". So when upgrading, our users will not have many surprises.

comment:11 in reply to: ↑ 1 ; follow-up: ↓ 15 Changed 7 years ago by fredck

Replying to alfonsoml:

Theres a new setting in the config file so the user can choose the possible commands enabled for the connector (for example only browse, but not upload or viceversa)

I would avoid this feature for now because its usage could bring confusion to the end user, as the interface would not reflect the configured settings.

There's a new setting to easily set where should each file type go

This one is cool, as it has been strongly requested previously.

I liked the way you have proposed it initially, by concatenating the "configured base path" with the type path (like ConfigUserFilesPath & "file/" in ASP). But, looking at the sources, you have changed it to "/userfiles/files/", and repeated it for each type.

I would prefer the initial proposal, as it brings a single point of configuration, which would be ok for the majority of users. Then, for those who really need more control for each type, they could go to the specific type and make the changes.

comment:12 in reply to: ↑ 8 Changed 7 years ago by fredck

Replying to alfonsoml:

This connector seems to allow passing the configuration data from the request string, and that could be a very dangerous thing and I think it should be removed.

It it certainly something to me removed.

We just only remember all those things, to list them in the Upgrade Instructions page.

comment:13 Changed 7 years ago by fredck

Please consider #273 in this ticket too to avoid conflict.

comment:14 in reply to: ↑ 10 ; follow-up: ↓ 16 Changed 7 years ago by alfonsoml

Replying to fredck:

You have done a great job Alfonso.

In my previous thoughts about it, I've been thinking about creating a new command called "QuickUpload" for this kind of upload, instead of reusing "FileUpload", which would be reserved for the File Browser. Maybe this separation could bring us ways to handle the upload accordingly, by, for example, handling special configurations for the quick upload. Of course, the upload code would be shared between those commands.

The QuickUpload isn't sending right now any command, so we could name it anyway we like, but due to the fact that it isn't used right now I would wait until we got a clear idea about what should be changed before creating a new command that doesn't provide any new feature.

One of the reasons for the command separation it that I was wondering that, for backward compatibility, shouldn't we provide a configuration similar to "UseFileType". So when upgrading, our users will not have many surprises.

That's why I've added the ConfigFileTypesPath setting: If an installation was using a single folder then it's easy to change that setting for each file type and everything goes as usual, but if they use both the file browser and the quickupload they have the 4 folders created and they can go on knowing that now if they change the folder for upload or browsing that setting will be also used without any other special coding.

comment:15 in reply to: ↑ 11 Changed 7 years ago by alfonsoml

Replying to fredck:

Replying to alfonsoml:

Theres a new setting in the config file so the user can choose the possible commands enabled for the connector (for example only browse, but not upload or viceversa)

I would avoid this feature for now because its usage could bring confusion to the end user, as the interface would not reflect the configured settings.

As I said the aim is to be able to let the developer make it so the users can keep uploading files, but not browsing the server like they had with the 2 connectors, and also now being able to browse but not upload anything, the other options are there because I thought that it was better to do it in a generic way instead of hardcoding a boolean setting only to enable/disable the file browsing. If someone wants to disable other options from the connector he can manually adjust the filebrowser html files as well.

There's a new setting to easily set where should each file type go

This one is cool, as it has been strongly requested previously.

I liked the way you have proposed it initially, by concatenating the "configured base path" with the type path (like ConfigUserFilesPath & "file/" in ASP). But, looking at the sources, you have changed it to "/userfiles/files/", and repeated it for each type.

I would prefer the initial proposal, as it brings a single point of configuration, which would be ok for the majority of users. Then, for those who really need more control for each type, they could go to the specific type and make the changes.

Ok, initially I thought that way, but then I had an urge to do a clean up and keep everything simpler, but it can be done one way or another, there's no problem and I'll revert to the original way in the next checkin

comment:16 in reply to: ↑ 14 ; follow-up: ↓ 17 Changed 7 years ago by fredck

Replying to alfonsoml:

Replying to fredck:

You have done a great job Alfonso.

In my previous thoughts about it, I've been thinking about creating a new command called "QuickUpload" for this kind of upload, instead of reusing "FileUpload", which would be reserved for the File Browser. Maybe this separation could bring us ways to handle the upload accordingly, by, for example, handling special configurations for the quick upload. Of course, the upload code would be shared between those commands.

The QuickUpload isn't sending right now any command, so we could name it anyway we like, but due to the fact that it isn't used right now I would wait until we got a clear idea about what should be changed before creating a new command that doesn't provide any new feature.

Well, that is not exactly true an in "upload.php" you are defining the command which will handle the quick upload (FileUpload).

Second point: there is a substantial difference between the normal upload and the quick upload. In the first case, you will be uploading the file in the "current folder". In the quick upload instead, you will be sending the file to a predefined folder, which can also be out of the File Browser space (previously, by setting UseFileType=false).

Many people prefer to have the Quick Upload pointing outside the File Browser to not mix the well organized files structure of the File Browser, with the "throw anything can" of the quick upload.

So, I believe it is better to handle the quick upload with a dedicated command (QuickUpload), which of course shares the FileUpload code.

Considering the above, maybe it is also a nice idea to have a dedicated setting for it:

$Config['QuickUploadPath']['File'] = '/userfiles/' ;

comment:17 in reply to: ↑ 16 ; follow-up: ↓ 18 Changed 7 years ago by alfonsoml

Replying to fredck:

Replying to alfonsoml:

Replying to fredck:

You have done a great job Alfonso.

In my previous thoughts about it, I've been thinking about creating a new command called "QuickUpload" for this kind of upload, instead of reusing "FileUpload", which would be reserved for the File Browser. Maybe this separation could bring us ways to handle the upload accordingly, by, for example, handling special configurations for the quick upload. Of course, the upload code would be shared between those commands.

The QuickUpload isn't sending right now any command, so we could name it anyway we like, but due to the fact that it isn't used right now I would wait until we got a clear idea about what should be changed before creating a new command that doesn't provide any new feature.

Well, that is not exactly true an in "upload.php" you are defining the command which will handle the quick upload (FileUpload).

Second point: there is a substantial difference between the normal upload and the quick upload. In the first case, you will be uploading the file in the "current folder". In the quick upload instead, you will be sending the file to a predefined folder, which can also be out of the File Browser space (previously, by setting UseFileType=false).

Many people prefer to have the Quick Upload pointing outside the File Browser to not mix the well organized files structure of the File Browser, with the "throw anything can" of the quick upload.

So, I believe it is better to handle the quick upload with a dedicated command (QuickUpload), which of course shares the FileUpload code.

Considering the above, maybe it is also a nice idea to have a dedicated setting for it:

$Config['QuickUploadPath']['File'] = '/userfiles/' ;

I defined the "FileUpload" command in the upload files to have the code as most similar as possible to the connector file, and that way check with the same code if the uploading of files is enabled or not. In comment 14 I meant that the Html form isn't sending any command right now, and if the behavior of both commands is gonna be the same then we could avoid adding it until there's a need that justify it.

I didn't thought that there were people that would rather have the quickuploaded files in a folder outside the browser, I only thought that there were two user cases:

  • Don't allow to browse the server, so the files are uploaded to a common directory and there's no need for the subfolders
  • The file browser is enabled, if a file is quick uploaded it should be possible to browse it at a later time.

Do you think that if we enable by default (or properly document how to do it) to a SUBfolder with just the FCKConfig.LinkUploadURL setting should be enough for those people?
I mean: the new code should be able to handle the CurrentFolder parameter in the quick upload, so the LinkUploadURL can be set to upload to a folder "/uploads/" or the like, without adding another configuration in the connectors themselves.

I'm suggesting this approach because in PHP adding a Config['QuickUploadPath'] setting for each type will need another setting also for the mapped folder: Config['QuickUploadAbsolutePath'], and that's 8 settings, and the code to get the current folder now will have to take care of the issued command to get it right.

comment:18 in reply to: ↑ 17 ; follow-up: ↓ 24 Changed 7 years ago by fredck

Replying to alfonsoml:

I didn't thought that there were people that would rather have the quickuploaded files in a folder outside the browser

Just to exemplify, I've saw cases where the folder for quick upload had hundreds of images, while the File Browser was used only for well organized enterprise docs, logos, etc...

Other than that, we should be backward compatible "by default", so the quick upload should send the files to "/usefiles/", while the File Browser will be working at "/userfiles/file/" and so on. Drastic changes can be thought for version 3.0, but not in a minor version.

Do you think that if we enable by default (or properly document how to do it) to a SUBfolder with just the FCKConfig.LinkUploadURL setting should be enough for those people?
I mean: the new code should be able to handle the CurrentFolder parameter in the quick upload, so the LinkUploadURL can be set to upload to a folder "/uploads/" or the like, without adding another configuration in the connectors themselves.

This is an error we've done in the past and we should not repeat it anymore: no configurations must go in the file browser URLs. Thinks must be done in the server, for security.

I'm suggesting this approach because in PHP adding a Config['QuickUploadPath'] setting for each type will need another setting also for the mapped folder: Config['QuickUploadAbsolutePath'], and that's 8 settings,

It is better to give full control to developers. It will not be a big issue, as we will have the main variable (/userfiles/) which can be used to easily change the base path for all settings. Then, for those few developers who need more precise control, they can go to the fine grained settings.

and the code to get the current folder now will have to take care of the issued command to get it right.

Your affirmation justifies the need of the "QuickUpload" command.

comment:19 Changed 7 years ago by alfonsoml

I've added the QuickUploadPath setting and so I had to change all the places where a call to ServerMapFolder or similar was done to also specify the current command.

Just to clarify something about the previous comment: I was suggesting to use the current CurrentFolder parameter, that shouldn't bring any new security issues because that parameter is already used for the file browser commands, and the only difference would be that the quickuploaded files would end up in \userfiles\file\upload\ without any other change to the code, because due to the changes that I did it does already work for the quickupload as it does for the normal upload.

comment:20 Changed 7 years ago by fredck

  • Milestone set to FCKeditor 2.5

comment:21 Changed 7 years ago by martinkou

The following SF report has been market as duplicate of this: http://sourceforge.net/tracker/index.php?func=detail&aid=1254053&group_id=75348&atid=543656

comment:22 Changed 7 years ago by alfonsoml

Fixed on trunk with [413]

I'll leave this open until I update the wiki and make sure that everything is fine

comment:23 Changed 7 years ago by alfonsoml

  • Status changed from assigned to closed
  • Resolution set to fixed

I've updated the wiki, closed all the related bugs that I could find and disabled by default the connectors, I think that now this is over.

comment:24 in reply to: ↑ 18 Changed 7 years ago by tallyce

Other than that, we should be backward compatible "by default", so the quick upload should send the files to "/usefiles/", while the File Browser will be working at "/userfiles/file/" and so on. Drastic changes can be thought for version 3.0, but not in a minor version.

I think what is most important is actually to get things fully correct, even though this is a point version - there seem to have been so many problems with the legacy connectors, which your work seems now to have fixed, that it would be good for people to have to take the opportunity to configure them well.

comment:25 Changed 7 years ago by tallyce

  • Status changed from closed to reopened
  • Resolution fixed deleted

I haven't had a chance to download the new files, but on the face of it this seems like a very extensive set of extremely useful work - well done, and very many thanks.

Is there any possibility to get in the regexp feature noted at http://dev.fckeditor.net/ticket/306 before these connectors get released? Certainly in the PHP case that functionality is fairly easy to implement - just search for 'regexp' in http://dev.fckeditor.net/attachment/ticket/306/phpconnector-diff-v241-v1-tallyce.txt and you'll see how it's relatively easy to implement.

(Obviously I would also be keen to see some configurability of what happens when a file of the same name is uploaded, as per ticket #306 again, but I realise that's probably more specialised. However I do think regexp validation is a sufficiently generic requirement.)

comment:26 Changed 7 years ago by alfonsoml

  • Status changed from reopened to closed
  • Resolution set to fixed

That regexp would be very problematic, it means that as soon as a user tries to upload a file with a character no in the regexp the upload would fail and the user will wonder what's going on.

It's much better to do it in the way that it's proposed in https://sourceforge.net/tracker/index.php?func=detail&aid=1209255&group_id=75348&atid=543655 but even then I don't think that's the ideal solution because it means that the uploaded files may have lots of underscores depending on the users locale.

Anyway that's outside the scope of this patch.

comment:27 Changed 7 years ago by tallyce

  • Status changed from closed to reopened
  • Resolution fixed deleted

That regexp would be very problematic, it means that as soon as a user tries to upload a file with a character no in the regexp the upload would fail and the user will wonder what's going on.

I think the original patch I proposed in ticket #306 could include an additional error code so that the user knows there is a filename mismatch.

But, aside from the issue of the specific regexp I suggested, could I again ask that regexp checking be a supported option, defaulting to false (or an empty string)? Again, I do think this should be relatively easy to implement, as the patch submitted demonstrates. It's very similar to the file extension checking mechanism.

(I've reopened this ticket, just so the suggestion isn't lost.)

comment:28 Changed 7 years ago by alfonsoml

  • Status changed from reopened to closed
  • Resolution set to fixed

This patch is about unifying the connectors and the auxiliary changes so everybody could keep on using them the way they did.

Any work on a regexp will be done in https://sourceforge.net/tracker/index.php?func=detail&aid=1209255&group_id=75348&atid=543655 or in another tracker, not here.

Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy