-
Notifications
You must be signed in to change notification settings - Fork 297
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
[reference] Generic thumbnailer #153
Conversation
Fixed! Check the list of commits above.
A few remarks:
|
…iling * also purge obsolete special treatment for ted.com, flickr.com, vimeo.com
…ecific regexps/configuration. A bit DRYer and cleaner. TODO: test flickr image links! see also #128
I have more info on thumbnails failing. I've turned on error reporting, and tried generating thumbnails using a dummy link list. Thumbnailing this link always fails: https://en.wikipedia.org/wiki/Cymatics. When accessing the corresponding permalink, this error shows up in my apache log:
@mro Do you get the same results? Any idea why this happens? |
very odd. I'm not familiar with how precise php error message line numbers are, but the Update: Uh - got it: it's line 2001 that's supposed to be
|
So will it be merged or do it need extra tester/testing ? |
Hi @Draky50110 nice to see you're interested :) I still have to apply the change suggested by mro (right now it's partially broken) and rebase changes on top of master. I'll do it today/tomorrow and yes, testers are welcome. One thing that was not decided: do we enable thumbnails by default for all domains, or just a few ones (those with predictable thumbnail size)? Or do we keep thumbnailing olny the old ones by default, and let users specify a whitelist for other domains? |
My own Shaarli displays thumbs for ALL links, based on thumbshots.com : http://gilles.wittezaele.fr/links/ |
We agree on this, your thumbnailer does the job locally and is more privacy respecting than the old one (does not leak visitor's IPs to the thumbnailed sites, it leaks the server IP instead) @mro would you like thumbnailing enabled for all sites by default? Or something else (configure
and populate the Edit: @Draky50110 this thumbnailer generates thumbs for past and future links. You can test it by cloning/downloading my |
frankly I'm undecided. I worry a bit about thousands of files in the image cache never being cleaned up. And thumbnails are not the main topic of shaarli. They are nice but optional. But if it causes no pain: yes. Otherwise configure URL regexps in options file. Also thought about an additional (optional) form field - pre-filled with |
Maybe it's probably better to consider this as a specific plugin or a specific template. When the plugin system is in place (#164), it will be (more) easy to develop this kind of functionality. |
Remaining todo with this, branch
Later: Add an option to enable thumbnails for all domains ( |
may seem a bit OT, but I hit a strange HTTP |
This works very well already :) Thanks for that, @nodiscc |
This needs to be rebased and these points should be fixed. Help on the rebase is welcome. |
I have unassigned myself since this probably needs to be rewritten from scratch, and thumbnailing moved to a plugin. I will leave this Pull Request open for future reference. |
Made obsolete by #687 (Use web-thumbnailer to retrieve thumbnails) |
Adapted from @mro's work in #128 (3 first commits.
Manually reviewed, the code now merges properly and doesn't cause Shaarli to crash. There was a missing{
in the other pull request. I left this commit https://github.com/mro/Shaarli/commit/3d338d499eb36780b9795f6360d00c1acd2d0bab out of the pull request for now.Theog:image
thumbnailing code doesn't seem to do anything; I've tried to understand regexps from the previous PR, and tried to match them against page that have theog:image
property (test page: https://soundcloud.com/crocriddim/test-007)The regexp'/<meta'.'[^>]*?'.'(?:'.$xogimage.'[^>]*?'.$xcontent.'|'.$xcontent.'[^>]*?'.$xogimage.')[^>]*>/mi'
compiles to/]*?(?:\s+(?:property|name)\s*=\s*["']og:image(?::secure_url|:url)?["'][^>]*?\s+content\s*=\s*["']([^"']+)["']|\s+content\s*=\s*["']([^"']+)["'][^>]*?\s+(?:property|name)\s*=\s*["']og:image(?::secure_url|:url)?["'])[^>]*>/mi
.It seems to matchproperty="og:image" content="https://i1.sndcdn.com/artworks-000107375178-9c6yju-t500x500.jpg">
in the test page https://soundcloud.com/crocriddim/test-007 - tested in http://www.regexr.com/However I've tried addingerror_log("HHAHAHHAHHA MATCH");
just above// download the image into $dat
and I never get this message in my logs. So I guess we never enter theif( preg_match('/<meta'......
loop.@mro can you have a look? You can get my code with
git clone -b generic-thumbnailer https://github.com/nodiscc/Shaarli