Extend configuration to allow editing text for login link (port from D7)

[irinaz]

It would be helpful to be able to change text for login url from "Federated Log In" to more specific for an organization (for example, for Sunet Login for Stanford). This features is implemented in D7 version (https://github.com/SU-SWS/stanford_ssp

[kreynen]

It exists in https://git.drupalcode.org/project/simplesamlphp_auth/-/blob/7.x-3.x/simplesamlphp_auth.admin.inc#L39 between simplesamlphp_auth_authsource and simplesamlphp_auth_grp_setup.

I'm not sure why this doesn't exist in https://github.com/backdrop-contrib/simplesamlphp_auth/blob/1.x-2.x/simplesamlphp_auth.admin.inc#L42

[irinaz]

@kreynen , thanks! Looks like there are some other features/options that were removed. I will try to open PR to bring this feature back in.

[irinaz]

@joelsteidl , @kreynen , I forked module and added missing code for the form 392fc3a. On my test site I the form shows, but input is not saved, so there should be another piece of code in D7 module that is actually saving input string.

[laryn]

I haven't had to interact with this module yet but it looks like the development branch on Drupal (3.x) has some updates that aren't in the recommended version that this module was ported from (2.x) and would need to be brought over as @irinaz has begun to do.

@irinaz Good start -- you'll need to also change variable_get to the config-related calls. It looks like the configuration names are also being shortened a bit since they are in module-specific configuration files and don't need to repeat the module name constantly. So in this case,

'#default_value' => variable_get('simplesamlphp_auth_login_link_display_name', 'Federated Log In'),

could be changed to

'#default_value' => $config->get('link_display_name'),

Two remaining tasks:

1. Make sure we have a default value for link_display_name ('Federated Log In') by adding this to the default config file, and by adding an update hook to set this default for sites that already have the module installed. (Also it would be good to check if this variable exists from Drupal 7 in case they were using 3.x dev on Drupal 7, and if so use that value).
2. Actually pull this configuration value in where needed: e.g. 1 2 3

[irinaz]

@laryn ,
I replaced code in the module file in three lines (1 2 3 )

$link = l(t('Federated Log In'), 'saml_login');
with
$link = l($link_display_name, 'saml_login');

but something is missing - now I do not see any text inside login link.

Should I add default setting in this config file simplesamlphp_auth.settings.json ? Or somewhere else?
Thanks!

[laryn]

@irinaz

1. Pulling the value from config and updating default configuration value
   We'll need to get the value of that from configuration, so instead of $link_display_name we can do a call like this:

   config_get('simplesamlphp_auth.settings', 'login_link_display_name');

   And yes, the simplesamlphp_auth.settings.json contains the default values that get set up when the module is first installed, so adding it there would be good.

2. Update hook
   For the update hook, we'll use hook_update_N (docs). Since this module is currently in 2.x you could create hook_update_1200() for this update.

3. Drupal 7 upgrades
   And for anybody coming from Drupal 7 version 3.x, we can add a few lines following the model of the variable to config conversion that is already occurring in simplesamlphp_auth_update_1000:

   • https://github.com/backdrop-contrib/simplesamlphp_auth/blob/1.x-2.x/simplesamlphp_auth.install#L55

   In the first section, add:
   $config->set('login_link_display_name', update_variable_get('simplesamlphp_auth_login_link_display_name', 'Federated Log In');

   And slightly further down:
   update_variable_del('simplesamlphp_auth_login_link_display_name');

[irinaz]

@laryn , thanks! I made changes locally, still learning :)

[laryn]

@irinaz Great! Happy to review if you push your changes to your PR. Also, a tip about the PR: if you edit the first description and add "Fixes #9" at the top, it will link it specially to this issue and make it easier to find.

[laryn]

@irinaz I've merged this and a follow up to allow translation to continue to work. Let me know if you have a chance to test it in the next few days.