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
Make CORS domains configurable in UI #13174
Conversation
core/Settings/Setting.php
Outdated
return $this->storage->getValue($this->name, $this->defaultValue, $this->type); | ||
$value = $this->storage->getValue($this->name, $this->defaultValue, $this->type); | ||
|
||
$config = $this->configureField(); |
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.
A while ago we refactored the entire settings API so we basically wouldn't need to configureField()
when getting the value as it is a performance issue. Neither should we validateValue
ideally. Any chance we can tweak the UI component to return the value in an alternative format through a flag or something?
We'd really need to make sure to leave the getValue
as it is... I probably should have added a comment about this...
$corsDomains[] = $value['domain']; | ||
} | ||
} | ||
return $corsDomains; |
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 think this could be shortened to return array_column($values, 'domain');
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.
Same for $hostnames
below
core/Settings/FieldConfig.php
Outdated
* | ||
* @var null|\Closure | ||
*/ | ||
public $prepareValue = null; |
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.
Not sure if it's better or worse (you can be the judge), but what about naming this variable $decodeValue
?
For now I've done @diosmosis suggestions. |
sounds good to me 👍 be also reusable then etc. |
@tsteur I've now implemented a new field |
That was this way before. Could change it, but that needs new translations then... Everthing else should be updated now |
Added a comment re the demo page otherwise 👍from my side. Tests will need to be fixed though (eg system setting is now returned in API) |
@tsteur I've reorganized the form demos a bit. Let me know what you think... |
Looks great 👍 for some reason I couldn't see the failing UI test in https://builds-artifacts.matomo.org/matomo-org/matomo/corssettings/28884/ ... the tests would still need to be fixed I reckon (unless I missed it in the changes files in the PR) |
tests should now be updated. Not sure why the UI tests didn't fail before... |
👍 |
So guess we are good to merge? Could anybody confirm that it's fine the trusted hosts confirmation will be removed? |
For me it would be fine to merge and that the confirmation is removed. Not really needed IMO |
Noticed something odd in the config file diagnostic page: https://builds-artifacts.matomo.org/matomo-org/matomo/corssettings/28915/UIIntegrationTest_admin_diagnostics_configfile.png The cors_domain/trusted_hosts settings are now in the |
If possible it's slightly better to keep it, but it's also not a blocker/required. How much effort would it be to keep it? |
we would need to extend the default behavior for system settings provided by plugins to implement something like that |
Need to have a look at that. Imho it doesn't make sense to list system settings there that are of type |
@diosmosis I've adjusted the way settings stored in config are handled... |
👍 looks good, will merge |
@sgiehl Actually spoke too soon, there is another failing test in ConfigReaderTest. Tried to fix myself but wasn't able to say whether the changes to EDIT: that's probably not the correct solution because then the |
@diosmosis should be fixed now |
👍 merged |
* Make CORS domain configureable in UI * Move trusted host settings to SystemSettings class * Use unique id for pluginSettings * Improve styling * Improve help text * improve code & naming * Implements new UI field array type * review adjustments * reorganize form demo * update UI files * update system test files * Improve handling of Config Settings
The new tuple input fields kind of did the same as the configuration for trusted host.
But to be able to provide the current value in the format the JS needs it, I added a new callback to the field config to convert the value. It's currently called
prepareValue
, but I'm not very happy with that name, but couldn't think of a better one. Maybe someone has an idea...Note: By moving the trusted host setting to the SystemSetting class there is a change in UI. Before the was a confirmation box shown before changing the trusted hosts. Imho it's not really needed, but we could also implement an optional confirmation for all plugin settings (that can be activated in each SystemSettings class)
fixes #13156