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

Trying to set image on existing page via API causes Exception #622

Closed
coheredigital opened this issue Jun 20, 2018 · 8 comments
Closed

Trying to set image on existing page via API causes Exception #622

coheredigital opened this issue Jun 20, 2018 · 8 comments

Comments

@coheredigital
Copy link

Short description of the issue

Trying to set image on existing page via API causes
Exception: Download URLs must begin with http:// or https:// (in /application/wire/core/WireHttp.php line 643)

Expected behavior

Image should be downloaded and saved to page.

Actual behavior

Error: Exception: Download URLs must begin with http:// or https:// (in /application/wire/core/WireHttp.php line 643)

#0 /application/wire/core/Pagefile.php(167): ProcessWire\WireHttp->download('', '/application/si...')
#1 /application/wire/core/Pageimage.php(1534): ProcessWire\Pagefile->___install('https://bookdir...')
#2 /application/wire/core/Wire.php(383): ProcessWire\Pageimage->___install('https://bookdir...')
#3 /application/wire/core/WireHooks.php(723): ProcessWire\Wire->_callMethod('___install', Array)
#4 /application/wire/core/Wire.php(442): ProcessWire\WireHooks->runHooks(Object(ProcessWire\Pageimage), 'install', Array)
#5 /application/wire/core/Pagefile.php(115): ProcessWire\Wire->__call('install', Array)
#6 /application/wire/core/Pagefile.php(88): ProcessWire\Pagefile->setFilename('https://bookdir...')
#7 /application/wire/core/Pageimage.php(118): ProcessWire\Pagefile->_construct(Object(ProcessWire\Pageimages), 'https://bookdir...')
#8 /application/wire/core/Pageimages.php(53): ProcessWire\Pageimage->

This error message was shown because: you are logged in as a Superuser. Error has been logged.

Optional: Suggestion for a possible fix

Bug seems, to reside with Sanitizer::url()
Passing the raw image URL to Sanitizer returns an empty string, the image URL below.
https://bookdirect_venue_images.s3.amazonaws.com/1520896236355-E4wlUG7UTORst5nE.jpg
(please let me know if this image is no longer live and I will replace, the system periodicaly replaces them)

Steps to reproduce the issue

Try to set the above link to and image field and save, or passed through Sanitizer::url().

Setup/Environment

Docker, NGINX, php-fpm

  • ProcessWire version: 3.0.98
  • (Optional) PHP version: 7.2
  • (Optional) MySQL version: MariaDB (latest)
@ryancramerdesign
Copy link
Member

ryancramerdesign commented Jun 20, 2018 via email

@coheredigital
Copy link
Author

So sorry Ryan, I deleted that comment as I became unsure. I was trying to follow the variable by stepping through the process in xdebug. I will be working on the code again today I will try to chime in with what I find. Thanks.

ryancramerdesign added a commit to processwire/processwire that referenced this issue Jun 27, 2018
…workaround that PHP’s FILTER_VALIDATE_URL does not accept underscores in hostnames, despite their use being fairly common (even if not technically valid).
@ryancramerdesign
Copy link
Member

Trying this again, and now I can duplicate it. It turns out that FILTER_SANITIZE_URL does allow underscores, but FILTER_VALIDATE_URL does not. While it seems like a bug in filter_var() I did some research on it and apparently underscores aren't technically allowed in hostnames. But clearly Amazon is using them (in the URL you pointed out), and apparently they can be used even if not technically valid. So if they are being used in the wild, we should support them, regardless of what's technically valid. I have updated the sanitizer url() method to workaround this behavior in FILTER_VALIDATE_URL, so that it should now allow for hostnames with underscores. Thanks.

@netcarver
Copy link
Collaborator

@spruijtdesign Coupd you check if this is now fixed to your satisfaction and close the issue if it's all done. Many thanks!

@netcarver
Copy link
Collaborator

@spruijtdesign Hi Adam. I intend closing this issue in a week or so if I've not heard anything back from you - or anyone else - about it by then.

You can always re-open this issue later if you need to.

@coheredigital
Copy link
Author

coheredigital commented Aug 5, 2018 via email

@netcarver
Copy link
Collaborator

@spruijtdesign Hi Adam. Thanks for the reply - I'll hold off doing anything until I hear back from you.

@coheredigital
Copy link
Author

coheredigital commented Sep 7, 2018

Thanks @netcarver I got the chance to test again today. Issues is fixed.

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

3 participants