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

Handle previously unhandled exceptions with social plugin and Hybridauth #4643

Merged
merged 3 commits into from
Dec 29, 2021

Conversation

Deltik
Copy link
Member

@Deltik Deltik commented Dec 28, 2021

Motivation and Context

#4192 – social login with FB - avoid fatal error if ID is missing

A frontend error should not happen if any social login provider is not configured correctly.

#4199 – handler dependent on social plugin

e107 should not break with a fatal error if the social plugin is removed.

Description

  • e107::getUserProvider() no longer throws exceptions, as the client usages do not handle them. Instead, the exceptions are suppressed now.
  • Creating an e_user_provider manually now has the option to un-suppress exceptions, which lets the admin verify that their configurations are valid at /e107_plugins/social/admin_config.php?mode=main&action=configure.
  • e_user_provider will no longer fatal error if the social plugin is removed.

How Has This Been Tested?

Added tests for bad Hybridauth configurations and resulting exceptions that are either suppressed or rethrown

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist

is a better name than
`\social_login_config::getValidConfiguredProviderConfigs()` because the
method does not validate the provider configs.
@Deltik Deltik requested a review from CaMer0n December 28, 2021 10:58
@codeclimate
Copy link

codeclimate bot commented Dec 28, 2021

Code Climate has analyzed commit 3f59b3b and detected 9 issues on this pull request.

Here's the issue category breakdown:

Category Count
Bug Risk 9

Note: there are 6 critical issues.

The test coverage on the diff in this pull request is 72.7% (80% is the threshold).

This pull request will bring the total coverage in the repository to 33.9% (0.1% change).

View more on Code Climate.

@CaMer0n
Copy link
Member

CaMer0n commented Dec 29, 2021

Thank you @Deltik !!

@CaMer0n CaMer0n merged commit cfa36cc into e107inc:master Dec 29, 2021
@Deltik Deltik deleted the fix/4192 branch December 29, 2021 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants