Skip to content

fix: give the remaining app config settings an empty-string default - #645

Merged
oc-tmueller merged 2 commits into
masterfrom
fix/appconfig-string-defaults
Sep 25, 2026
Merged

oc-tmueller merged 2 commits into
masterfrom
fix/appconfig-string-defaults

Conversation

@oc-tmueller

Copy link
Copy Markdown
Contributor

Fixes #644.

The bug

AppConfig::getAppValue() only passes a default down to IConfig for keys listed in
AppConfig::$defaults. Five keys were missing from that list, so they returned null until an
admin saved the setting: edit_groups, wopi_url, test_wopi_url, doc_format and
menu_option.

On PHP 8.1 and later that has three visible consequences:

null reaches effect
explode() in DocumentController::isAllowedEditor() an ERROR in owncloud.log on every single document open — the reported bug
DiscoveryService::getWopiUrl(): string TypeError, because a string-typed method may not return null
p() in templates/settings-admin.php (3 places) deprecation when the admin settings page is opened

The fix

The five missing keys get an '' default, which is how every other string setting in the class is
already handled. No other production code changes.

No consumer branches on these values being null, so the change is behaviour preserving — with one
exception it incidentally fixes: the admin template hides the test server block when
test_wopi_url === '', which could never be true while the value was null. With a test group set
but no test server URL the block used to render even though js/settings-admin.js left its
checkbox unchecked; it is now hidden, which is what that condition always meant to express.

zoteroAPIPrivateKey is deliberately left out: it is a user value and both of its readers guard
with !empty().

Verification

Run against a real ownCloud 11 install (owncloudci/php:8.3, sqlite), reading the values through
the real IConfig rather than a mock, with and without the fix:

=== WITHOUT the fix ===            === WITH the fix ===
edit_groups      => NULL           edit_groups      => ''
wopi_url         => NULL           wopi_url         => ''
test_wopi_url    => NULL           test_wopi_url    => ''
doc_format       => NULL           doc_format       => ''
menu_option      => NULL           menu_option      => ''
RAISED: explode(): Passing null to parameter #2 ($string) of type string is deprecated
getWopiUrl() THREW TypeError:      getWopiUrl() => ''
  Return value must be of type
  string, null returned
  • tests/unit/AppConfigTest.php gains a data-provider case per key. Its with(..., identicalTo(''))
    constraint is the load-bearing part: all five cases fail with
    Failed asserting that null is identical to '' when the $defaults entries are removed.
    (A loose with(..., '') does not catch this — PHPUnit compares loosely and null == ''.)
  • Full unit suite green: 83 tests, 209 assertions.
  • php-cs-fixer clean (0 of 45 files), phpstan level 5 "No errors", phan clean.

Branch scope

master only. The missing defaults exist on 4.2 too, but PHP 7.4 does not surface the
deprecation there, and the 4.2.4 artifact is Collabora's to build (#643) — a commit on 4.2 would
change the tree they were pointed at.

🤖 Generated with Claude Code

AppConfig::getAppValue() only passes a default down to IConfig for keys
listed in AppConfig::$defaults, so edit_groups, wopi_url, test_wopi_url,
doc_format and menu_option returned null until an admin saved them.

On PHP 8.1 and later that made every document open log an ERROR, because
DocumentController::isAllowedEditor() explodes edit_groups:

  explode(): Passing null to parameter #2 ($string) of type string is
  deprecated at lib/Controller/DocumentController.php#697

The same null also reached p() on the admin settings page, and made
DiscoveryService::getWopiUrl(), which declares a string return type,
throw a TypeError on a server with no Collabora URL configured.

No consumer branches on these values being null, with one exception that
this incidentally fixes: the admin template hides the test server block
when test_wopi_url === '', which could never be true while the value was
null. With a test group set but no test server URL the block used to
render even though js/settings-admin.js left its checkbox unchecked; it
is now hidden, which is what that condition always meant to express.

Fixes #644

Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@oc-tmueller
oc-tmueller requested a review from a team as a code owner September 25, 2026 13:09
Reinstating the Unreleased heading makes its compare link definition live
again, so point it at v4.3.1 instead of the stale v4.2.0.

Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread CHANGELOG.md


[Unreleased]: https://github.com/owncloud/richdocuments/compare/v4.2.0...master
[Unreleased]: https://github.com/owncloud/richdocuments/compare/v4.3.1...master

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There are a few other releases 4.2.1 4.2.2 4.3.2 that have gone missing from this section.
Maybe fix that in a follow-up PR (not directly related to this)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, and done as a follow-up: #646.

It turned out to be a bit more than missing entries. Five headings had no definition at all —
4.2.1, 4.2.2, 4.2.3, 4.3.0, 4.3.1 (there is no 4.3.2 yet) — and on top of that two of the
existing definitions pointed at tags that were never created, so they were live 404s:

  • [4.0.0] → compare/v3.0.1...v4.0.0 — v3.0.1 was only ever tagged -beta.1 / -rc.1
  • [3.0.1] → same missing tag, and an orphan: there is no ## [3.0.1] section
  • [2.4.0] → compare/v2.2.0...v2.4.0 — v2.4.0 is the oldest tag in the repo

#646 fetches every link in the block afterwards: all 15 return 200, and every section heading has
exactly one definition.

One thing worth knowing: #646 touches this same contiguous block, so the two conflict by
construction. Merging this PR first and letting me rebase #646 is the easy order.

@oc-tmueller
oc-tmueller merged commit 61a394e into master Sep 25, 2026
14 checks passed
@oc-tmueller
oc-tmueller deleted the fix/appconfig-string-defaults branch September 25, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Opening a document logs an ERROR on PHP 8: explode(null) because edit_groups has no default

2 participants