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

Suggested changes #7

Closed
BelleNottelling opened this issue Feb 16, 2024 · 2 comments
Closed

Suggested changes #7

BelleNottelling opened this issue Feb 16, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request

Comments

@BelleNottelling
Copy link

I noticed a few items that I'd suggest refactoring in your code:

Config handling

You're directly including into an array which will break if we decide to move the config file or change the format that it's stored in.
Starting with version 0.6.10, we've specifically added in a new class to make config file management easier and less likely to cause headache.

// Get the whole config
$dbConfig = \FOSSBilling\Config::getProperty('db', []);

// Or for example, just the DB type
$dbConfig = \FOSSBilling\Config::getProperty('db.type', 'mysql');

The first parameter is the array key to reference, written using dot-notation. The second parameter is optional and just allows you to set a default value if one isn't set.

At the very least, PATH_CONFIG contains absolute path to the config file, so include __DIR__ . '/../../../config.php'; would just be include PATH_CONFIG, but in either case we consider accessing the config file through anything other than the config class to be deprecated.

Exception class

Every time a \FOSSBilling\Exception exception is thrown, that's caught and sent via error reporting. so every time your module throws an exception to say "Domain name is not set." or even just "Not yet implemented", we get an event on our end.

\FOSSBilling\InformationException is treated as something that's simply informational and as such is not forwarded to error reporting, reducing spam and clutter in our dashboard. Ideally, the normal exception class should only be used for an actual error.

If you want to change that I can let events from the module continue to be captured and periodically forward them to you to look at, but otherwise I'm just going to add it to the blacklist so we don't get events for it, either way is fine with me.

@BelleNottelling
Copy link
Author

And on a similar note, with files like this, rather than loading the config file and then creating a PDO object, you can literally just pass the DI to it and then have access to $this->di['pdo'] and $this->di['db'].

@getpinga getpinga self-assigned this Jul 4, 2024
@getpinga getpinga added the enhancement New feature or request label Jul 4, 2024
@getpinga
Copy link
Contributor

Thanks for the information, all fixes are now implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants