Extract INI file merging logic in Config class and move to new IniFileChain class. #7312

diosmosis · 2015-02-27T04:21:40Z

This is an intermediate step in allowing Config/Plugin\Manager to exist in DI. Will allow reading conifg w/o needing to instantiate Config instance (so we can read Plugins to load list before creating Config instance).

Remaining TODO:

revise Config tests for new logic (the tests don't need to test for merging logic)
move INI merging tests (and write new ones if needed) to unit test for IniFileChain
make sure UI tests pass

diosmosis · 2015-03-01T08:29:51Z

@piwik/core-team This change is ready for review; don't merge for any 2.11 release, only 2.12 or later.

mattab · 2015-03-01T21:12:56Z

Adding 2.12.0 milestone (feel free to add milestone to Pull Requests when they must be merged in particular versions)

mnapoli · 2015-03-01T21:18:21Z

👍

…eChain class. This is an intermediate step in allowing Config/Plugin\Manager to exist in DI.

… inherit and get tests to pass.

…catch the exception and augment message w/ file being read in IniFileChain. Also includes some mild refactoring to IniFileChain.

mattab · 2015-03-12T01:40:30Z

tests/PHPUnit/Unit/ConfigTest.php

                $header . "[General]\ndebug = 1\n\n[Tracker]\nanonymize = 2\n\n",
            )),

-            array('sort, common sections before new section', array(
+            /*TODO: is this test needed?


I guess this test is needed, is there an issue with it?

It tests that the order of the sections is first sections that have data in common w/ global + config, and then data that is only in config.ini.php. I don't think I can easily replicate this behavior, and I'm not sure if it's important. Is it?

It's important that sections are stored in config.ini.php in the same order as they are in global.ini.php
this is because when we write the dump to file it should put [database] at top, then general, tracker, debug, and after only the plugins etc. we could also hardcode the order of sections if that's easier

Fixed in most recent commit.

mattab · 2015-03-12T01:47:53Z

Looks good to me.

I propose that we merge this for 2.13.0, to give us more time to test this. if you are confident about it and tests, we could merge it sooner

Conflicts: core/Config.php

…onfig, the config file is erased. Also sort config sections w/ case insensitive search.

diosmosis · 2015-03-12T11:42:29Z

Merging in 2.13 is ok w/ me, it would be nice if it could be done as soon as the last minor 2.12 is out. Once this is merged, I will do two more refactorings to get Config + Plugin\Manager stored in DI, then I can make test related code a lot simpler.

…leton).

diosmosis · 2015-03-12T12:29:28Z

@mattab Forgot about a bug I found in core while doing this pull request. In Config::array_merge_recursive_distinct (and now IniFileChain::array_merge_recursive_distinct) the code will only merge merge array values if they have the same key. But since the arrays being merged for config data are not associative, nothing ever gets merged. For example:

array_merge_recursive_distinct(
    array(
        'Plugins' => array('CustomAlerts', 'WhiteLabel')
    ),
    array(
        'Plugins' => array('SitesManager', 'CoreAdminHome')
    ),
)

will result in

array('Plugins' => array('SitesManager', 'CoreAdminHome'))

Is this the correct behavior? If not, can we fix it if it means users' common.config.ini.php might behave differently? I'm guessing not, let me know.

mattab · 2015-03-13T00:01:07Z

Is this the correct behavior? If not, can we fix it if it means users' common.config.ini.php might behave differently? I'm guessing not, let me know.

It is the correct behavior. In the global.ini.php there are loads of plugins in [Plugins] but imagine a user disables one... we should then not merge the new [Plugins] with the old one or the disabled plugin will not be disabled

…ngs file chain the expected behavior.

…ar in file chain.

mnapoli · 2015-03-30T21:56:04Z

It seems there is this test that fail and then we can merge: https://travis-ci.org/piwik/piwik/jobs/54478316#L610 Maybe that's expected

1) Piwik\Tests\Integration\ReleaseCheckListTest::test_phpFilesStartWithRightCharacter
File /home/travis/build/piwik/piwik/tmp/tmp.config.ini.php does not start with ; <?php exit;
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'[GeneralSecti'
+'; <?php exit;'

…othingIfThereAreNoChanges so it doesn't trigger failure in ReleaseCheckListTest.

…roperty has no effect'.

…ings file is completely empty and add test case. Also add type hint to IniFileChain::__construct.

diosmosis · 2015-03-31T01:39:07Z

UI test failures should be fixed in #7583.

Extract INI file merging logic in Config class and move to new IniFileChain class for future re-use.

…ct order. Bug occurs due to incorrect sorting that sorts sections according to which files they appear in, but not where they appear in individual files.

…s been added to Config.

mattab added this to the Piwik 2.12.0 milestone Mar 1, 2015

diosmosis added 5 commits March 1, 2015 13:34

Extract INI file merging logic in Config class and move to new IniFil…

1ee1607

…eChain class. This is an intermediate step in allowing Config/Plugin\Manager to exist in DI.

Add tests for IniFileChain, compose IniFileChain in Config instead of…

48192de

… inherit and get tests to pass.

Fixing Plugin\ManagerTest integration test.

8ff84e6

Attempting to fix Installation UI tests.

e3f6ab4

Fix Installation UI tests.

28734b5

mnapoli mentioned this pull request Mar 1, 2015

[WIP] Update config for PerformanceInfo pluging #7318

Closed

Forgot to add new unit test for IniFileChain.

203b885

diosmosis force-pushed the config_ini_merge_refactor branch from a052b38 to 203b885 Compare March 1, 2015 23:11

diosmosis mentioned this pull request Mar 1, 2015

Add filename property to IniReadingException... matomo-org/component-ini#3

Closed

Do not use missing getIniFile() exception message in Config, instead …

d2d3992

…catch the exception and augment message w/ file being read in IniFileChain. Also includes some mild refactoring to IniFileChain.

mattab reviewed Mar 12, 2015
View reviewed changes

mattab modified the milestones: Piwik 2.13.0, Piwik 2.12.0 Mar 12, 2015

diosmosis added 2 commits March 12, 2015 03:14

Merge branch 'master' into config_ini_merge_refactor

a439679

Conflicts: core/Config.php

Fix a regression (w/ new test) where if no changes in user settings c…

745c41f

…onfig, the config file is erased. Also sort config sections w/ case insensitive search.

Remove TODO from Config.php (about test event Config.createConfigSing…

ddc9530

…leton).

Merge branch 'master' into config_ini_merge_refactor

8e32ff4

diosmosis added 2 commits March 15, 2015 10:28

Fix unit test, make array settings being overwritten in default setti…

c923086

…ngs file chain the expected behavior.

Force sections in INI file output to be ordered by when sections appe…

9dc5238

…ar in file chain.

diosmosis mentioned this pull request Mar 30, 2015

Remove LocalTracker class from use in tests. #7563

Closed

1 task

diosmosis added 2 commits March 30, 2015 15:53

Merge branch 'master' into config_ini_merge_refactor

b8c0125

Remove test config.ini.php file in ConfigTest::test_forceSave_writesN…

77d1ae5

…othingIfThereAreNoChanges so it doesn't trigger failure in ReleaseCheckListTest.

diosmosis removed the Needs Review PRs that need a code review label Mar 30, 2015

diosmosis added 2 commits March 30, 2015 16:28

Attempting to fix travis error 'indirect modification of overloaded p…

459efa7

…roperty has no effect'.

Fix issue where reference not returned by IniFileChain::get when sett…

2bb3842

…ings file is completely empty and add test case. Also add type hint to IniFileChain::__construct.

diosmosis added a commit that referenced this pull request Mar 31, 2015

Merge pull request #7312 from piwik/config_ini_merge_refactor

b8db68b

Extract INI file merging logic in Config class and move to new IniFileChain class for future re-use.

diosmosis merged commit b8db68b into master Mar 31, 2015

diosmosis deleted the config_ini_merge_refactor branch March 31, 2015 01:40

diosmosis pushed a commit that referenced this pull request Apr 7, 2015

Refs #7312, fix for last commit if dumping changes and new section ha…

452d91f

…s been added to Config.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract INI file merging logic in Config class and move to new IniFileChain class. #7312

Extract INI file merging logic in Config class and move to new IniFileChain class. #7312

diosmosis commented Feb 27, 2015

diosmosis commented Mar 1, 2015

mattab commented Mar 1, 2015

mnapoli commented Mar 1, 2015

mattab Mar 12, 2015

diosmosis Mar 12, 2015

mattab Mar 12, 2015

diosmosis Mar 15, 2015

mattab commented Mar 12, 2015

diosmosis commented Mar 12, 2015

diosmosis commented Mar 12, 2015

mattab commented Mar 13, 2015

mnapoli commented Mar 30, 2015

diosmosis commented Mar 31, 2015

Extract INI file merging logic in Config class and move to new IniFileChain class. #7312

Extract INI file merging logic in Config class and move to new IniFileChain class. #7312

Conversation

diosmosis commented Feb 27, 2015

diosmosis commented Mar 1, 2015

mattab commented Mar 1, 2015

mnapoli commented Mar 1, 2015

mattab Mar 12, 2015

Choose a reason for hiding this comment

diosmosis Mar 12, 2015

Choose a reason for hiding this comment

mattab Mar 12, 2015

Choose a reason for hiding this comment

diosmosis Mar 15, 2015

Choose a reason for hiding this comment

mattab commented Mar 12, 2015

diosmosis commented Mar 12, 2015

diosmosis commented Mar 12, 2015

mattab commented Mar 13, 2015

mnapoli commented Mar 30, 2015

diosmosis commented Mar 31, 2015