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

File name including space raise a 404 error #95

Closed
KokoKoder opened this issue Dec 14, 2018 · 24 comments
Closed

File name including space raise a 404 error #95

KokoKoder opened this issue Dec 14, 2018 · 24 comments
Assignees
Labels

Comments

@KokoKoder
Copy link

KokoKoder commented Dec 14, 2018

Issue seems to come from an encoding change between ASCII and either unicode or UTF-8. Space are replaced by %20 in the file name.

@rosell-dk rosell-dk self-assigned this Dec 14, 2018
@rosell-dk
Copy link
Owner

%20 seems to indicate that the space was url encoded. That would have happened before calling this library.

I just tested all converters with a filename including space. cwebp and imagickbinary both fails. The rest succeeds.

@rosell-dk rosell-dk added the bug label Dec 14, 2018
@KokoKoder
Copy link
Author

KokoKoder commented Dec 14, 2018

I tried to string replace %20 in $source in webp-on-demand.php but it doesn't solve the issue. file name with space should be returned as ASCII to the browser

@rosell-dk
Copy link
Owner

Did you make sure to to replace $source before $destination is calculated?

@KokoKoder
Copy link
Author

yes, I did echo $source and $destination and clear browser and site cache but all filename with space were displayed with %20 and returning 404 error.

@rosell-dk
Copy link
Owner

In webp-on-demand.php, there is a line which is commented out:
//echo $source; exit;

If you remove the comment and visit the image in the browser, what does it say?

@rosell-dk
Copy link
Owner

Btw, fortunately Wordpress seems to replace spaces with dashes, when you upload, so the impact is not major critical (not that it don't need fixing, obviously)

@rosell-dk
Copy link
Owner

How does the image url look?

@KokoKoder
Copy link
Author

Not using composer so I don't have this line. Indeed leaving space in image name is not something I would recommend but I have a site with such files. It looks that way: https://store.seobytes.eu/images/virtuemart/category/resized/CLIF%20komoda%20RTV%20-%20san%20remo%20&%20grafit%20mat_0x360.jpg

@rosell-dk
Copy link
Owner

oh, yeah, that is a new line.
Just copy it in immediately after $source is first assigned.

@rosell-dk
Copy link
Owner

Are you on Apache or Nginx?

@rosell-dk
Copy link
Owner

rosell-dk commented Dec 14, 2018

I am currently programming an option not to send the file name in the query string, but to rather use $_SERVER['REQUEST_URI']. It will be optional in 0.9. If it works well on all platforms, the option will be removed in 0.10, and there will be no more passing in query string.

You can help out testing this by setting $source like this in webp-on-demand.php:

    $requestUriNoQS = explode('?', $_SERVER['REQUEST_URI'])[0];
    $source =  rtrim($_SERVER["DOCUMENT_ROOT"], '/') . urldecode($requestUriNoQS);

@KokoKoder
Copy link
Author

I am on Apache
echo $source return the following string
/data01/virt64171/domeenid/www.seobytes.eu/store/images/virtuemart/category/resized/CLIF komoda RTV - san remo

@KokoKoder
Copy link
Author

Nicely done, your fix worked.

@rosell-dk
Copy link
Owner

Ok. And where do you see the %20. In the filename in the converted file?

@KokoKoder
Copy link
Author

It's actually in the navigation bar.

@rosell-dk
Copy link
Owner

oh, but it is perfectly normal behaviour that spaces in URLs get encoded with %20. It also behaves like that when the plugin is disabled. But it should of course not result in 404.

I'm a bit puzzled about the combination.

  1. You had a 404
  2. $source was not containing '%20', but normal spaces
  3. The fix fixed the 404

What converter method are you using? As first discovered, the cwebp and imagickbinary methods currently fails converting images containing spaces.

If you would be so kind to test again before the fix, what output do you get if you add "?debug" to the URL ?

@rosell-dk
Copy link
Owner

(I would really like to provide a fix also for the old method (passing source in query string), as it is still going to be an option in 0.9)

@KokoKoder
Copy link
Author

I was sure I was missing something trivial here :D It should be imagick but can't say if it is binary or extension. How can I check that?
Also can you specify to which URL shall I add '?debug' ?

@rosell-dk
Copy link
Owner

On WebP Express options page, you see a list of conversion methods. The first one with a green checkmark is the one that is going to be used.

The image url with ?debug added might look something like this:
http://example.com/wordpress/wp-content/uploads/space%20in%20name.jpg?debug

Adding ?debug tells webp convert to show a report of the conversion rather than the converted image

@KokoKoder
Copy link
Author

might be gd extension then but I am bot running wordpress on the website, so I am not on webp Express. I try to use webp convert as stand alone on server not running wordpress
in that circumstances ?debug give me only the image itself . I run WebP Express on another server with exactly the same config and it returns gd extension first and then Imagick extension and Imagick binary

@rosell-dk
Copy link
Owner

Ah, ok, happy to hear that the WebP On Demand solution is used :) I'm getting so many requests regarding WebP Express (which is based on WebP On Demand), that I just assumed you accidentally posted to this repo rather than WebP Express.

But, in that case - to trigger debug mode, simply set the 'show-report' option to true. The report will perhaps reveal what went wrong.

I have btw just released a bug release, 1.3.1, where cwebp and imagickbinary are able to convert images with filenames that contains spaces.

I'm off for weekend. Cheers

@KokoKoder
Copy link
Author

I am also an advocate for html site whenever it makes sense :). I'll test the show report

@rosell-dk
Copy link
Owner

Perhaps the NE flag will kill the %20 bug?

In the RewriteRule, add "NE" to the flags.
ie:

RewriteRule ^(.*)\.(jpe?g|png)$ webp-on-demand.php?source=%{SCRIPT_FILENAME} [NC,NE,L]

Please let me know, thanks :)

@KokoKoder
Copy link
Author

It gives a 404 too. (I removed the fix above to do the test). Same when enabling display error message

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

No branches or pull requests

2 participants