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
Config class refactoring #1713
Comments
Another thought: could we drop the .ini format and switch to pure php? We could use var_export when writing. This allows us to eliminate the dependency on parse_ini_file() -- and the workaround when it is disabled. Loading would be faster and it is cacheable by APC. |
Refactoring sounds great. However I vote for keeping INI format for now, as it is universal. Editing php arrays for config is not as friendly, and also wouldn't easily support comments. |
(In [5484]) refs #1713 |
Quick grep shows the following non-core plugins are affected:
|
(In [5537]) refs #1713 - create branch for review |
How do you plan to deal with the affected plugins? will you repackage all plugins to solve the bug and require an update? I wouldn't put high priority since it won't add any feature, fix a bug and it breaks all plugins. Otherwise looks good, always nice to simplify code base! |
This would probably be a good time to fix/repackage all the affected plugins since 3rd party plugins aren't routinely tested for compatibility. Also:
|
(In [5586]) refs #1713 - to matt for review/feedback |
Wow, nice work Anthon! long due change indeed :)
|
I was trying to avoid cruft in the code base, but I agree a backward compatibility layer would reduce headaches. I'll look into this. |
in [5608], added backward compatibility layer |
Great :) After adding this layer, what are the plugins that still need updating? is there any at all? If so, can we scheduled this for Piwik 1.8? Can you think of any other risk in this change? Thanks!! |
Should work with the aforementioned plugins. I'll be busy testing this weekend. Otherwise looking good for 1.8 |
(In [5615]) refs #1713 - naming inconsistency with Config.php |
See also the double quotes bug in #2968 |
(In [5951]) refs #1713 - merge dev branch to trunk (config class refactoring) |
(In [5952]) refs #1713 - fix svn property on core/Config/Compat.php |
(In [5954]) refs #1713 - partially resolve regression from the merge |
(In [5959]) Refs #1713 Fixing NOTICE when element not in config file |
Great code cleanup and getting rid of akward code for sure :) I notice that the trunk is broken, many integration tests fail - do you see the same behavior on your local box? for some reason Jenkins fails at webtests when it should fail in the unit/integration tests, not sure why? Thanks |
(In [5960]) Refs #1713 fixes changing SU email |
(In [5961]) Refs #1713 Admin settings were not saved |
I don't yet know why the integration tests pass on dev6 but fail locally. |
OK, I can help investigate in ~ 1 hour if you're still stuck! |
|
EDIT: Strange I just lost my [Tracker] section. Thanks to Eclipse local history I could restore :) |
|
The web tests do not fail, but trunk install is broken currently "SQLSTATE[42S02]: Base table or view not found: 1146 Table 'piwik_trunk_testsvn1.piwik_option' doesn't exist" |
(In [6028]) refs #1713 - remove Config __destruct() |
(In [6029]) refs #1713 - __set() should set dirty bit |
(In [6034]) refs #1713 - not so lightweight anymore, but still an improvement by unifying Zend_Config and Piwik_Tracker_Config |
Feedback
Thanks for your nice follow up too, that was a tricky large core change :) |
I noticed a bug:
|
It's intentional and is a result of array_unmerge. |
I think it's more usable if we leave the values in the file... it's common to switch 1 to 0 to 1 etc. if the line has to be fully written each time, it's not practical. I vote for revert and do it like before, thoughts? |
This would reintroduce #1945 where changing a local setting programmatically would copy the rest of the section. |
OK, maybe we can keep track of which config elements were first read from config.ini.php, keep a flag for those, and always write them out in config.ini.php? |
(In [6075]) refs #1713 removing login=root from global.ini.php or it is removed from config.ini.php on rewrite if SU login is root |
(In [6109]) refs #1713 - workaround php 5.1.x - php 5.2.0 reference bugs when config is returned by reference |
Thanks for the better fix!! |
Open critical issues related to the new config:
@vipsoft Do you think these could be tackled in next days as to be able to release 1.7.2 ? Thanks!! |
|
Replying to matt:
I think I understand the first issue. A core/Updates/ script should suffice. I understand the potential usability issue in the second case, but I can't think of any easy fix at the moment. Replying to matt:
Off-hand, I'm going to say yes it's possible, but I haven't tried. |
OK
My idea was to remove the array_unmerge, and instead, write out only the settings that are currently written out in the local config.ini.php (not remove any setting or add any new one from global). eg. This could be done by checking configLocal before when writing out the config file? |
array_unmerge solves the problem of #1945, where changing one setting in a section would copy the rest of the section, such that the entire section was duplicated in local config.ini.php. |
Looks OK, I think some users will complain that the config file automatically removes the config settings which are similar to the global.ini.php but we can wait and see how many actually mention it, since fixing it is not trivial :) Thanks for these great refactoring ad code improvements! |
(In [6311]) Perf optimization refs #1713 |
Increase abstraction and reduce runtime dependence on Zend Framework. This provides a more uniform interface for core and plugin developers.
That is:
Read individual setting or entire section:
Change individual setting or entire section:
Split/move core/Config.ini.php into:
Remove core/Config/Ini.php and core/Tracker/Config.php.
Impact:
Must prevent recurrence of #1945. (Ideally, repair config.ini.php, perhaps by comparing against global.ini.auto-backup-before-update.php to isolate which General settings were actually custom.)
The text was updated successfully, but these errors were encountered: