Opened 4 years ago

Closed 2 years ago

Last modified 23 months ago

#1713 closed New feature (fixed)

Config class refactoring

Reported by: vipsoft Owned by: vipsoft
Priority: major Milestone: Piwik 1.8
Component: Core Keywords:
Cc: Sensitive: no

Description (last modified by vipsoft)

Increase abstraction and reduce runtime dependence on Zend Framework. This provides a more uniform interface for core and plugin developers.

That is:

  • use Piwik_Config::getInstance() instead of Zend_Registry::get('config')
  • use Piwik_Config::getInstance() instead of Piwik_Tracker_Config::getInstance()

Read individual setting or entire section:

$value = Piwik_Config::getInstance()->section['option'];

$section = Piwik_Config::getInstance()->section;

Change individual setting or entire section:

Piwik_Config_Writer::getInstance()->section = $newSection;

Piwik_Config_Writer::getInstance()->section['option'] = $value;

Split/move core/Config.ini.php into:

  • Config/Abstract.php - base class
  • Config.php - config reader; can set but does not save
  • Config/Writer.php - config reader/writer; will save changes

Remove core/Config/Ini.php and core/Tracker/Config.php.

Impact:

  • not backwards compatible because Zend_Config isn't used to represent array options
  • 3rd party plugins affected: TBD

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.)

Change History (73)

comment:1 Changed 4 years ago by vipsoft (robocoder)

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.

comment:2 Changed 4 years ago by matt (mattab)

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.

comment:3 Changed 3 years ago by vipsoft (robocoder)

  • Milestone changed from Features requests 1.x or 2.x to 1.2 - Piwik 1.2

comment:4 Changed 3 years ago by vipsoft (robocoder)

  • Description modified (diff)

comment:5 Changed 3 years ago by vipsoft (robocoder)

  • Owner set to vipsoft

comment:6 Changed 3 years ago by vipsoft (robocoder)

  • Milestone changed from 1.6 Piwik 1.6 to 1.x - Piwik 1.x

comment:8 Changed 2 years ago by vipsoft (robocoder)

  • Description modified (diff)

comment:9 Changed 2 years ago by vipsoft (robocoder)

  • Description modified (diff)

comment:10 Changed 2 years ago by vipsoft (robocoder)

  • Description modified (diff)

Quick grep shows the following non-core plugins are affected:

  • CASLogin
  • ClickHeat
  • DoNotTrack
  • EntryPage
  • Gall
  • GeoIPMap
  • HttpAuthLogin
  • KSVisitorImport
  • LdapLogin
  • Maps
  • MobileAnalytics
  • OpenIDLogin
  • OxidPlugin
  • Signup

comment:11 Changed 2 years ago by vipsoft (robocoder)

(In [5537]) refs #1713 - create branch for review

comment:12 Changed 2 years ago by matt (mattab)

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!

comment:13 Changed 2 years ago by vipsoft (robocoder)

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:

  • blog and/or post to piwik-hackers before this makes it into a release
  • an update script would disable known affected 3rd party plugins

comment:14 Changed 2 years ago by vipsoft (robocoder)

(In [5586]) refs #1713 - to matt for review/feedback

comment:15 Changed 2 years ago by matt (mattab)

Wow, nice work Anthon! long due change indeed :)
Review:

  • isMacOs && Ip.test.php changes are probably unrelated to this change
  • Did you check that password in config files still work when it contains <>&"' characters (I saw that html decode call was removed in plugins/DBStats/API.php
  • To go forward with this proposal, we must first ensure that all plugins are updated and packaged for the new version.
    • OR, what do you think about making the old format Zend_Registry::get('config')->General->anonymous_user_enable_use_segments_API still working for "READ" operations? Ie. we could add a fake config object that would reroute to the Piwik_Config::getInstance() equivalent? I suppose that by doing so, most plugins would not need repackaging since most plugins would only READ values from the file. The goal would be to make this commit less painful and less risky and not require all existing plugin users to upgrade their third party plugins after the piwik core update. What do you think?

comment:16 Changed 2 years ago by vipsoft (robocoder)

  • Yes, there are some unrelated changes there. It just happens that my dev environment was MacOS X. I'll merge the unrelated changes into trunk this weekend.
  • DBStats/API.php was double decoding. It predates [1165].

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.

comment:17 Changed 2 years ago by vipsoft (robocoder)

in [5608], added backward compatibility layer

comment:18 Changed 2 years ago by matt (mattab)

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!!

comment:19 Changed 2 years ago by vipsoft (robocoder)

  • Milestone changed from 1.x - Piwik 1.x to 1.8 Piwik 1.8

Should work with the aforementioned plugins. I'll be busy testing this weekend. Otherwise looking good for 1.8

comment:20 Changed 2 years ago by vipsoft (robocoder)

(In [5615]) refs #1713 - naming inconsistency with Config.php

comment:21 Changed 2 years ago by matt (mattab)

See also the double quotes bug in #2968

comment:22 Changed 2 years ago by vipsoft (robocoder)

(In [5951]) refs #1713 - merge dev branch to trunk (config class refactoring)

comment:23 Changed 2 years ago by vipsoft (robocoder)

(In [5952]) refs #1713 - fix svn property on core/Config/Compat.php

comment:24 Changed 2 years ago by vipsoft (robocoder)

  • Milestone changed from 1.8 Piwik 1.8 to 1.7.x - Piwik 1.7.2

comment:25 Changed 2 years ago by vipsoft (robocoder)

(In [5954]) refs #1713 - partially resolve regression from the merge

comment:26 Changed 2 years ago by vipsoft (robocoder)

(In [5955]) refs #1713, refs #1331 - workaround for auto-update

comment:27 Changed 2 years ago by vipsoft (robocoder)

(In [5957]) refs #1713 - rollback this file to r5454; workaround for #1331

comment:28 Changed 2 years ago by matt (mattab)

(In [5959]) Refs #1713 Fixing NOTICE when element not in config file

comment:29 Changed 2 years ago by matt (mattab)

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

comment:30 Changed 2 years ago by matt (mattab)

(In [5960]) Refs #1713 fixes changing SU email

comment:31 Changed 2 years ago by matt (mattab)

(In [5961]) Refs #1713 Admin settings were not saved

comment:32 Changed 2 years ago by vipsoft (robocoder)

I don't yet know why the integration tests pass on dev6 but fail locally.

comment:33 Changed 2 years ago by matt (mattab)

OK, I can help investigate in ~ 1 hour if you're still stuck!

comment:34 Changed 2 years ago by matt (mattab)

  • On rewriting the config file, the sections are not put back in the order they were in. It is important that sections are written in the order found in global.ini.php (ie. superuser, then database, etc.)

comment:35 Changed 2 years ago by matt (mattab)

  • Running integration tests is overwriting the local config.ini.php !! not so good, I lost my local mods

EDIT: Strange I just lost my [Tracker] section. Thanks to Eclipse local history I could restore :)

comment:36 Changed 2 years ago by matt (mattab)

  • Install is broken too..: No entry is registered for key 'config'

comment:37 Changed 2 years ago by vipsoft (robocoder)

(In [5967]) refs #1713 - fix installation; will have to workaround #1331 some other way

comment:38 Changed 2 years ago by vipsoft (robocoder)

(In [5968]) refs #1713 - splitting reader/writer just isn't possible with the way integration tests run

comment:39 Changed 2 years ago by vipsoft (robocoder)

(In [5971]) refs #1713 - fix installation and workaround #1331 (deja vu?)

comment:40 Changed 2 years ago by vipsoft (robocoder)

To do:

  • broken one click update; I'll narrow this down today
  • re-write sections in original order, appending new sections at the end
  • investigate disappearing section (was it the same as global.in?)

comment:41 Changed 2 years ago by vipsoft (robocoder)

(In [5980]) refs #1713, refs #1331 - this should fix the one click update

comment:43 Changed 2 years ago by matt (mattab)

(In [6000]) Refs #1713

  • keep sections that don't exist inglobal.ini.php (prevent NOTICE).
  • save sections in order that they appear in global.ini.php

comment:44 Changed 2 years ago by matt (mattab)

(In [6002]) Refs #1713

  • Ignoring local config.ini.php when running tests
  • Disable IP Anonymisation if it's enabled on dev box (was causing some integration tests fail)

comment:45 Changed 2 years ago by vipsoft (robocoder)

(In [6011]) refs #1713 - refactor for testability

comment:46 Changed 2 years ago by vipsoft (robocoder)

(In [6012]) refs #1713 - add unit test for dumpConfig()

comment:47 Changed 2 years ago by vipsoft (robocoder)

(In [6021]) refs #1713 - update the dirty bit and sorting tests

comment:48 Changed 2 years ago by vipsoft (robocoder)

Four of the tests are failing.

  • local copy (different), cached get - this is suboptimal
  • local copy, cached set local, new section - this is a bug
  • local copy, cached set global, new section - this is a bug
  • sort, common sections before new section - this is a bug

comment:49 Changed 2 years ago by vipsoft (robocoder)

(In [6022]) refs #1713 - aha, bad tests

comment:50 Changed 2 years ago by vipsoft (robocoder)

(In [6023]) refs #1713 - fix problem with new sections not being added; comment out two tests where writer is sub-optimal (i.e., it writes out a new config file even if there is no change)

comment:51 Changed 2 years ago by matt (mattab)

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"

comment:52 Changed 2 years ago by vipsoft (robocoder)

(In [6028]) refs #1713 - remove Config destruct()

comment:53 Changed 2 years ago by vipsoft (robocoder)

(In [6029]) refs #1713 - set() should set dirty bit

comment:54 Changed 2 years ago by vipsoft (robocoder)

(In [6034]) refs #1713 - not so lightweight anymore, but still an improvement by unifying Zend_Config and Piwik_Tracker_Config

comment:55 Changed 2 years ago by matt (mattab)

Feedback

  • Great work. Maintaining BC and simplifying code and interfaces, big win for the software maintainability and plugin developers.
  • Do you know why the webtest was not failing when trunk was broken? Is it because the webtest was disabled maybe?
  • It looks good for me, is the ticket now fixed or are there pending tasks?

Thanks for your nice follow up too, that was a tricky large core change :)

comment:56 Changed 2 years ago by matt (mattab)

  • Priority changed from normal to critical

I noticed a bug:

  • when the config.ini.php contains a value the same as global.ini.php, it gets deleted at the next config file write.
  • this is causing some frustration, because it means opening the file global.ini.php every time again to enable some custom features
  • the solution would be that once a value has been modified in config.ini.php, it is always left there, even when the value is set to the same as in global.ini.php

comment:57 Changed 2 years ago by vipsoft (robocoder)

It's intentional and is a result of array_unmerge.

comment:58 Changed 2 years ago by matt (mattab)

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?

comment:59 Changed 2 years ago by vipsoft (robocoder)

This would reintroduce #1945 where changing a local setting programmatically would copy the rest of the section.

comment:60 Changed 2 years ago by matt (mattab)

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?

comment:61 Changed 2 years ago by matt (mattab)

(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

comment:62 Changed 2 years ago by vipsoft (robocoder)

(In [6109]) refs #1713 - workaround php 5.1.x - php 5.2.0 reference bugs when config is returned by reference

comment:63 Changed 2 years ago by vipsoft (robocoder)

(In [6111]) refs #1713 - tracker config did not previously throw exceptions (BC); revert [6107] and part of [6103]

comment:64 Changed 2 years ago by matt (mattab)

Thanks for the better fix!!

comment:65 follow-up: Changed 2 years ago by matt (mattab)

Open critical issues related to the new config:

  • login=root and salt=XX are not in the config file on rewrite. During the update to 1.7.2-rc the login=root and salt=xx should be copied to the config./ini.php (or it prevents Super User login!! CRITICAL)
  • for usability it is important to leave the modified config settings in config.ini.php so as to avoid users to go back and forth between config and global files

@vipsoft Do you think these could be tackled in next days as to be able to release 1.7.2 ? Thanks!!

comment:66 Changed 2 years ago by matt (mattab)

  • Maybe it would be possible to remove Zend/Config files from libs/Zend ?

comment:67 in reply to: ↑ 65 ; follow-up: Changed 2 years ago by vipsoft (robocoder)

Replying to matt:

Open critical issues related to the new config:

  • login=root and salt=XX are not in the config file on rewrite. During the update to 1.7.2-rc the login=root and salt=xx should be copied to the config./ini.php (or it prevents Super User login!! CRITICAL)
  • for usability it is important to leave the modified config settings in config.ini.php so as to avoid users to go back and forth between config and global files

@vipsoft Do you think these could be tackled in next days as to be able to release 1.7.2 ? Thanks!!

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:

  • Maybe it would be possible to remove Zend/Config files from libs/Zend ?

Off-hand, I'm going to say yes it's possible, but I haven't tried.

comment:68 in reply to: ↑ 67 Changed 2 years ago by matt (mattab)

A core/Updates/ script should suffice.

OK

I understand the potential usability issue in the second case, but I can't think of any easy fix at the moment.

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?

comment:69 Changed 2 years ago by vipsoft (robocoder)

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.

comment:70 Changed 2 years ago by matt (mattab)

  • Milestone changed from 1.7.x - Piwik 1.7.2 to 1.7.2 - Piwik 1.7.2

comment:71 Changed 2 years ago by matt (mattab)

  • Resolution set to fixed
  • Status changed from new to closed

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!

comment:72 Changed 2 years ago by matt (mattab)

(In [6311]) Perf optimization refs #1713

comment:73 Changed 23 months ago by matt (mattab)

  • Priority changed from critical to major
Note: See TracTickets for help on using tickets.