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

Refactor database and config init #8527

Merged
merged 3 commits into from Apr 11, 2018

Conversation

Projects
None yet
3 participants
@murrant
Member

murrant commented Apr 9, 2018

Connect to the database without loading full config
Load config completely so post-processing is always done consistently.
Erase existing $config when loading, fixes issues in case we load the config twice.
If the database is not connected, don't try to load database settings. (Fixes some db errors on install)
Attempt to remove $config access/modification before init.php
Remove usage of db_name, that might not match the connected database.
Centralize db config loading, so we consistently apply db_test database settings.
Many of these changes are influenced by Laravel port.

Tested:

  • install
  • validate (web + cli with/without db_test)
  • tests (with/without db/snmpsim)
  • build-base.php (with/without command line options/config.php/db_test)
  • poller.php

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

Refactor database and config init
Connect to the database without loading full config
Load config completely so post-processing is always done consistently.
Erase existing $config when loading, fixes issues in case we load the config twice.
If the database is not connected, don't try to load database settings. (Fixes some db errors on install)
Attempt to remove $config access/modification before init.php
Remove usage of db_name, that might not match the connected database.
Centralize db config loading, so we consistently apply db_test database settings.
Many of these changes are influenced by Laravel port.

@murrant murrant added the Refactor label Apr 9, 2018

murrant added some commits Apr 9, 2018

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Apr 9, 2018

The inspection completed: 1 new issues, 4 updated code elements

@@ -53,7 +53,7 @@
// Check for install.inc.php
if (!file_exists('../config.php') && $_SERVER['PATH_INFO'] != '/install.php') {
// no config.php does so let's redirect to the install
header("Location: {$config['base_url']}/install.php");
header("Location: /install.php");

This comment has been minimized.

@laf

laf Apr 9, 2018

Member

If install is in a sub dir then this will break those installs.

This comment has been minimized.

@murrant

murrant Apr 9, 2018

Member

$config is not set here, as far as I can tell. It is before init.php

@laf

This comment has been minimized.

Member

laf commented Apr 9, 2018

How come you've switched to doing a db query to get the db name rather than just use config?

@murrant

This comment has been minimized.

Member

murrant commented Apr 9, 2018

If you use an alternative test database with db_test_name, db_name will not point to the right name.

@murrant

This comment has been minimized.

Member

murrant commented Apr 9, 2018

The main goal is to make loading the config and connecting to the database not depend on each other.

@murrant

This comment has been minimized.

Member

murrant commented Apr 11, 2018

@librenms/reviewers any issues with this still?

@laf

This comment has been minimized.

Member

laf commented Apr 11, 2018

Let me test again

@laf

laf approved these changes Apr 11, 2018

LGTM. Tested poller, discovery and webui.

@laf laf merged commit d841625 into librenms:master Apr 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@laf laf deleted the murrant:init-refactor branch Apr 11, 2018

@laf laf referenced this pull request Apr 11, 2018

Closed

mib_dir config variable missing - issue with pre-commit.php #8540

5 of 5 tasks complete
@murrant

This comment has been minimized.

Member

murrant commented Apr 12, 2018

/me goes off to rebase the Laravel PR...

TheMysteriousX added a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018

refactor: Refactor database and config init (librenms#8527)
* Refactor database and config init
Connect to the database without loading full config
Load config completely so post-processing is always done consistently.
Erase existing $config when loading, fixes issues in case we load the config twice.
If the database is not connected, don't try to load database settings. (Fixes some db errors on install)
Attempt to remove $config access/modification before init.php
Remove usage of db_name, that might not match the connected database.
Centralize db config loading, so we consistently apply db_test database settings.
Many of these changes are influenced by Laravel port.

* Some safety so we don't assign strings to numeric port field
Smooth out phpunit bootstrap

* Fix a couple of scrutinizer warnings.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.