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

WebPathResolver - sanitize URL to directory name #480

Merged
merged 2 commits into from
Oct 29, 2014

Conversation

teohhanhui
Copy link
Contributor

No description provided.

@makasim
Copy link
Collaborator

makasim commented Aug 20, 2014

@teohhanhui it is not clear what this realy fix. could you give more information on why do we need this?

@teohhanhui
Copy link
Contributor Author

Before this change Filesystem::dumpFile throws IOException when trying to cache external images, as it's not possible to create a directory with "/" in its name. This is a rudimentary fix.

@makasim
Copy link
Collaborator

makasim commented Aug 20, 2014

ok, I see, could you add a test then?

@makasim
Copy link
Collaborator

makasim commented Oct 22, 2014

sorry for the delay... I've just found out that you had added some tests.

What about other resolvers, I think this fix has to be applied on a higher level, maybe inside the cache manager

@teohhanhui
Copy link
Contributor Author

I disagree. Path handling should be left to each resolver as it currently
is (e.g. what makes sense for the filesystem might not make sense for S3).
On Oct 23, 2014 1:56 AM, "Maksim Kotlyar" [email protected] wrote:

sorry for the delay... I've just found out that you had added some tests.

What about other resolvers, I think this fix has to be applied on a higher
level, maybe inside the cache manager


Reply to this email directly or view it on GitHub
#480 (comment)
.

@makasim
Copy link
Collaborator

makasim commented Oct 23, 2014

I can try to pass an url to s3 too, what would be key on s3 in this case?

@teohhanhui
Copy link
Contributor Author

That should be left up to the S3 resolver to decide how to normalize the
URL to S3 key... My point is, I don't think there's a one-size-fits-all
solution here.
On Oct 23, 2014 2:44 PM, "Maksim Kotlyar" [email protected] wrote:

I can try to pass an url to s3 too, what would be key on s3 in this case?


Reply to this email directly or view it on GitHub
#480 (comment)
.

@makasim
Copy link
Collaborator

makasim commented Oct 23, 2014

okay okay

makasim added a commit that referenced this pull request Oct 29, 2014
WebPathResolver - sanitize URL to directory name
@makasim makasim merged commit c8b6eca into liip:master Oct 29, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants