Problem/Motivation

Social Auth should provide configuration options to allow site builders manage the behavior of the site in some circumstances. We should be able to provide settings options similar to the ones of Simple FB Connect

This issue covers only the creation of the options in the form. There's no need to make the options work because that will be done in this issue

Proposed resolution

Create a form class which extends ConfigBaseForm. This form class will be extended by the implementers, so this class should render the setting fields in the buildForm() method.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

gvso created an issue. See original summary.

gvso’s picture

Issue summary: View changes
felribeiro’s picture

Status: Active » Needs review
FileSize
4.2 KB

Please, review this patch and suggest some improvement.

e0ipso’s picture

Thanks for the patch @felribeiro. I added some suggestions:

  1. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,108 @@
    +  public function getFormId() {
    +    return ''
    +    . 'social_auth_admin_form';
    +  }
    

    This feels a bit weird. Maybe:

    return 'social_auth_admin_form';
    
  2. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,108 @@
    +    $form['module_settings']['post_login_path'] = array(
    

    We should allow to redirect back to the page where they started the login process.

  3. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,108 @@
    +    $form['module_settings']['disabled_roles'] = array(
    +      '#type' => 'checkboxes',
    +      '#title' => $this->t('Disable Social Auth login for the following roles'),
    +      '#options' => $options,
    +      '#default_value' => $social_auth_config->get('disabled_roles'),
    +    );
    

    Shouldn't we use the regular permissions system instead?

gvso’s picture

Status: Needs review » Needs work

Thanks for the patch!

  1. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,108 @@
    +<?php
    +
    +/**
    + * @file
    + * Contains \Drupal\social_auth\Form\SocialAuthSettingsForm.
    + */
    +
    

    I think we have not removed this kind of comments in the code yet, but I have seen that it's not longer necessary. Core modules don't have a @file comment in their classes anymore

  2. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,108 @@
    +use Drupal\Component\Utility\SafeMarkup;
    

    Should we use SafeMarkup? It is deprecated, so I don't know if it is recommendable to use it

  3. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,108 @@
    +    return [
    +      'social_auth.settings',
    +    ];
    +  }
    

    I think we should return only a string, so the child can do something like:

    return [
    parent::getEditableConfigNames(),
    'module.settings'
    ]

gvso’s picture

We should allow to redirect back to the page where they started the login process.

I agree, we should specify to leave it in blank if site builders want to redirect to where the process started.

gvso’s picture

Issue summary: View changes
Issue tags: +Novice
Related issues: +#2821853: Provide Social Auth settings: functionality
gvso’s picture

Issue tags: +GCI16
denutkarsh’s picture

Status: Needs work » Needs review
FileSize
4.12 KB
1.58 KB

Okay, so i fixed the previous patch and added suggestions from comment #4,#5,#6.

gvso’s picture

Status: Needs review » Needs work
  1. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,99 @@
    +    return 'social_auth.settings';
    

    Sorry for the last comment. Return an array as most modules do. Implementers will need to do something like return parent::getEditableConfigNames() + ['module.settings'];

  2. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,99 @@
    +        $options[$key] = SafeMarkup::checkPlain($role_object->get('label'));
    

    The new patch do not include SafeMarkup at the top, but has this code which will produce an error

To test if your form works. You can change the code of the settings form of any implementer (e.g. Social Auth Twitter). Instead of extending ConfigFormBase, you need to extend from the new SocialAuthSettingsForm. Plus, in getEditableConfigName() you should do something like return parent::getEditableConfigNames() + ['module.settings'];

denutkarsh’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
848 bytes

@gvso Okay so i have made this patch fixing the issues mentioned in previous comment. I have also tested it on Social Auth Google.

gvso’s picture

  1. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,101 @@
    +    $form['module_settings'] = array(
    ...
    +    $form['module_settings']['post_login_path'] = array(
    ...
    +    $form['module_settings']['redirect_user_form'] = array(
    ... among others
    

    Please change 'module_settings' to 'social_auth'

  2. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,101 @@
    +      '#title' => $this->t('Social Auth configurations'),
    

    I'd say "Social Auth settings"

  3. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,101 @@
    +    foreach ($roles as $key => $role_object) {
    +      if ($key != 'anonymous' && $key != 'authenticated') {
    +        $options[$key] = \Drupal\Component\Utility\Html::escape($role_object->get('label'));
    +      }
    +    }
    

    Instead, implement an use statement above and don't use (\) before Drupal

I didn't check the functionality yet. I hope you tested if configurations are saved

gvso’s picture

Status: Needs review » Needs work
denutkarsh’s picture

FileSize
4.13 KB
3.27 KB

@gvso Here is the new patch that fixes all the issues mentioned.

denutkarsh’s picture

Status: Needs work » Needs review
denutkarsh’s picture

@gvso After applying this patch, i got the this error while configuring the settings.
[13-Jan-2017 05:58:50 Europe/Berlin] Uncaught PHP Exception Drupal\Core\Config\ImmutableConfigException: "Can not set values on immutable configuration social_auth_google.settings:client_id. Use \Drupal\Core\Config\ConfigFactoryInterface::getEditable() to retrieve a mutable configuration object" at /Applications/XAMPP/xamppfiles/htdocs/drupalV/drupal-8.2.x/core/lib/Drupal/Core/Config/ImmutableConfig.php line 27
So i change changed the submitForm function in GoogleAuthSettingsForm to this

public function submitForm(array &$form, FormStateInterface $form_state) {
    $values = $form_state->getValues();
    $config = \Drupal::getContainer()->get('config.factory')->getEditable('social_auth_google.settings');
    $config->set('client_id', $values['client_id'])
           ->set('client_secret', $values['client_secret'])
           ->save();

    parent::submitForm($form, $form_state);
  }

It started working after that.

denutkarsh’s picture

Using return array_merge(parent::getEditableConfigNames(), array('social_auth_google.settings')); instead of + operator in getEditableConfigNames() solved the issue mentioned above. After this we don't need to use the method mentioned above, it has mutable configuration now.

gvso’s picture

Status: Needs review » Needs work

Thanks @denutkarsh,

  1. +++ b/config/install/social_auth.settings.yml
    @@ -1 +1,5 @@
    +post_login_path: user
    +redirect_user_form: 0
    +disable_admin_login: 1
    +disabled_roles: []
    

    We will need a schema.yml file and use true/false instead of 1/0

  2. +++ b/config/install/social_auth.settings.yml
    @@ -1 +1,5 @@
    +post_login_path: user
    +redirect_user_form: 0
    +disable_admin_login: 1
    +disabled_roles: []
    

    Since these are new settings added, we'll need to add a hook_update_n() function for sites which update the module and add the same values as default.

  3. +++ b/src/Form/SocialAuthSettingsForm.php
    @@ -0,0 +1,102 @@
    +use Drupal\Core\Form\ConfigFormBase;
    +use Drupal\Core\Form\FormStateInterface;
    +use Drupal\Component\Utility\Html;
    

    Sort them alphabetically

gvso’s picture

denutkarsh’s picture

Status: Needs work » Needs review
FileSize
5.58 KB
1.91 KB

Here is the new patch.

gvso’s picture

Status: Needs review » Needs work

Thanks for the patch. We are almost there.

  1. +++ b/config/install/social_auth.settings.yml
    @@ -1 +1,5 @@
    +redirect_user_form: 0
    +disable_admin_login: 1
    

    We should change these two to true and false values

  2. +++ b/config/schema/social_auth.schema.yml
    @@ -0,0 +1,16 @@
    +    redirect_user_form:
    +      type: integer
    +      label: 'Redirects to Drupal user form after the user is created if checked.'
    +    disable_admin_login:
    +      type: integer
    +      label: 'Allows to Disable Social Auth login for administrator'
    

    As a consequence of the previous change, they will be "boolean" type

  3. +++ b/social_auth.module
    @@ -22,3 +22,15 @@ function social_auth_theme() {
    +  $config->set('post_login_path', 'user')->save();
    +  $config->set('redirect_user_form', 0)->save();
    

    Because $this is returned in set method, we can do something like
    $config->set($key1, $value1)
    ->set($key2, $value2)
    ->save();

denutkarsh’s picture

Status: Needs work » Needs review
FileSize
5.58 KB
1.81 KB

@gvso I forgot about the first suggestion, i thought i implemented it in the new patch. Anyways here is the new patch

  • gvso committed ca2c9af on 8.x-1.x
    Issue #2763757 by denutkarsh, felribeiro, e0ipso: Provide Social Auth...
gvso’s picture

Status: Needs review » Fixed

Thanks! I've committed the patch with some changes

  1. +++ b/config/schema/social_auth.schema.yml
    index 192f606..783ab08 100644
    --- a/social_auth.module
    
    --- a/social_auth.module
    +++ b/social_auth.module
    
    +++ b/social_auth.module
    +++ b/social_auth.module
    @@ -22,3 +22,16 @@ function social_auth_theme() {
    
    @@ -22,3 +22,16 @@ function social_auth_theme() {
     function social_auth_preprocess_login_with(&$variables) {
       $variables['base_path'] = base_path();
     }
    +
    +/**
    + * Implements hook_update_N().
    + */
    +function social_auth_update_8001(&$sandbox) {
    

    Sorry, I didn't notice this hook was created in social_auth.module. I moved it to social_auth.install

  2. +++ b/social_auth.module
    @@ -22,3 +22,16 @@ function social_auth_theme() {
    +  $config->set('post_login_path', 'user')
    +         ->set('redirect_user_form', FALSE)
    +         ->set('disable_admin_login', TRUE)
    +         ->set('disabled_roles', array())
    +         ->save();
    

    For coding standards, the last 3 set() and the save() methods has to be indented with only 2 spaces more than $config

It seems we can now focus on #2821853: Provide Social Auth settings: functionality

hussainweb’s picture

I am a little confused here. How do I access this form? I don't see anything in routing.yml or links files or anything in the patch that links it to any of the existing routes.

denutkarsh’s picture

@hussainweb After this issue was fixed implementers can implement this form by extending SocialAuthSettingsForm. See #10 for more information https://www.drupal.org/node/2763757#comment-11865493

denutkarsh’s picture

@gvso Do you think we should implement this functionality right now in implementers or should we do it after this issue is fixed https://www.drupal.org/node/2821853 ?

hussainweb’s picture

Thanks! I didn't see this in Google Auth module and was confused. This might be useful but I can wait for this.

gvso’s picture

@denutkarsh, I think it's better to fix #2821853: Provide Social Auth settings: functionality first, so site builders don't get confused if their settings don't work

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.