-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Handle case of first CORS whitelist add-delete #29742
Conversation
@@ -136,11 +136,14 @@ public function addDomain($domain) { | |||
*/ | |||
public function removeDomain($id) { | |||
$userId = $this->userId; | |||
$domains = json_decode($this->config->getUserValue($userId, 'core', 'domains')); | |||
|
|||
$domains = json_decode($this->config->getUserValue($userId, 'core', 'domains'), true); |
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.
why not also add "[]"
here for consistency since you did above ?
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.
True, in theory removeDomain
is only called if there is something to remove. But I guess it should be resilient and silent remove something from nothing if asked.
if ($id >= 0 && $id < count($domains)) { | ||
unset($domains[$id]); | ||
$this->config->setUserValue($userId, 'core', 'domains', json_encode($domains)); | ||
if (count($domains)) { |
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.
empty($domains)
?
5e9412b
to
64e1461
Compare
0b969d9
to
d74f122
Compare
d74f122
to
59cc6a3
Compare
@ anybody who cares - I think this swill pass unit tests and also actually works now. CI will tell. Ready for review again. |
Codecov Report
@@ Coverage Diff @@
## master #29742 +/- ##
=========================================
Coverage 62.61% 62.61%
+ Complexity 17582 17581 -1
=========================================
Files 1037 1037
Lines 57914 57914
=========================================
Hits 36263 36263
Misses 21651 21651
Continue to review full report at Codecov.
|
@phil-davis please backport for 10.0.4 |
stable10: #29749 |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Description
Handle a default array the first time when there are no CORS whitelist entries.
When removing the last entry, delete the value completely.
Related Issue
Motivation and Context
Be able to add and delete CORS whitelist entries.
How Has This Been Tested?
Add and delete various entries manually in the webUI, refreshing to confirm the changes.
Types of changes
Checklist: