Client and Server parts separation. Better API support.

[psolom]

Hello there.
Thanks to the explanations and ideas of @jlaustill and other contributors. I have made a number of modifications at the client side and PHP connector (API description will be adapted as well). The changes came from the point that FM client-side should be separated from server-side (connector) as much as possible. The lower conjuction the better. All developers and maintainers must follow this way while working on their connectors. The list of changes you can see below.

• server part is separated from client part to a greater extent;
• removed fileConnector option from server-side;
• removed 'Preview' and 'Thumbnail' params from getinfo response;
• all preview paths (images, media, office files) are now associated to connector. Absolute paths for icons exclusively;
• all preview paths are now built at the client-side (connector-independent) for better API support;
• added support of seeking for media files in PHP connector (including S3 storage plugin);
• PHP connector refactored, following OOP practices;
• added icons for OpenOffice files;

One minor thing I also want to change is to rename fileConnector to apiConnector option in config file, or something like that. The name should better reflect the meaning. Currently it's hard to understand what fileConnector is intended for from its title. Please, share your suggestion, if you have the other one for this option.

Also I suppose normalizePath function could be removed from JS file because now connector should return relative path solely. But before do this I want to hear @gmkll, @fabriceci opinions on this, because normalizePath was implemented initially for Java connector needs.

@gmkll, @fabriceci, @jlaustill
Be aware of these modifications and welcome to discussion.

@gmkll, please update your PR to the last changes and will merge it.

[jlaustill]

@servocoder this all sounds great to me! apiConnector seems like a good name to me, but if anyone has a better suggestion I'm game for that too.

If it's ok with you, I'm going to add a bit to the API documentation on the wiki this week as I work through these as well. I had to do quite a bit of trial and error to figure some stuff out, then I realized it would be good to just add it to the documentation to make developers working on other connectors lives easier(hopefully anyways).

[psolom]

@jlaustill Sure, It would be great to have clear and detailed API for developers. Thanks for your contribution.

[fabriceci]

Hi, I think it's a very good idea to separate client and server (separation of concerns) but (there is always a but )

During the last 3 weeks, there were at least 2 commits that broke API's connectors (in minor version change), this is not good. I think we should avoid breaking the API, I would even say that , we must never change the API (on a minor version change).

That gives an instability impression and in the long run, there will only be the PHP connector up to date.

I suggest :

• Work now with two branches : master and dev
• Freeze the current version (only do bug fixes)
• Prepare a next major release, to do that, we should open a new thread to discuss what we do.

I will separate the discussion in 4 points :

1. Decide what we do in order to reach a definitive API and consider, after that, we should "never" change it again. Then, the client would be able to evolve without breaking anything (evolution independent of the connectors). (Example, as @jlaustill said to standardize API's responses, etc.).
2. Fully separate client and server (Example: separate repositories, a request to the API to know what is the connector able to do, etc.)
3. Code refactoring (wrap Ajax calls, etc.)
4. Enhancements and new functionalities

What do you think of this approach @servocoder @jlaustill @gmkll ?

[jlaustill]

@fabriceci I think that is a FANTASTIC idea! The more I'm getting into this the more a new major version seems extremely appropriate!

[psolom]

I agree to create dev repo.
@fabriceci @jlaustill Sorry for breaking your connectors. In the case with last commit I knew that it will crash your connector, but did it intentionally. It's a major thing, but it was better to force you to change the behavior of your connector in that point, than after you coding a lot over stale code. It was rude, but necessary in my opinion. Anyway I will create dev repo for future major modifications.

[jlaustill]

@servocoder no worries, the concern isn't about forcing us to update our connectors, but more so about the public appearance of our stability in the master branch that other people will download.

I highly encourage you to code like hell in the new dev branch and force us to keep up as long as we are going down the route of more standardized code and a better, more stable future :)

[fabriceci]

@servocoder no worries, I didn’t say that for my case. We should reach a state where a connector should almost never be updated.

To have a more standardized API or even to finish the way you started (the complete separation between the client and the server), there will be other steps that unfortunately will break connectors.

That's why I proposed to make this time a big step and open a discussion on a major new release (divided in 4 themes). We can work slowly in the dev branch or create another one without disturbing the current version)

Do you have an opinion about that?

[jlaustill]

In order to be in a place where the connectors rarely need updated, the codebase will need to be in a very consistent, standards-based place. I think that consolidating the ajax requests into two functions, apiGet, and apiPut, will bring to light a lot of the places that are currently inconsistent.

servocoder's last commit was a nice step in this direction. He consolidated the url generation into one function. I will bring this into the example apiGet function I wrote in the next few days.

Another step in the right direction would be following the REST api json standard, found here. Basically, this would mean returning objects that looked like this

{ errors: [], data: [] }

and then the consolidated ajax functions would have a standard way to deal with errors. Basically,

if (doc.errors) {
do error stuff
} else {
return doc.data;
}

In this format, the errors array only exists if there are errors, so it's super easy to deal with them. Then pass back the data. This also makes dealing with the errors and data in your api at lot easier.

There is a lot more to the standard, but that is the basics. This kind of a change should absolutely happen in a dev branch, one endpoint at a time.

Currently, I think there are a few bugs we should concentrate on to get the existing code stable on the main branch. Then work on the new changes in dev. But I'm curious to see what your thoughts are on that y'all :)

[psolom]

Ok, I have updated repo to v1.0.6 and it's a stable release.
You can see new "dev" branch is created based on the last stable version.
I have also created the milestone for the next major 1.1.0 release and associated all issues that I plan to include into it at the moment. If anyone wants to do any other major things (standardize, separating, improving, etc.) please create separate issues for that purpose, take a lead on that, and I will add that issue to the milestone.

[psolom]

@fabriceci Please confirm that I can remove normalizePath function. I guess it's useless now since FM expects to get only relative paths now.

@jlaustill I have created new issue #37 for json API standardization, can you take a lead and, perhaps, come with a new PR on that?

[jlaustill]

@servocoder absolutely. I'll start by getting the functions set up, but I have one question first. The entire filemanager.js is in a function, and therefore has its own scope. But I would still like to put new functions in an object so it is more obvious what is being used and to have less of a chance of conflict. For my example, I went with the name

var _$ = {};

But I can go with any name at all, other options I can think of are
var fm = {};
var $fm = {};
var _fm = {};

Do y'all have a preference?

[fabriceci]

@servocoder yes indeed.

@jlaustill When I read your question, I though, as we are in the big step, I'm not a ninja in JavaScript, but I saw that's a good practice to put the plugin functions into a single object (prototype), especially for testability.

First, standardize the API, and then think about what we can improve in the JS (and for now just create a object function like the others)

What do you think?

// something like this
var fm = function (options) {
  // foo
}

fm.prototype.close = function() {
  // foo
}

// client side
var fm = new RichFileManager({ ... });

[psolom]

Well yeah, I also have some thoughts about how to keep the FM plugin, I even would go further and make some of FM functions public and so on. But let's be consistent and postpone all this stuff for now. We have more important things for now like API standardization, config file division and so on. This subject is interesting but we will consider it after the main stuff is done.

[jlaustill]

Yeah, I don't think I explained myself very well here, sorry. I was referring to an internal object, one that would be internal to the fm object you coded an example of @fabriceci . The object I'm referring to would live inside that object, like so

`
// something like this

var fm = function (options) {
var _$ = {};
}

`

The purpose of the object would be purely to contain functions written as utilities for fm verses functions of fm. So this is very simple and just cleans up the code. Sorry if I made this seem like a much bigger scope thing :)

[psolom]

@jlaustill Which functions/variables you are going to put to that internal object? Give a few examples, so I can get your point better.

[jlaustill]

For sure. Basically, anything that is a utility function, and reused internally. I think of it as my libraries personal jquery. So a few examples just looking through the code would be

_$.isDocumentFile
_$.buildConnectorUrl
_$.apiGet
_$.apiPut

But things that would not belong in this object would be the unique functions of fm itself. In the future, these may be functions you expose as public for instance, or maybe not.

var selectItem
var renameItem
var replaceItem

etc etc. These unique functions would use the utility functions. The gain is twofold, first, it is obvious when making changes to, say, _$.apiGet, that this change will affect the code all over the place, and not just in that function. Second, it's a beginning to work toward OOP principles and start to define scopes, just a tiny step in that direction.

For now, I only want to put the new functions I write into it, thus ensuring that I don't cause any side effects with existing code, and to make it clear what has been moved and what hasn't down the road as we start to implement the new consolidated api features one endpoint at a time.

I am really bad at describing things without a whiteboard, thanks for your patience 💯

[psolom]

@jlaustill Thanks for the explanations.
Ok, code it as you suggested, we will see how it goes.

[gmkll]

I think it not off topic to let you know, that I have started some ajax call refactoring using deferred promises in PR #21. I started with changes in loading configurations as this should of course happen before any other calls (this includes calling the file_exists function as a promise). Non synchronous calls are much more standard conform.

I also started to put most stuff in a scoped object "fm", but will remove this from the PR, as @servocoder suggested to not interfere with any other ongoing changes, as I understand previous comments here. I support the proceeding here, which is much more thorough than mine to be sure!

But I will invest some more effort, if it´s worth doing, to merge the recent changes into the PR´s filemanager.js as well the Java Connector. I´ll need about 1-2 days to get it finished. Any ideas about this?

[jlaustill]

@gmkll can you provide an example of your ajax deferred promises? I would like to see how you are doing that. Promises are sort of a new thing in my world that I have avoided because of browser support. I tend not to use things until they are extremely well supported. So this could be a great learning opportunity for me.

[gmkll]

just have a look at my forked repos fm.js!

I use jQuery.Deferred() types. I think it is a bit tricky at the beginnig, but if you get it, things work very well. I regularily use it in my client side code. The nice thing is, that since jQuery 1.5 $.ajax is a promise itself. You have to get used to work with states, which are linked to deferred objects. The state is pending, until it´s resolved, which you could control yourself or you get it set as in $.ajax, You then would set a result to the resolved state, which is, what you normally would like to evaluate. I like especially the $.when function, as you can concatenate promises and otherwise declare deferred objects myself to have full control.

[jlaustill]

@gmkll oh wow, that looks freakin awesome, I guess I know what I'm learning today!

[gmkll]

I have an issue concerning

fully separating client and server

BTW Thanks for your structuring comment, @fabriceci! May be the 4 points should become tags/labels? E.g. "API change", "client and server separation", "refactoring", the last is already one.

In the API it´s mentioned, that "readfile GET is intended to use when it's not possible to get file by direct URL (e.g. protected file)." I still think, FM should be able to support both at least for selectItem, although it may be not more the case (see discussion links below).

In the first case the connector would provide a URL (relative or not, by default I would assume it´s ready to use), in the second case, its an API call to the connector (mode "readfile"). @servocoder made the point, that it may be too complicated for the frontend developer to decide, what to do (cft. comments in issue #27, this, this). But I think, it could easily be resolved, if (at the core) the item property "protected" is evaluated. In case of
protected > 0 // check undefined
it´s an API call to readfile, otherwise it MAY be a API call as well, but it COULD be a direct URL. This decision could be made on client side, as it doesn´t matter for the server, if it´s not protected. But the server has to know, if the client prefers direct URLs or not - and has to get the information from the client. I would suggest a global option and not yet be granular (item property) at the moment. The question may arise, whats the default. but as currently, if selecting an item (selectItem) always a protected URL is returned (calling createPreviewUrl) it should be the API call.

The workflow would be:

1. client sets JSON property directURL = true|false
2. server reads it and sets path to direct URL, if a) client property is true and b) the item property is not protected

In all other cases the server´s API calls are used.

I think, its a reasonable workflow. What do you think? I could think of an extra property externalPath, but this would break the API.

More Info

• The original discussion could be found in createPreviewUrl dynamic path and normalize path bug #27.
• Copy URL and selectItem #40 may be related too

[psolom]

In the API it´s mentioned, that "readfile GET is intended to use when it's not possible to get file by direct URL (e.g. protected file)."

Yeah, it was in that way. Now readfile is mandatory.
Anyway I don't mind to implement direct URLs, but only in case it's implemented at the client-side.
I can suggest a simple solution with a directURL option as you mentioned, but it should be client-side associated option solely. directURL can take false (default, means readfile request) or string - URL which will be added to the relative path that is returned in the response. And again - server should always return only relative path, no other options, no absolute paths returned in the response.

And don't care of protected property, as I remenber you are not able to open or select item with protected == 1. The phrase mentioned in the API reads about the case when you are not able to reach file by direct url, as you can get at the AWS S3 storage, for instance. But the protected flag is still will be 0 in that case.

I have an issue concerning

Btw, what is the issue? I can see only suggestions.

[gmkll]

I agree totally with

server should always return only relative path

The Java-Connector does not yet support any authorization mode, but it may be, some work seem to be in progress? At least - Protected == 0 is not sufficient. The server has to support direct URL as well..

Sorry, the "I have an issue" statement just means I want to discuss more about this topic ;-)

[psolom]

I agree totally with: server should always return only relative path

and

The server has to support direct URL as well.

look as opposite statements )
Perhaps you meant FM has to support direct URLs (at the client-side as I described above).
Either way, is that good for you the way I have suggested to utilize directURL?

[gmkll]

I could live with the client side option only. A relative direct URL would just be any relative (normalized) URL based on document root, but not to the FileManager. If selected the client (Tiny/CKEDITOR...) has a FileManager-independent (relative) URL to the image/file. It´s true, you have then lost one indirection, and the image has to be matched on the server (where the client editor lives). Nevertheless, it seems to me, that´s not a very uncommon use case, but feel free to point me to better/other solutions.

[fabriceci]

I completely forgot : that's the reason why I needed an absolute path.

@servocoder when you select a file in the file manager through a WYSIWYG, the file manager should return the direct URL (in almost all cases).

The most obvious case when you need to select an image/file is when you are in a CMS environment. Get an image through a connector is heavy for the server . If the website's traffic is high, that will be very bad for performance, a server language is not good to serve files.

I think the cleanest solution is to make an additional Ajax request when you click on the select button. The manager will ask "can I have the select path" to the server and the connector will be able to decide what path it return (relative or not).

What do you think of this?

[fabriceci]

@servocoder, what about in my last post? Without this I'm stuck on a old version :( ! What do you think to add an ajax call for the select function?

[psolom]

@fabriceci I am currently working on separation config files and thinking over the absolute path problem. I think the better option is to set preview mode (connector path VS absolute path) of the images and media files in the configuration options. So all the files paths will be treated in the same way. Here what I have come to in the config file:

    "preview": {
        "absolutePath": true,
        "previewUrl": false,
        "folder": "userfiles"
    },

• "absolutePath" - whether to use absolute path to images and media files, otherwise the connector path is applied. The next options make sense only if "absolutePath" set to "true";
• "previewUrl" - url to concatenate with the relative file path, which is returned in json response. You can leave it "false" if the baseUrl (FM page url) is the same as previewUrl. I have a case when those urls are different so it should be customizable.
• "folder" - the most controversial option for me. The explanation is below.

If you perform your connector in the same way as PHP connector you probably saw that it's possible to change "userfiles" folder (root folder for user files). So one user may have his file as /sam/images/file.jpg and another one as /john/images/file.jpg. In the current version of FM you will get only "/images/file.jpg" part in the "Path" param of the response, so we need server to return that user-based folder name (/sam or /john) to build the correct absolute path. We have 2 options for that:

• return "PreviewPath" in the response along with "Path" (as it was before, if you remember). So "Path" will be "/images/file.jpg" and the "PreviewPath" - "/sam/images/file.jpg". In this case you actually don't need "config.preview.folder" option. Absolute path will be "config.preview.previewUrl" + "PreviewPath" from the response.
• since user-based folder doesn't change until the FM page is reloaded (that is how it works for me, perhaps there are more complex cases), we can implement "init" request to the connector. It's going to be performed only once after the FM in initialized. "init" request serves to return some options from the server side back to the client side. One of such options may be a name of user-based folder: "/sam" or "/userfiles/sam", to store it into "config.preview.folder". In this case we don't need to return "PreviewPath" in the response for each file and an absolute path will be "config.preview.previewUrl" + "config.preview.folder" + "Path" from the response.

[psolom]

While writing the answer I have realized that the second option looks more complicated. Return "PreviewPath" in the response for each file seems a simplier solution. But "init" request may be useful to check some info from server or even override some config options at the client-side. Anyway I want to know what do you think on that.

The main idea that I don't want to make "absolute path" feature exclusively for "select" feature. If you want absolute paths during the selection then I can suppose you want them all over the FM. That is why it should be defined in the config. The only thing we need is to choose the way how to get the last part for the absolute path from server.

[psolom]

Capability to preview images and media files with absolute path along with connector path has been implemented at the dev branch. Since there were no suggestion I have come to the second option and that is how it works:

You can see new section in the config file:

"preview": {
        "absolutePath": true,
        "previewUrl": false
    },

Absolute path enabled by default and works with the default FM files structure.
If your URL for preview images differs from the baseUrl (current url of the FM) you are free to setup your own with previewUrl option. Note that "PreviewPath" param in the server response comes back and it is treated to build absolute path. That means connector path will be used if you don't care of "PreviewPath" param. Setting absolutePath to "false" force FM to use connector path.

[psolom]

Solved with the RichFilemanager release v2.0.0