Skip to content

Handle config defaults better - #1619

Open
cpeel wants to merge 5 commits into
DistributedProofreaders:masterfrom
cpeel:handle-config-defaults-better
Open

Handle config defaults better#1619
cpeel wants to merge 5 commits into
DistributedProofreaders:masterfrom
cpeel:handle-config-defaults-better

Conversation

@cpeel

@cpeel cpeel commented Sep 1, 2026

Copy link
Copy Markdown
Member

This is a set of small commits that better support the default (often null) configuration values set in SiteConfig. These are always set on TEST and PROD but new sites and test configurations don't necessarily need to set them and the default of null should work.

This is prep work to remove the bash-based configuration mechanism.

@cpeel
cpeel requested a review from bpfoley September 1, 2026 23:55
@cpeel cpeel self-assigned this Sep 1, 2026
Comment thread pinc/SiteConfig.inc
if (! is_file($filename)) {
throw new RuntimeException("Configuration file $filename not found.");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A question, about this... not strictly relevant to the patch, but still... why do we make $filename an argument? We only ever use pinc/site_vars.php and that's from the one callsite at the end of this file, right? Having require $filename; like this makes life more difficult on static analysers like PHPStan: they choke on the non-literal argument, and can't see the dependency.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The thought was that for handling some of our upgrade checks it will make it easier to point to another config file to load the values within the script, effectively:

require_once(__DIR__ . "/../pinc/SiteConfig.inc");
SiteConfig::load($some_config_file);  // <-- overwrite the values from the new config file

I'm still finalizing that code now and if we don't actually need it I can rip the configurable-ness out.

Comment thread pinc/Project.inc
// if there is no forum defined, we can't do anything
if ($forum_id == null) {
return null;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A minor stylistic thing, but if you wanted to, you could fuse this with the assignment above: PHP scopes assignments within if conditions to the block containing the if.

if (($forum_id = ProjectStates::get_forum($this->state)) == null) {
    return null;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If PHP is scoping the assignment to within the if() block then I don't want it here because it's used outside the if() scope, right? The other case in ProjcatTransition.inc I do.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Perhaps surprisingly, the scoping is the block that contains the if, not the the block with the 'then' body.

function foo() {
    if (($r = rand(0, 100)) > 50) {
       echo "$r is luckier than average!\n"; // <-- in scope here
    }
    echo "You rolled $r\n";  // <-- but also in scope here
}
foo();

Comment thread pinc/ProjectState.inc
Comment thread pinc/ProjectTransition.inc Outdated
Comment thread feeds/backend.php Outdated
@cpeel
cpeel force-pushed the handle-config-defaults-better branch from 5d80301 to f738725 Compare September 3, 2026 16:01
@cpeel
cpeel force-pushed the handle-config-defaults-better branch from f738725 to 8737ad9 Compare September 3, 2026 16:05
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.

3 participants