Skip to content

Feature/configure engine #1577

Merged
merged 1 commit into from Aug 31, 2013

2 participants

@ADmad
CakePHP member
ADmad commented Aug 30, 2013

Using suffix "Reader" felt wrong as the "readers" don't just read, they dump config vars to file too.

@markstory markstory and 1 other commented on an outdated diff Aug 30, 2013
lib/Cake/Configure/IniEngine.php
* 'yes', 'no', 'on', 'off', 'null' are handled. These values will be
* converted to their boolean equivalents.
*
* @package Cake.Configure
* @see http://php.net/parse_ini_file
*/
-class IniReader implements ConfigReaderInterface {
+class IniEngine implements ConfigEngineInterface {
@markstory
CakePHP member
markstory added a note Aug 30, 2013

I wonder if Cake\Configure\Engine\Ini isn't a simpler classname. Or perhaps Cake\Configure\Engine\IniConfig. I don't think there is anything wrong with this currently, just thinking out loud. We'll have 3 things that use 'engines'. Cache, Log and now Configure all have engines.

@ADmad
CakePHP member
ADmad added a note Aug 31, 2013

Cake\Configure\Engine\IniConfig sounds fine. I will make changes accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@markstory
CakePHP member

Code changes seem simple enough. I guess the config() method and extension registry will be done separately?

@ADmad
CakePHP member
ADmad commented Aug 31, 2013

I have some reservations about the changes to Configure::load()/config() we discussed. If we start including extension always like Configure::load('foo.ini') if you want to use another engine we would have to change the key and use Configure::load('foo.php').
Also what if someone has a db engine? You would have to use a dummy extension like .db and map it to your engine. That doesn't seem right.

@markstory
CakePHP member

That is a very good point. Let's leave things as they are unless @lorenzo can think of a good way to solve this predicament.

@ADmad
CakePHP member
ADmad commented Aug 31, 2013

@markstory Class names updated.

@markstory
CakePHP member

Looks good to me. Can you update the migration guide as well?

@markstory markstory merged commit ab83f42 into cakephp:3.0 Aug 31, 2013

1 check failed

Details default The Travis CI build failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.