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

[IAppConfig] format values based on type #43258

Closed
wants to merge 1 commit into from

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Feb 1, 2024

returns formated values based on type when calling getAllValues() and searchValues()

note: deprecated method getValues() use getAllValues() but if the config value is defined as MIXED (default), the value is kept as a string.

original:

{
    "key1": "value1",
    "key2": "value0",
    "key6": "1",
    "key7": "{\"test1\":1,\"test2\":2}",
    "test8": "1",
    "key3": "value0",
    "key4": "3",
    "key5": "3.14"
}

new:

{
    "key1": "value1",
    "key2": "value0",
    "key6": true,
    "key7": {
        "test1": 1,
        "test2": 2
    },
    "test8": true,
    "key3": "value0",
    "key4": 3,
    "key5": 3.14
}

@ArtificialOwl ArtificialOwl added this to the Nextcloud 29 milestone Feb 1, 2024
@ArtificialOwl ArtificialOwl added the 3. to review Waiting for reviews label Feb 1, 2024
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch 2 times, most recently from 56af4d1 to 83b4b44 Compare February 2, 2024 08:37
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch from 83b4b44 to cf673bc Compare February 29, 2024 12:57
Copy link
Contributor

@emoral435 emoral435 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐘

@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch 4 times, most recently from 16273db to 58c6124 Compare March 11, 2024 13:50
@Altahrim Altahrim mentioned this pull request Mar 12, 2024
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch from 58c6124 to b984382 Compare March 12, 2024 10:45
@Altahrim Altahrim mentioned this pull request Mar 14, 2024
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch from b984382 to 0443c8f Compare March 14, 2024 10:21
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch from 0443c8f to d84954c Compare March 15, 2024 09:24
@ArtificialOwl ArtificialOwl added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Mar 15, 2024
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch 2 times, most recently from 43338de to a697c94 Compare March 15, 2024 12:23
@ArtificialOwl
Copy link
Member Author

ArtificialOwl commented Mar 15, 2024

I have found a performance issue as this PR would load lazy config values when searching for the enabled apps at boot.
This is fixed, and test should be green now 🤞

Adding also a test to confirm that no lazy config values are loaded during the normal boot of Nextcloud

@skjnldsv
Copy link
Member

There was 1 failure:

1) Test\AppConfigTest::testLazyConfigNotLoaded
It seems lazy config values are loaded during the boot of Nextcloud ?
Failed asserting that true matches expected false.

/home/runner/work/server/server/tests/lib/AppConfigTest.php:231

@Altahrim Altahrim mentioned this pull request Mar 18, 2024
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/returns-formated-app-values branch from 0cf37c8 to a667224 Compare March 19, 2024 09:03
@ArtificialOwl
Copy link
Member Author

There was 1 failure:

1) Test\AppConfigTest::testLazyConfigNotLoaded
It seems lazy config values are loaded during the boot of Nextcloud ?
Failed asserting that true matches expected false.

/home/runner/work/server/server/tests/lib/AppConfigTest.php:231

test was not related to the PR. I will implement it in a separated PR

@skjnldsv skjnldsv added 3. to review Waiting for reviews technical debt and removed 2. developing Work in progress labels Mar 19, 2024
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have liked this covered in Tests before we get this in

@skjnldsv skjnldsv deleted the enh/noid/returns-formated-app-values branch April 3, 2024 08:04
@ArtificialOwl ArtificialOwl restored the enh/noid/returns-formated-app-values branch April 3, 2024 08:34
@skjnldsv skjnldsv removed this from the Nextcloud 29 milestone Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants