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

Also include *.config.php files #535

Merged
merged 5 commits into from
Mar 6, 2024
Merged

Also include *.config.php files #535

merged 5 commits into from
Mar 6, 2024

Conversation

JensSpanier
Copy link
Contributor

Fixes #384

Inspired by this: https://github.com/nextcloud/server/blob/566586c609253f498c1b3f0c6c004b30ec2d998b/lib/private/Config.php#L205-L252

index.php and updater.phar still need to be regenerated.

This comment was marked as outdated.

@come-nc
Copy link
Collaborator

come-nc commented Feb 27, 2024

Please fix DCO by signing off your commit.
Also, use make index.php updater.phar to update them.
psalm errors:

ERROR: UndefinedVariable - lib/Updater.php:62:10 - Cannot find referenced variable $CONFIG (see https://psalm.dev/024)
			unset($CONFIG);

ERROR: UnresolvableInclude - lib/Updater.php:65:4 - Cannot resolve the given expression to a file path (see https://psalm.dev/106)
			/** @var array $CONFIG */
			require_once $configFile;

ERROR: RedundantConditionGivenDocblockType - lib/Updater.php:67:8 - Docblock-defined type array<array-key, mixed> for $CONFIG is never null (see https://psalm.dev/156)
			if (isset($CONFIG) && is_array($CONFIG)) {

ERROR: RedundantConditionGivenDocblockType - lib/Updater.php:67:8 - Docblock-defined type array<array-key, mixed> for $CONFIG is always array (see https://psalm.dev/156)
			if (isset($CONFIG) && is_array($CONFIG)) {

ERROR: RedundantConditionGivenDocblockType - lib/Updater.php:67:26 - Docblock-defined type array<array-key, mixed> for $CONFIG is always array (see https://psalm.dev/156)
			if (isset($CONFIG) && is_array($CONFIG)) {

You should move the /** @var array $CONFIG */ inside the if I think.
For the unset you may have so use psalm-suppress, or maybe do it at the end of the loop instead of the beginning.

@solracsf solracsf removed their request for review February 27, 2024 11:26
JensSpanier and others added 3 commits March 5, 2024 08:12
Signed-off-by: Jens Spanier <lg5i0f1jb1@konto.spnr.de>
Signed-off-by: Jens Spanier <lg5i0f1jb1@konto.spnr.de>
Signed-off-by: Jens Spanier <lg5i0f1jb1@konto.spnr.de>
@JensSpanier
Copy link
Contributor Author

Can you please review again? Thanks!

@come-nc
Copy link
Collaborator

come-nc commented Mar 5, 2024

@JensSpanier There is a leading space on some lines that codesniffer is complaining about. Please run composer run cs:fix to fix it, then make index.php updater.phar to update them, and then push the result?

Signed-off-by: Jens Spanier <lg5i0f1jb1@konto.spnr.de>
Signed-off-by: Jens Spanier <lg5i0f1jb1@konto.spnr.de>
@JensSpanier
Copy link
Contributor Author

There is a leading space on some lines that codesniffer is complaining about.

Sorry about that. Hope it looks good this time.

@come-nc come-nc merged commit b0d2bc6 into nextcloud:master Mar 6, 2024
23 checks passed
@come-nc
Copy link
Collaborator

come-nc commented Mar 14, 2024

@JensSpanier This is causing troubles in some cases as explained in nextcloud/server#44157

Is that something that you had the need for yourself?
I’m not sure what is the best fix, we can catch Throwables from the require I suppose but we cannot log the error I think.
I do not think it’s possible to provide the OC class at this point.

@JensSpanier
Copy link
Contributor Author

@come-nc I wasn't aware that the OC class can appear in configuration files.
I'm having the same problem like in #384. I'm also hosting at the provider netcup.
I think it would be enough if you could specify the config files that will be read in when calling updater.phar.
Or just another way to overwrite the config parameter datadirectory when calling updater.phar.

I also think we should reopen #384 because it's not fixed anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updater ignores additional config.php files
4 participants