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

Gravatar #8

Closed
wants to merge 7 commits into
base: develop
from

Conversation

Projects
None yet
5 participants
@samt

samt commented Nov 22, 2010

http://tracker.phpbb.com/browse/PHPBB3-9498

Adds the following:

  • New avatar type: Gravatar
  • Ability for board administrators to default every new users' avatar to a Gravatar (even when avatars are turned off - blog mode, so to speak)
  • Ability for board administrators to specify the maximum content rating of the Gravatar allowed on their board.
  • Ability for board administrators to specify a default avatar in the event of a non-existent Gravatar
  • Ability for board administrators to specify the dimensions of a default Gravatar

naderman and others added some commits Nov 20, 2010

Merge commit 'release-3.0.8'
* commit 'release-3.0.8': (393 commits)
  [prep-release-3.0.8] Incrementing version number to 3.0.8 and update changelog
  [ticket/9903] Script for detecting potentially malicious flash bbcodes
  [ticket/9904] Update WebPI Parameters.xml to work with WebMatrix.
  [ticket/9899] Change recaptcha theme from default to 'clean' in the ACP.
  [ticket/9509] Fix a typo and wrong period placement
  [ticket/9903] Fix XSS in BBcode-parser's Flash-BBcode.
  [develop-olympus] Updating changelog for last minute 3.0.8-RC1 fixes.
  [ticket/9140] Check current board version in incremental update packages
  [ticket/9891] Updater drops language-selection after database-update
  [develop-olympus] Updating changelog with latest changes for 3.0.8-RC1
  [ticket/9884] Reduce queue interval to 60 seconds, email package size to 20
  [ticket/9886] Update fails on PostgreSQL because of an error in _add_module
  [ticket/9888] Update fails when Bing [Bot] was already added to the users table
  [develop-olympus] Bumping version number for 3.0.8-RC1.
  [ticket/9885] Fix extension group name updater. Loop through all languages.
  [ticket/9847] Fix typo in search synonyms. Use british english for 'judgement'.
  [ticket/9883] Change an American English spelling to British English.
  [task/phing-build] Correct the path for update package patch files.
  [ticket/9880] Change "antibot" to "anti-spambot".
  [ticket/9696] Surpress is_dir() notice when using SQLite with open_basedir.
  ...
Sam Thompson
Added Gravatar
Going to do some testing then add some more features to it, such as allowing the board admin to specify the rating and defautlt image.
Sam Thompson
Full gravatar integration
Everything seem to work.
@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Nov 22, 2010

Contributor

From quickly skimming over it, you're not following the coding guidelines everywhere with your ifs. 'usegravatar' could maybe be 'use_gravatar'. The modules table needs to be adjusted to reflect the new avatar display conditions in the UCP.

There should be a ticket for this, the branch should be named 'feature/gravatar' and the commit messages should follow the guidelines described here. The commits could perhaps be squashed and rebased to make things a little cleaner.

Apart from that, it looks pretty good so far. Thumbs up. I think we should include this in ascraeus, also considering how small the patch is.

Oh, and my bad, just saw that there actually is a ticket. :)

Contributor

igorw commented Nov 22, 2010

From quickly skimming over it, you're not following the coding guidelines everywhere with your ifs. 'usegravatar' could maybe be 'use_gravatar'. The modules table needs to be adjusted to reflect the new avatar display conditions in the UCP.

There should be a ticket for this, the branch should be named 'feature/gravatar' and the commit messages should follow the guidelines described here. The commits could perhaps be squashed and rebased to make things a little cleaner.

Apart from that, it looks pretty good so far. Thumbs up. I think we should include this in ascraeus, also considering how small the patch is.

Oh, and my bad, just saw that there actually is a ticket. :)

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Nov 23, 2010

Member

This feature needs to come with tests in order to be merged. Ideally we would refactor the code so that adding more avatar types will not require extensive code changes anymoe.

Member

naderman commented Nov 23, 2010

This feature needs to come with tests in order to be merged. Ideally we would refactor the code so that adding more avatar types will not require extensive code changes anymoe.

@samt

This comment has been minimized.

Show comment
Hide comment
@samt

samt Nov 23, 2010

I'll make the requested changes on another branch (feature/gravatar) and consolidate the changes to a single commit. There is also a really picky, consistency thing with a config var I named I would like to fix anyway.

As far as testing goes, I will look into it and resubmit the pull request with proper tests.

samt commented Nov 23, 2010

I'll make the requested changes on another branch (feature/gravatar) and consolidate the changes to a single commit. There is also a really picky, consistency thing with a config var I named I would like to fix anyway.

As far as testing goes, I will look into it and resubmit the pull request with proper tests.

Sam Thompson
[feature/gravatar] Implemented Gravatar
Created a new avatar type allowing users to use their Gravatar associated with their email address. Addtional ACP controls have been added to allow the admin to specify a max content rating, default image, and default Gravatar size.

PHPBB3-9498
@@ -2259,6 +2297,13 @@ function avatar_get_dimensions($avatar, $avatar_type, &$error, $current_x = 0, $
case AVATAR_REMOTE :
break;
case AVATAR_GRAVATAR :
// Note: we actually return in this case. We accecpt whatever

This comment has been minimized.

@p

p Mar 4, 2011

Contributor

accecpt -> accept

@p

p Mar 4, 2011

Contributor

accecpt -> accept

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Mar 4, 2011

Member

Cullen has been working on a proper implementation of avatar abstraction so I'll close this pull request.

Member

naderman commented Mar 4, 2011

Cullen has been working on a proper implementation of avatar abstraction so I'll close this pull request.

{
global $user, $config;
// Hieght and width, Gravatars are /always/ square, and we are using 80 is

This comment has been minimized.

@marc1706

marc1706 Apr 7, 2012

Member

Hieght -> Height

you also might not want to capitalise it

@marc1706

marc1706 Apr 7, 2012

Member

Hieght -> Height

you also might not want to capitalise it

@@ -2259,6 +2297,13 @@ function avatar_get_dimensions($avatar, $avatar_type, &$error, $current_x = 0, $
case AVATAR_REMOTE :
break;
case AVATAR_GRAVATAR :
// Note: we actually return in this case. We accecpt whatever
// current_x is and fall back on 80 (Gravatar standard default)

This comment has been minimized.

@marc1706

marc1706 Apr 7, 2012

Member

Kind of hard to understand.

maybe: "use current_x if the size has been set and fall back on 80 (Gravatar default) if not

@marc1706

marc1706 Apr 7, 2012

Member

Kind of hard to understand.

maybe: "use current_x if the size has been set and fall back on 80 (Gravatar default) if not

'GRAVATAR_FORCE' => 'Force Gravatar avatars',
'GRAVATAR_FORCE_EXPLAIN' => 'Force all newly registered users to use Gravatar avatars. This will work regardless of if avatars are on or not.',
'GRAVATAR_DEFAULT' => 'Default Gravatar image',
'GRAVATAR_DEFAULT_EXPLAIN' => 'If the user\'s email address does not have a Gravatar associated with it, default to this picture. See all possible values <a href="http://en.gravatar.com/site/implement/images/#default-image">here</a> or specify the full URI of a default avatar.',

This comment has been minimized.

@marc1706

marc1706 Apr 7, 2012

Member

shouldn't be using a backslash to escape it

@marc1706

marc1706 Apr 7, 2012

Member

shouldn't be using a backslash to escape it

@naderman

This comment has been minimized.

Show comment
Hide comment
@naderman

naderman Apr 7, 2012

Member

This pull request has already been closed in favour of a more generalized implementation.

Member

naderman commented Apr 7, 2012

This pull request has already been closed in favour of a more generalized implementation.

@marc1706

This comment has been minimized.

Show comment
Hide comment
@marc1706

marc1706 Apr 7, 2012

Member

I noticed that after I commented on it.

Member

marc1706 commented Apr 7, 2012

I noticed that after I commented on it.

erikfrerejean pushed a commit to erikfrerejean/phpbb3 that referenced this pull request Apr 18, 2012

mungo pushed a commit to mungo/phpbb3 that referenced this pull request Jun 27, 2014

Merge pull request phpbb#8 from bantu/travis-lint-before-script
Benutze before_script Sektion für Programminstallation.

nickvergessen added a commit that referenced this pull request Nov 25, 2014

Merge pull request #8 from phpbb/ticket/security-169
[ticket/security-169] Stop loop through referer dir in top directory

brunoais pushed a commit to brunoais/phpbb that referenced this pull request Sep 30, 2015

Merge pull request phpbb#8 from VSEphpbb/update-fixes
Add message about htaccess when installing/updating

@3D-I 3D-I referenced this pull request Mar 8, 2018

Merged

[ticket/15580] Remove redundant code from ACP #5156

4 of 4 tasks complete

This issue was closed.

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