Adding proxy options to check when a Gravatar exists #16

Closed
wants to merge 13 commits into
from

2 participants

@jamhall

I've added the option to use a proxy when a user requests if a gravatar exists. Currently it's using fsockopen. At our company this is a problem as our servers don't have a direct connection to the internet, everything goes through a proxy.

I've replaced fsockopen with curl as curl offers out-of-the-box options to specify a proxy. By default there is no proxy.

If you'd like me to add options to specify a username and password for a proxy that needs authentication, do not hesitate to let me know. Thanks.

@henrikbjorn henrikbjorn and 1 other commented on an outdated diff May 29, 2012
GravatarApi.php
+ $ch = curl_init();
+ curl_setopt($ch, CURLOPT_URL, $path);
+ if($this->defaults['proxy'])
+ {
+ curl_setopt($ch, CURLOPT_PROXY, $this->defaults['proxy']['url']);
+ curl_setopt($ch, CURLOPT_PROXYPORT, $this->defaults['proxy']['port']);
+ }
+ curl_setopt($ch, CURLOPT_HEADER, true);
+ curl_setopt($ch, CURLOPT_NOBODY, true);
+ curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
+ curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
+ curl_setopt($ch, CURLOPT_MAXREDIRS, 10);
+ $data = curl_exec($ch);
+ $httpcode = curl_getinfo($ch, CURLINFO_HTTP_CODE);
+ curl_close($ch);
+ return $httpcode >= 200 && $httpcode < 300 ? true : false;
@henrikbjorn
Owner
henrikbjorn added a line comment May 29, 2012

First the Coding Standard is wrong here.

The fsockopen is used because it is faster than any other option, and this does matter when you are checking on stuff like images exists server side.

Also i think that the proxy thing should not be done in the code but server side or in with some custom code by extending this class and overwriting it.

@jamhall
jamhall added a line comment May 29, 2012

Hi @henrikbjorn - This is the first time i've done it. Why is the coding standard wrong? :) If using a proxy, CURL was the cleanest solution. It is is however slower than fsockopen (although for us we're on a 10gbit internet connection so we don't notice any speed issues) You don't need to accept this pull request, I just thought it would have been nice for anyone else who happens to need to use a proxy to have this configuration option set-up and ready to use.

EDIT: if not using a proxy, I could change it to use a socket, if using a proxy we could keep curl....options are open. what do you think?

@henrikbjorn
Owner
henrikbjorn added a line comment May 29, 2012

The coding standard should be 4 spaces. with the { on the same line as the construct, this bundle follows the same as Symfony does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@henrikbjorn henrikbjorn commented on an outdated diff May 29, 2012
README.markdown
@@ -63,14 +65,14 @@ If you use twig you can use the helper like this exemple:
Or if you want to check if a gravatar email exists:
- {% if gravatar_exists('alias@domain.tld') %}
+ {% if gravatar_exits s('alias@domain.tld') %}
@henrikbjorn
Owner
henrikbjorn added a line comment May 29, 2012

theres a space that shouldnt be there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@henrikbjorn henrikbjorn and 1 other commented on an outdated diff May 29, 2012
DependencyInjection/Configuration.php
@@ -21,8 +21,14 @@ public function getConfigTree()
->scalarNode('size')->defaultValue('80')->end()
->scalarNode('rating')->defaultValue('g')->end()
->scalarNode('default')->defaultValue('mm')->end()
+ ->arrayNode('proxy')
+ ->addDefaultsIfNotSet()
+ ->children()
+ ->scalarNode('url')->cannotBeEmpty()->end()
+ ->scalarNode('port')->cannotBeEmpty()->end()
@henrikbjorn
Owner
henrikbjorn added a line comment May 29, 2012

It can be empty and should be most of the time, use a defaultValue.

@jamhall
jamhall added a line comment May 29, 2012

What would the default value be?

@henrikbjorn
Owner
henrikbjorn added a line comment May 29, 2012

currently you are forcing people to set url and port, the default value should be null.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@henrikbjorn
Owner

I am not positive about this. Instead of checking that a gravatar exists it would be easier to specify a default for the image used. Also this looks like a niche case.

By replacing fsockopen with curl you are currently making it slower (and if i remember correctly a lot slower).

Instead of adding this complexity you should subclass the helper instead and overwrite the service definition with your class.

@henrikbjorn henrikbjorn closed this Sep 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment