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
Use configuration classes instead of configuration arrays #1550
Comments
@bartlangelaan Hi Bart! It's definitely an interesting thing to discuss! I do think however, that a minimum version of PHP 8.1 is still a long way ahead of us. For our next major release we're aiming at 7.4 |
I agree - but the classses could be built in a PHP 7.4 compatible way, where they could be used with named parameters (if the user uses 8.1) or by changing the propertyes (for 7.4 / 8.0). <?php
namespace \SimpleSAML\Configuration;
class SpAuthsource {
/**
* This is a key that should be unique within the simplesamlphp
* authsources.
*/
public string $key = 'default-sp';
/**
* The entityID is the ID that will be used by other systems to identify
* this service provider. If NULL, the URL where the metadata of this sp
* can be downloaded will be used.
*/
public ?string $entityID = NULL;
/**
* This is the certificate that will be used while decrypting ...
*/
public ?SpCertificate $certificate = NULL;
/**
* If you want to use key rollover, you can provide your previous
* certificate here so ...
*/
public ?SpCertificate $oldCertificate = NULL;
/**
* Configure a new SAML service provider, that could be used to get data from
* a SAML identity provider.
*/
public function __construct(
string $key = 'default-sp',
string $entityId = NULL,
SpCertificate $certificate = NULL,
SpCertificate $oldCertificate = NULL,
) {
$this->key = $key;
$this->entityId = $entityId;
$this->certificate = $certificate;
$this->oldCertificate = $oldCertificate
}
} It could be used in either of these ways: <?php
// authsources.php
// PHP 8.1
$authsources[] = new \SimpleSAML\Configuration\SpAuthsource(
key: 'default-sp',
certificate: new \SimpleSAML\Configuration\Certificate(
certificate: __DIR__ . '../cert.crt',
),
);
// PHP 7.4 / PHP 8.0
$certificate = new \SimpleSAML\Configuration\Certificate();
$certificate->certifictate = __DIR__ . '../cert.crt';
$otherSp = new \SimpleSAML\Configuration\SpAuthsource('other-sp');
$otherSp->certificate = $certificate;
$authsources[] = $otherSp; Both ways are still fully typed. |
Agreed! I'm a bit worried about the complexity for deployers though.. They're not necessarily PHP programmers. |
Hmm, that is still not typed and thus statically checked. Also, in our case, the configuration is not for the deployers. It is part of a complex Drupal setup, where the configuration is automatically set based on environment variables and files being available on the server. Having the option to configure things with yaml could be great, but I think that that could still be as complex as having a php array. Making sure that every configuration class also has a ::fromArray() method, that could also be used for importing yaml, in the future. |
Hi @bartlangelaan ! I've made a small PoC of what I had in mind. I'm curious to hear what you think! |
Is your feature request related to a problem? Please describe.
SimpleSAMLphp is a great library that provides all sorts of configuration options so it does exactly what you want.
All options are (or at least, should be) documented on the website. However, all configuration is done using simple PHP arrays - which are very flexible, but don't have any typehints available and are not checked on runtime.
Describe the solution you'd like
It would be great, if it is possible to configure (some parts) with configuration classes - that are fully OOP and therefore typehinted.
This has a few advantages:
This can be a backwards compatible change - for example, new options could be configured with classes that are still put into an array. And all classes could have an fromArray method that would make convertions easy (and that could be automatically called by simplesamlphp, so it is not a breaking change).
Describe alternatives you've considered
Keep everything as it is. But I think that with all new PHP stuff going on, arrays are not the best option anymore.
Additional context
Of course, there are a lot of different ways these classes could be structured.
For example, if PHP 8.1 is the minimum required PHP version, this would be possible:
and for the end-user, it would be configured like this:
Is this something that would be considered?
The text was updated successfully, but these errors were encountered: