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

refactor smsapi functions #889

Closed
davidcoutadeur opened this issue Apr 18, 2024 · 3 comments · Fixed by #891
Closed

refactor smsapi functions #889

davidcoutadeur opened this issue Apr 18, 2024 · 3 comments · Fixed by #891
Assignees
Milestone

Comments

@davidcoutadeur
Copy link

davidcoutadeur commented Apr 18, 2024

Currently, smsapi functions contains global variables:

  • htdocs/sendsms.php
  • lib/smsovh/smsapi-ovh.inc.php
  • lib/smsapi-twilio.inc.php
  • lib/smsapi-signal-cli.inc.php

we should remove global variables and transform the lib/* files into classes, instanciated by a function.

The appropriate configuration parameters in config.inc.php should be passed to the selected class.

Also: in function get_mobile_and_displayname in htdocs/sendsms.php, the $ldapInstance variable is not accessible. This generates a bug. We should pass it as an argument of the function.

@davidcoutadeur
Copy link
Author

This issues requires some refactoring for removing the global variables in smsapi-* files:

  • transform these files into classes
  • use a dedicated function to load the correct API driver, and populate all the corresponding variables
  • remove global variables

Note that this will change the way custom api drivers are integrated, and require at least a change note to explain how to migrate.

@davidcoutadeur
Copy link
Author

davidcoutadeur commented Apr 19, 2024

New refactoring in fbba0f9 allows to dynamically load the sms classes and pass the corresponding parameters found in the configuration.
There is no need to define any extra param in config.inc.php for specifying the class name and the corresponding parameters.

TODO:

  • add unit tests
  • update the documentation to describe the new way to write sms libs (with a class containing properties)
  • write the changelog to describe the way to migrate the existing custom sms libs to the new format

@davidcoutadeur
Copy link
Author

davidcoutadeur commented Apr 23, 2024

Release notes:

Migration to 1.6.0 requires the admin to update the smsapi files.

These files describe the connection to a SMS API for password reset.

Currently, three files are bundled in self-service-password:
- lib/smsapi-signal-cli.inc.php
- lib/smsapi-twilio.inc.php
- lib/smsovh/smsapi-ovh.inc.php

The admin can create his own smsapi file, as described in the documentation:

https://self-service-password.readthedocs.io/en/stable/config_sms.html#api

Before version 1.6.0, the smsapi file only had to contain a send_sms_by_api function.

Here are the required adaptations:

* you have to define a namespace as first directive of the file: namespace smsapi;

* you have to transform the file into a class:

class smsMyCustomApi
{
}

* if you need extra parameters, you should declare them as private properties of the class,
and define the corresponding constructor:

class smsMyCustomApi
{
    private $param1;
    private $param2;

    public function __construct($param1, $param2)
    {
         $this->param1 = $param1;
         $this->param2 = $param2;
    }
}


* you should adapt the parameters configured above in the send_sms_by_api function, by using $this->my-param:

    function send_sms_by_api($mobile, $message) {
        if (!$this->param1 || !$this->param2 ) {
          error_log('Missing parameter');
          return 0;
        }
        ...
        return 1;
    }

* the configuration keys present in config.inc.php or config.inc.local.php will automatically be passed
to the smsapi constructor. In the example shown above, we should define two parameters in config.inc.local.php:

$param1 = "value1";
$param2 = "value2";

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

Successfully merging a pull request may close this issue.

1 participant