Skip to content

feat: add config header#58886

Open
Keeper-of-the-Keys wants to merge 2 commits intonextcloud:masterfrom
Keeper-of-the-Keys:config-header
Open

feat: add config header#58886
Keeper-of-the-Keys wants to merge 2 commits intonextcloud:masterfrom
Keeper-of-the-Keys:config-header

Conversation

@Keeper-of-the-Keys
Copy link
Contributor

@Keeper-of-the-Keys Keeper-of-the-Keys commented Mar 12, 2026

Summary

config.php is re-rendered by nextcloud regularly removing comments and rendering constants into their values, nextcloud supports using multiple config files and renders their content into the main config.php without modifying the external files.
This PR adds a header to config.php warning the admin/user of this and suggesting the use of the multiple config files mechanism.

TODO

Checklist

AI (if applicable)

  • The content of this PR was partly or fully generated using AI

@Keeper-of-the-Keys Keeper-of-the-Keys requested a review from a team as a code owner March 12, 2026 12:03
@Keeper-of-the-Keys Keeper-of-the-Keys requested review from Altahrim, ArtificialOwl, CarlSchwan and salmart-dev and removed request for a team March 12, 2026 12:04
@Keeper-of-the-Keys
Copy link
Contributor Author

Please note that I asked on NC Talk how I could test this code before creating the PR and did not get an answer, I would love to test it before we move forward, I solidify my claim that this works as expected.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Nice addition! I think there are some small things to improve, but overall it looks very good already. I will let the CI run once these things have been adjusted.

@joshtrichards joshtrichards changed the title Config header feat: add config header Mar 13, 2026
@Keeper-of-the-Keys
Copy link
Contributor Author

Do I need to do anything more or regularly update the branch until it is merged or is my side of things done?

@provokateurin
Copy link
Member

You need to update the tests to account for your changes. You can see the failures here: https://github.com/nextcloud/server/actions/runs/23019402943/job/66860023752?pr=58886. You can run them locally using ./autotest.sh sqlite tests/lib/ConfigTest.php. After they pass, just amend the commit.

Please squash all your commits and make sure they follow https://www.conventionalcommits.org/en/v1.0.0/.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Tests need to be updated.

@Keeper-of-the-Keys
Copy link
Contributor Author

I just tried to run a rebase, because I originally started working on this a half year ago there are ~5600 commits in the squash commits including conflicts that I need to resolve, it may be better to close this PR and reopen a new branch based on the current HEAD and the comments that were here, unless there is some better tool to do this.

@provokateurin
Copy link
Member

That's not necessary. If you want I can rebase your PR for you. I could also adjusts the tests, but maybe you'd like to do that yourself? Just let me know what you need from us :)

@Keeper-of-the-Keys
Copy link
Contributor Author

I'm happy to try on the one hand, on the other it seems to be more work than just creating a new branch and a new PR...

What I tried to do was git rebase -i HEAD~11

If there is a better/faster way to do this I'm happy to learn but it seems like a waste of time to me to start resolving merge conflicts in the past half year of commits that were for sure already resolved/were never an issue to begin with.

@provokateurin
Copy link
Member

A git fetch && git rebase origin/master should be enough.

@Keeper-of-the-Keys
Copy link
Contributor Author

Keeper-of-the-Keys commented Mar 16, 2026

That doesn't look like it squashed everything unless I'm very much mistaken.

/Edit - I ran:
git fetch upstream && git rebase upstream/master

@provokateurin
Copy link
Member

Yes sorry, I also realized my instructions might not match your setup.

@provokateurin
Copy link
Member

Can you adjust the tests now so they pass?

@Keeper-of-the-Keys
Copy link
Contributor Author

When you say adjust the tests what exactly do you mean?

@miaulalala
Copy link
Contributor

When you say adjust the tests what exactly do you mean?

You need to add your new header to the text in \Test\ConfigTest::TESTCONTENT

You can run the test locally by running NOCOVERAGE=1 ./autotest.sh sqlite tests/lib/ConfigTest.php

If you need more help, join us in our community dev chat

@miaulalala
Copy link
Contributor

Cypress and performance won't run on your fork, so nothing to do there.

Can you squash your commits into a single one and sign the commit?

easiest way is:

git fetch origin
git reset --soft origin/master
git commit -sm "feat: add config header"
git push --force-with-lease

Signed-off-by: E.S. Rosenberg a.k.a. Keeper of the Keys <es-github@rosenberg.org.il>
@Keeper-of-the-Keys Keeper-of-the-Keys requested review from a team and skjnldsv as code owners March 18, 2026 17:38
@Keeper-of-the-Keys Keeper-of-the-Keys requested review from sorbaugh and szaimen and removed request for a team March 18, 2026 17:38
@Keeper-of-the-Keys
Copy link
Contributor Author

OOF, looks like I should have rebased first based on the amount of files changed.

@szaimen szaimen removed their request for review March 18, 2026 17:40
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.

[Bug]: Upgrade process re-renders config.php in a less readable/useable fashion

4 participants