Closed frankiejarrett closed 6 years ago
💯 so cool!
Added a bunch of thoughts to the README to explain how this all works.
@fjarrett Looks great so far!
This will obviously need a robust set of unit tests to make sure we can rely on these changes to work across the board. You've mentioned in the description that you'll take care of tests as well, but feel free to ping me when you need help with anything!
Travis is missing a composer install
step. That's why you're getting the error that the vendor/autoload.php
file is missing.
@schlessera OK I've thrown a bunch of test scenarios together. I found it difficult to find a simple testing pattern since we have to create actual files then include them to check their values and constants can only be defined once. As a result, things turned out a little more heavy-handed than I prefer, and I'm open to suggestions for simplifying the whole thing.
@fjarrett Hehe, you went all out on that one, great job so far! 😄
I'll probably only get around to doing a thorough review after new year's day, but here are already a few quick observations:
bin
folder. By convention, bin
normally stands for binary
, and contains the executable files of a project. A better name would be fixtures
, for example.wp-config-sample.php
gives the impression it would be a copy from the WP core files. However, the last line is commented out. I'd either rename the file to make it clear it is not a valid config file, or restore the last line.If you have questions about any of the above, feel free to ask away or ping me on Slack!
Thanks @schlessera! First round of changes pushed based on your feedback.
@fjarrett Looks good. I think you misinterpreted my comment about the short description, though. If you click through the link I provided to WPCS, you can see that it requires the third person singular form.
So, instead of this:
Apply formatting to a config value.
WPCS requires this:
Applies formatting to a config value.
You write the short description as if the sentence would start with the pronoun it
, referring to the function/method:
[It] Applies formatting to a config value.
Travis is throwing disk quota exceeded
errors - hoping that will clear up on its own...
Seems to have been a temporary issue, a restart got us through successfully.
Merging this as is so it can more easily be used and tested. Any improvements/fixes will be separate PRs.
@schlessera so the thing to do here first is to review wp-cli/wp-config-transformer
for bundling? Or has it been sufficiently reviewed and we just need to do a PR on wp-cli/wp-cli
to bundle it?
(Am reviewing this PR anyway...)
Edit: meant to do this and following comment on https://github.com/wp-cli/config-command/pull/44.
As it's just one file and phpunit tests I'm wondering if it wouldn't be best/simplest to merge wp-cli/wp-config-transformer
into this repo.....?
One of the original goals was to create a stand-alone library to deal with wp-config.php modifications, as having a WP-CLI command to do it is just one individual use case. There are other instances where such a library can be useful as well.
Hmm, okay, but having all these separate repos is a logistical nightmare to be honest.
So this repo is reviewed and just needs to be bundled then?
Edit: And released.
(Unfortunately I'm very stuck for time today and won't get back to doing WP-CLI stuff until tonight.)
Oh okay I see it's already released. And will be "bundled" with config-command
as soon as https://github.com/wp-cli/config-command/pull/44 is merged.
So the only (but big) to-do item is to change make-phar.php
to include it?
Edit: turns out no change needed for make-phar.php
.
@danielbachhuber API here is pretty self-explanatory. So far it seems to work pretty well.
Going continue to poke at it and get a test framework in place so we can throw in a ton of scenarios.
Examples:
Examples using raw values:
Variables also work: