-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adds new convenience methods #3
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 64
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!! Yeah these are definitely good updates. I added some suggestions to make the API's behavior a bit clearer in my eyes.
Add some tests 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that we php 7.1+ now: wanna add a buncha type hints and stuff?
This what I am using for the Redis plugin: // Redis cache server config (WP Redis plugin)
if (env('REDIS_HOST')) {
$_SERVER['CACHE_HOST'] = env('REDIS_HOST');
}
$_SERVER['CACHE_PORT'] = env('REDIS_PORT'); // WP Redis plugin expects a defined PORT
if (env('REDIS_PASSWORD')) {
$_SERVER['CACHE_PASSWORD'] = env('REDIS_PASSWORD');
}
if (env('REDIS_SALT')) {
Config::define('WP_CACHE_KEY_SALT', env('REDIS_SALT')); // salt uses a constant instead of `$_SERVER` !
}
if (env('REDIS_DB_NO')) {
$_SERVER['CACHE_DB'] = env('REDIS_DB_NO');
} Interestingly, |
In my Bedrock projects I typically extend the
Config
class with some simple static methods which I have found to have broad utility. It is the aim of this PR to have them integrated into the coreConfig
class.I think that they are a good fit for inclusion here as they promote readability while also not over-complicating or obscuring implementation details (a central tenant of the Bedrock project.)
Further I am happy to submit a PR to the Bedrock repo taking advantage of the features included here, should there be interest. Or, it could just be left to be adopted incrementally.
Note that I have not updated the test suite. Personally I think adding
@covers
annotations to the existing methods would be sufficient, since the new methods are simply wrapping existing methods.Examples of the syntax this PR enables