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

createPreviewUrl dynamic path and normalize path bug #27

Closed
gmkll opened this issue Aug 9, 2016 · 12 comments
Closed

createPreviewUrl dynamic path and normalize path bug #27

gmkll opened this issue Aug 9, 2016 · 12 comments
Milestone

Comments

@gmkll
Copy link

gmkll commented Aug 9, 2016

Cft comment in #18.

gmkll pushed a commit to gmkll/RichFilemanager that referenced this issue Aug 9, 2016
@psolom psolom added this to the 1.1.0 milestone Aug 16, 2016
@gmkll
Copy link
Author

gmkll commented Aug 17, 2016

#21 is now updated and merged, cft. last commit. The Java connector now will set the path property to a ready-to-use URL. On the client side I use at the moment just:

var createPreviewUrl = function(path) { return "/" + path; };
This way I get - if selecting an item, just the correct URL.

If the PR is accepted, I would like to add a configuration key e.g. named previewUrl, which, if true, would just return the server path value (with prefix / i.e. fileRoot), otherwise the default.

@psolom
Copy link
Owner

psolom commented Aug 17, 2016

Great, thanks.
What is your case with preview url?

  1. The idea is to make strict separation of front and back parts, so if your connertor is on server1 and the frontend in on server2 this way (relative path) won't work. So createPreviewUrl function is intended to create url like http://site.com?mode="readfile"... to connector and you have to handle it.

  2. Some previewers aren't work with relative paths, Google Viewer for sure, perhaps Pdf Viewer as well.

So the only right way fot any connector is to implement "readfile" method, which reads file and outputs its content.

@gmkll
Copy link
Author

gmkll commented Aug 18, 2016

I could do this - sorry, I missed this. Preview is now readfile, I have to update the Java Connector.

The case is, that I do not always want to export (select) the proxy url (readfile URL). You could think of it like, that you release this image may be by copying it into a public folder. Then I have not to do always check and restrict access, it´s released.

I have to update the Java Connector to get the two settings working both, the default setting with relative paths in path, and the one I described with direct URL (using document root in config file).

Do you think, it makes sense?

@psolom
Copy link
Owner

psolom commented Aug 18, 2016

I guess it's a bad idea to use direct urls (relative or absolute) to get image or media files. It leads to the situation when the frontend should expect various results in response and handle each case in a different way. This makes things more complicated. Frontend won't be able to handle cases for each connector. Eventually we will get a mess in the code.

I understand that you may have images in a public folder. That is my case in fact. All files which I have on my local machine and demo page are in a public folder and may be reached by a direct URL. And that's how the FM was arranged before. But, since we decided to bring more standards in FM and make it more unified, I also had to modify PHP connector to handle readfile request and get rid of direct URLs.

Take the idea that all logic (which file to read and the way to output it) should be perfomed in the readfile method of your connector.

@gmkll
Copy link
Author

gmkll commented Aug 18, 2016

Hm, I understand. There has been some discussion around this, I assume.

Neverthelesse I´ll update the connector, but try to keep the direct mode, as it is the default for many instances we use - with backward compatibility and URL consistency some reasons behind.

Is this ok?

@psolom
Copy link
Owner

psolom commented Aug 18, 2016

I would prefer to get rid of direct URL completely. There are no working connectors now except PHP. Java and Node.js are in development now. Any other connectors have to be rewritten and no need in backward compatibility. Let's do in the following way. You implement readfile method in Java connector in order to make it works in conjuction with the actual version of FM (no direct urls). I will merge it to DEV branch including the changes I'm working on at the moment. After that I don't mind if you come with some elegant solution to handle direct urls, but we have to discuss it at first. So the steps:

  1. You complete you PR with readfile implementations, no direct urls to use
  2. I merge your PR + my changes to DEV branch
  3. We can discuss if really need direct URLs and the way to implement them (if needed)

@psolom
Copy link
Owner

psolom commented Aug 18, 2016

Original discussion on standards and unification is in #31 FYI

@gmkll
Copy link
Author

gmkll commented Aug 18, 2016

Hey, thanks! I do my best to separate it. I´ll include just the crude one without direct URL support in the PR, but keep the extended version for later consideration, which then may become another PR.

@psolom
Copy link
Owner

psolom commented Aug 18, 2016

Yeah, that's great

@gmkll
Copy link
Author

gmkll commented Aug 18, 2016

The code is updated and it´s working apparantly. I kept the (two) methods required for direct URL as dummy indirections, but otherwise removed anything, so it´s much cleaner. If the community comes later up with another solution they could just removed them, without any side effects. This way I can just extend the existing RichFM.java.

gmkll pushed a commit to gmkll/RichFilemanager that referenced this issue Aug 18, 2016
JAVA
 - support readfile mode
 - remove preview filtering/mapping, but keep/provide the empty wrappers to allow
 for extending and first step hint for implementation, cft. issue psolom#27 and
 psolom#31
@psolom
Copy link
Owner

psolom commented Aug 18, 2016

Thank you, I am going to merge it up to next week.

@psolom
Copy link
Owner

psolom commented Nov 4, 2016

Solved with the RichFilemanager release v.2.0.0. Check this post for details.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants