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

IUserConfig #47658

Merged
merged 5 commits into from
Nov 19, 2024
Merged

IUserConfig #47658

merged 5 commits into from
Nov 19, 2024

Conversation

ArtificialOwl
Copy link
Member

@ArtificialOwl ArtificialOwl commented Aug 31, 2024

  • replace IConfig's preferences-related method
  • tests

@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 2 times, most recently from 6d81b97 to a9e2a38 Compare August 31, 2024 22:30
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from e9ec009 to cda7029 Compare September 1, 2024 19:15
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from 1fbb6a9 to 317202a Compare September 1, 2024 22:42
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from c78f304 to 979ecee Compare September 2, 2024 01:09
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from 979ecee to 41ba78f Compare September 2, 2024 01:19
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 2 times, most recently from 8edb1c1 to e510535 Compare September 2, 2024 02:31
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
lib/private/UserPreferences.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from f71bb7d to e24d29a Compare September 2, 2024 03:33
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 2 times, most recently from 32c209d to b80315b Compare September 2, 2024 14:57
lib/private/AllConfig.php Fixed Show fixed Hide fixed
lib/private/AllConfig.php Fixed Show fixed Hide fixed
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from b80315b to 0c489af Compare September 2, 2024 16:03
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from 0c489af to 6ae7f9f Compare September 2, 2024 19:50
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from ceb814d to a219133 Compare September 30, 2024 16:25
@ArtificialOwl ArtificialOwl marked this pull request as ready for review September 30, 2024 16:26
@ArtificialOwl ArtificialOwl added the 3. to review Waiting for reviews label Sep 30, 2024
@ArtificialOwl ArtificialOwl added this to the Nextcloud 31 milestone Sep 30, 2024
$value = $cache[$app][$key];
try {
$this->decryptSensitiveValue($userId, $app, $key, $value);
$value = $this->convertTypedValue($value, $typedAs ?? $this->getValueType($userId, (string)$app, $key, $lazy));

Check failure

Code scanning / Psalm

RedundantCast

Redundant cast to string
* @param string $value preference value
* @param bool $caseInsensitive non-case-sensitive search, only works if $value is a string
*
* @return list<string>

Check failure

Code scanning / Psalm

MoreSpecificReturnType

The declared return type 'list<string>' for OC\UserPreferences::searchUsersByValueString is more specific than the inferred return type 'array<array-key, mixed>'
* @since 31.0.0
*/
public function searchUsersByValueString(string $app, string $key, string $value, bool $caseInsensitive = false): array {
return $this->searchUsersByTypedValue($app, $key, $value, $caseInsensitive);

Check failure

Code scanning / Psalm

LessSpecificReturnStatement

The type 'array<array-key, mixed>' is more general than the declared return type 'list<string>' for OC\UserPreferences::searchUsersByValueString
* @param string $key preference key
* @param array $values list of preference values
*
* @return list<string>

Check failure

Code scanning / Psalm

MoreSpecificReturnType

The declared return type 'list<string>' for OC\UserPreferences::searchUsersByValues is more specific than the inferred return type 'array<array-key, mixed>'
* @since 31.0.0
*/
public function searchUsersByValues(string $app, string $key, array $values): array {
return $this->searchUsersByTypedValue($app, $key, $values);

Check failure

Code scanning / Psalm

LessSpecificReturnStatement

The type 'array<array-key, mixed>' is more general than the declared return type 'list<string>' for OC\UserPreferences::searchUsersByValues
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

Code is fine, but still want to confirm we want this name.

Reason: We use app config and system config and this was config->getUserValue.
So UserConfig might make more sense.

Yet all other places we have words for this refer it as user settings (documentation, occ commands etc), so maybe UserSettings. The only place we have that call it preferences is the DB table.

I am fine with anything regarding this if its agreed :)

lib/public/IConfig.php Outdated Show resolved Hide resolved
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch 2 times, most recently from c9ec4ca to 97ab26e Compare November 18, 2024 13:18
@ArtificialOwl
Copy link
Member Author

So, it has been renamed to IUserConfig; you happy ? :)

Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

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

🥳

Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
Signed-off-by: Maxence Lange <maxence@artificial-owl.com>
@ArtificialOwl ArtificialOwl force-pushed the enh/noid/user-preferences branch from 97ab26e to 5b4f190 Compare November 18, 2024 21:11
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Nov 19, 2024
@skjnldsv skjnldsv changed the title IUserPreferences IUserConfig Nov 19, 2024
@skjnldsv skjnldsv merged commit 899b651 into master Nov 19, 2024
186 checks passed
@skjnldsv skjnldsv deleted the enh/noid/user-preferences branch November 19, 2024 08:31
@kesselb
Copy link
Contributor

kesselb commented Nov 19, 2024

Please send a follow-up and increase the version to trigger the migration.

@st3iny
Copy link
Member

st3iny commented Nov 19, 2024

Please send a follow-up and increase the version to trigger the migration.

An exception occurred while executing a query: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'type' in 'field list' /home/richard/src/nextcloud/master/lib/private/DB/Exceptions/DbalException.php at 54

* @throws \OCP\PreConditionNotMetException if a precondition is specified and is not met
* @throws \UnexpectedValueException when trying to store an unexpected value
* @deprecated 31.0.0 - use {@see IUserConfig} directly
Copy link
Member

Choose a reason for hiding this comment

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

https://docs.nextcloud.com/server/latest/developer_manual/digging_deeper/api.html#php-unstable-api

Code from the OCP namespace must never mention anything coming from the NCU namespace. It can not require it as an argument, constant or return something from NCU.

So you can not deprecate this and neither the public interface.

* @return string
* @deprecated 31.0.0 - use {@see IUserConfig} directly
Copy link
Member

Choose a reason for hiding this comment

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

same for all methods

@nickvergessen
Copy link
Member

💥 This PR breaks talks tests.
We set a user config and fetch it in another place of the code, but receive the default value instead:
https://github.com/nextcloud/spreed/actions/runs/11911817197/job/33195693787?pr=13750#step:15:2199

string $userId,
string $app,
string $key,
string $default = '',
Copy link
Member

Choose a reason for hiding this comment

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

Breaking change, before mixed was documented and no type was casted.
At least null is used by apps and was used even in server's own method (setUserValue)

string $key,
string $default = '',
?bool $lazy = false,
): string {
Copy link
Member

Choose a reason for hiding this comment

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

When the default value is "widen" again, the return type also needs to be

@skjnldsv skjnldsv added the pending documentation This pull request needs an associated documentation update label Nov 19, 2024
@skjnldsv
Copy link
Member

skjnldsv commented Nov 19, 2024

$userPreferences = \OCP\Server::get(IUserConfig::class);
if ($preCondition !== null) {
try {
if ($userPreferences->getValueMixed($userId, $appName, $key) !== (string)$preCondition) {
Copy link
Member

Choose a reason for hiding this comment

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

this is a behaviour change, previously the precondition did not fail, when the value is not set in the database.

* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace NCU\Config\Exceptions;
Copy link
Member

Choose a reason for hiding this comment

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

Please create a documentation for this new Namespace and the way it should work (when is it going to become stable, when can devs use it... etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish developer experience enhancement pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants