-
Notifications
You must be signed in to change notification settings - Fork 62
Display the current configuration with the occ "status" command if "-v" #669
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
base: master
Are you sure you want to change the base?
Conversation
ec6d121
to
e695f76
Compare
lib/Provider/Gateway/AGateway.php
Outdated
} | ||
|
||
#[\Override] | ||
public function getConfiguration(array $schema = []): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already have a similar method called getSettings. Isn't this the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I thought it was but that "getSettings()" only returns the name and and instructions, see here:
public function getSettings(): array { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about replace the constant SCHEMA that is returned by settings by specific methods to return the values as the constant SCHEMA property provide. Maybe making this will be more clear to developers about what need to override. The schema is a bit fragile considering that haven't a contract that says what's necessary. I also thought about get back with a config class to each provider but this approach also will get back with a lot of files to implement a provider. That do you think about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed the public const SCHEMA = [ ... ]
is rather "magic" or obscured. Having documented interface methods is preferable over the current state. So we are talking here about something like (?)
public function getName():string;
public function getInstructions():string;
public function getConfigurationKeys: array;
// and what else is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can also convert this PR to a draft. Shall I? I'd like to see a status command which also displays the current configuration, but there is no hurry. Clearly, if you replace the SCHEMA property by something cleaner then this PR needs to be rewritten anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace the SCHEMA property by something cleaner
I wish to do this but I think that don't will affect this PR. Let's think more about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Starting to check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you check again your PR after the last changes at main branch? I replaced the SCHEMA by a setting class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also implemented some tests covering the status command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, seems to work again. I should probably augment the test to cover also the output with the "-v / --verbose" flag in action.
I have also added a new method to the TConfigurable trait
private static function keyFromFieldName(string $id, string $fieldName):string {
return $id . '_' . $fieldName;
}
in order to have just one place where the config keys are formed. I also saw that the handling of the provider id is slightly different in AGateway::isConfigured() and in the TConfigurable trait (the latter always ever only uses configuration->id, where AGateway looks at self::getProviderId().
19884cc
to
b9961ab
Compare
…verbose. Signed-off-by: Claus-Justus Heine <himself@claus-justus-heine.de>
b9961ab
to
0a3d918
Compare
This PR displays the configuration of the gateways providers (phone numbers etc.) if the "-v" is passed to the
occ
invocation.