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

AL-553 - Added Config class to use in lieu of constants #1162

Merged
merged 3 commits into from
Aug 31, 2016

Conversation

TeslaDethray
Copy link
Contributor

@TeslaDethray TeslaDethray commented Aug 27, 2016

The old method of setting configurabls constnats necessitated the creation of a Configurator object, properly belonging to WP-CLI's dispatcher, to make use of Terminus' configuration. I've separated the Terminus Config from the Configurator and ended the widespread use of constants in favor of static methods from the Config class.

Also:

  • Removed a whole lot of duplicative code
  • Standardized the format of models and collections

@pantheon-mentionbot
Copy link

@TeslaDethray, thanks for your PR! By analyzing the blame information on this pull request, we identified @bensheldon and @joshkoenig to be potential reviewers

@TeslaDethray TeslaDethray force-pushed the addition/10_models branch 2 times, most recently from f50658b to 12afdfd Compare August 30, 2016 16:51
@greg-1-anderson
Copy link
Member

@TeslaDethray Looks like you need a mkdir somewhere around here. There may be other places; might be convenient to add mkdir as a side effect of Config::get('cache_dir'), but better practice to ensure the directory is created as needed prior to writing.

@TeslaDethray TeslaDethray force-pushed the addition/10_models branch 4 times, most recently from c091307 to 15f5cd9 Compare August 30, 2016 18:25
@TeslaDethray TeslaDethray force-pushed the addition/10_models branch 2 times, most recently from 0394ac9 to 30b8cad Compare August 30, 2016 22:07
@TeslaDethray TeslaDethray changed the title AL-553 - Added Config class to use in lieu of constants [WIP] AL-553 - Added Config class to use in lieu of constants Aug 30, 2016
@coveralls
Copy link

coveralls commented Aug 30, 2016

Coverage Status

Coverage increased (+0.6%) to 18.362% when pulling 30b8cad on addition/10_models into c2538bf on master.

/**
* @var string
*/
protected $collected_class = 'Terminus\Models\SiteAuthorizations';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be singular.


/**
* Changes connection mode
*
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did that function disappear or were the inline docs just duplicated?

@coveralls
Copy link

coveralls commented Aug 31, 2016

Coverage Status

Coverage increased (+0.5%) to 18.334% when pulling d588402 on addition/10_models into c2538bf on master.

@ronan
Copy link
Contributor

ronan commented Aug 31, 2016

I'm a big supporter of the concept but have a couple of notes:

  1. We should consider trying to use standard Symfony components as much as possible rather than building our own. This will mean less code to maintain and better interoperability with other components: http://symfony.com/doc/current/components/config.html

  2. I would rather see some sort of dependency injection used for configuration rather than the global access pattern: $config = Config::getAll();

Taken together, the logical conclusion of this would be to make sure all of our code works with the Symfony DI container: http://symfony.com/doc/current/components/dependency_injection.html
Doing so will take some work (and some rethinking) but ultimately things will get a lot easier with config, dependency management, dependency injection all handled by a pretty robust and trusted component.

Given how much there is to do in this refactor and that either of these items could send us down a rabbit-hole, I'm comfortable endorsing this PR as-is though.

+1

@TeslaDethray TeslaDethray merged commit 0b6f34e into master Aug 31, 2016
@TeslaDethray TeslaDethray deleted the addition/10_models branch August 31, 2016 18:24
@dustinleblanc
Copy link
Contributor

@ronan there are a few other good DI containers out there, Symfony isn't the only option. League's container can work really well.

@greg-1-anderson
Copy link
Member

@dustinleblanc I was about to suggest league/container as well! I find it much nicer to work with than the Symfony DI container.

Also, I have been meaning to write a config system for Robo based on the Symfony Config class for some time. I agree with Ronan's conclusion that doing that at this point would be a rabbit-hole (said after already merged :P )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants