Skip to content

fix: add IConfig for AppConfigOverwrite - #1366

Closed
kesselb wants to merge 1 commit into
mainfrom
add-missing-config-dependency
Closed

fix: add IConfig for AppConfigOverwrite#1366
kesselb wants to merge 1 commit into
mainfrom
add-missing-config-dependency

Conversation

@kesselb

@kesselb kesselb commented Jul 31, 2025

Copy link
Copy Markdown
Contributor

Ran into when logging into my local Nextcloud with a guest user:

Could not boot testing: OCA\Guests\AppConfigOverwrite::__construct(): Argument #2 ($config) must be of type OCP\IConfig, OC\Log\PsrLoggerAdapter given, called in /var/www/html/apps-extra/guests/lib/RestrictionManager.php on line 84

IConfig was added as a dependency for AppConfig with nextcloud/server#53449.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb self-assigned this Jul 31, 2025
@kesselb kesselb added the 3. to review Waiting for reviews label Jul 31, 2025
@kesselb
kesselb requested review from icewind1991 and skjnldsv July 31, 2025 13:52
@kesselb kesselb added the bug Something isn't working label Jul 31, 2025
@kesselb

kesselb commented Jul 31, 2025

Copy link
Copy Markdown
Contributor Author

Pinging @ArtificialOwl @nickvergessen @sorbaugh for awareness. AppConfigOverwrite ensures that guest users can only share with other guest users by enforcing a different value for a given config key. With the deprecation of getValue and the recommendation to use getValueBool, getValueString, etc. there might be some adjustments needed over here.

@skjnldsv skjnldsv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

psalm not happy ?

@nickvergessen

Copy link
Copy Markdown
Member

Sounds like you need to split the app, 1 version for 30+31, another one for 32+

@kesselb

kesselb commented Aug 1, 2025

Copy link
Copy Markdown
Contributor Author

Sounds like you need to split the app, 1 version for 30+31, another one for 32+

I would prefer that.

However if you prefer keeping one version for 30, 31 and 32 we can also add a new class "AppConfigOverwrite32" with a matching constructor and register AppConfigOverwrite for 30, 31 and AppConfigOverwrite32 for 32. Psalm will also not like that 馃ぃ

@nickvergessen

Copy link
Copy Markdown
Member

Splitting is the proper way, but up to https://github.com/nextcloud/guests/blob/main/.github/CODEOWNERS#L3

@skjnldsv

skjnldsv commented Aug 2, 2025

Copy link
Copy Markdown
Member

Split it is 馃憤

@icewind1991

Copy link
Copy Markdown
Member

#1367 for a solution that doesn't require splitting

@kesselb

kesselb commented Aug 5, 2025

Copy link
Copy Markdown
Contributor Author

Resolved via #1367

@kesselb kesselb closed this Aug 5, 2025
@kesselb
kesselb deleted the add-missing-config-dependency branch August 5, 2025 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants