@diosmosis opened this Pull Request on April 8th 2015 Member

Call me Ishmael.

This pull requests provides logic to encapsulate the entire environment setup logic in a single class. As a proof of value, the Plugin\Manager has been successfully added to the DI container.

New Concepts

This PR introduces one new concept:

Kernel Objects

Currently in Piwik, most services are singletons, despite the presence of a DI container. Ideally, all of these singletons should be moved to the DI container, however, this is non-trivial since in order to create the container, some singletons are used (specifically Config & Plugin\Manager).

In order to decouple container creation from these singletons, I introduce the concept of Piwik's kernel, which is the core part of Piwik that cannot be overridden through DI (and thus by plugins). Currently, only two types of objects exist here: the PluginList & the GlobalSettingsProvider.

The PluginList currently returns the list of activated plugins, so the container can load plugin-specific config. The GlobalSettingsProvider provides config information used to create the container (and for the default implementation of PluginList).

These objects are created before the DI container is created, but they are placed in the container so other objects can access them w/o having to treat them as singletons.

Though these objects cannot be overridden through DI, it is my intent to allow this eventually, through Application encapsulation (see the next steps list).

List of changes

  1. Introduction of Environment class

    This class encapsulates the creation of kernel objects & then the DI container. It also encapsulates environment validation, which before, was in several places, including Config.php/FrontController.php/CronArchive.php

  2. Introduction of PluginList kernel object interface + IniPluginList implementation

    This class is used to create the container, and so cannot be created through DI.

    Rationale: I created a new interface for this functionality, as it could be useful later. When we modify plugins to be available through composer, we could create a new implementation to manage the activated plugin list using composer.json or another file.

  3. Introduction of GlobalSettingsProvider kernel object interface + IniSettingsProvider implementation

    This class is used to create the container, and so cannot be created through DI.

    Rationale: I created a new interface, because it might be useful to provide new implementations later. Eg, during the meetup, pro devs described needing to store most config values in the DB so propagating changes through a load balanced environment would be easier. Creating another implementation of this class (when it's possible to) could provide a possible solution.

  4. Moving all environment validation logic to EnvironmentValidator class (another kernel object).

    Currently in Piwik, Environment validation is done manually in Config, FrontController & CronArchive. This PR moves all that validation to a new class used by Environment. The EnvironmentValidator class will check if the global.ini.php/config.ini.php file is available, and based on the current execution environment (ie, tracker/cli/web), will display an error or start the installation process.

    There is also a system test that verifies this behaviour.

  5. Removed ability to change config files used by a Config instance

    In order to fix tests after this change and to allow INI files to be read before Config is created, I had to ensure that files used by Config couldn't change at any random point during test execution. Instead, one must create a new singleton instance. This is closer to overriding via DI, so should help in putting Config in DI.

    This change includes:

    • Deprecating init(), setTestEnvironment(), clear() and removing uses in core Piwik code.
    • Making reload() protected.
  6. Moved Config::encodeValues/Config::decodeValues to IniFileChain.

    Since the encoding/decoding logic is used to avoid some INI parsing bugs, it should be part of IniFileChain, not just the Piwik-specific Config class.

  7. Lots and lots of hacks to test code

    These changes are changes to TestingEnvironment.php, the introduction of TestConfig.php and changes to test case classes. These are all there just to make tests pass, and should be considered temporary. Eventually, I will clean up test code significantly.

  8. Tweaks to CronArchive.php environment setup

    CronArchive.php has some unnecessary code for environment setup. Now, since archiving is initiated through archive.php & through core:archive, the environment will already be setup, so there's no need to set the host and create the Config instance. I verified it works w/ --piwik-domain & w/ Host: ... HTTP header.

  9. Change to core\IP.php

    There's a small change here. For some reason, the error silencing on the statement <a class='mention' href='https://github.com/Config'>@Config</a>::getInstance()->General['...']; didn't work unless it was split into two statements.

  10. Plugin\Manager added to DI

    Not much to say about this. ::getInstance() is still there, as more objects are added to DI, we can perhaps remove it.

  11. Removed cli-script-bootstrap.php

    No longer used, so removed.

    Next Steps

  12. Application Encapsulation.

    Involves encapsulating every entry point in a class that creates an Environment. For example, the tracker can be encapsulated in a TrackerApplication class that provides a public function track($params) method. The piwik.php file should then be just:

    require_once PIWIK_INCLUDE_PATH . '/core/Tracker/Application.php'; // handles autoload requiring + whatever else
    $tracker = new TrackerApplication();
    $tracker->track($_GET + $_POST, $_SERVER, ...);
  13. Moving Config class to DI & deprecating, merging with IniSettingsProvider

    If we merge the rest of Config logic w/ IniSettingsProvider, we will have one less singleton. Obviously, we can't get rid of Config because of it's pervasive use, but it would become just a view of IniSettingsProvider.

    Removing it completely will force cleaning up the test environment setup code (which is do-able, but I cannot think of a concrete solution w/o starting to work on it).

  14. Filling out PluginList description

    We can move other plugin list manipulation logic from Plugin\Manager to PluginList + IniPluginList to make it easier in the future to install plugins through Composer. We can potentially create an implementation that uses composer.json or another file.

    Post-merge TODO

    • [ ] Get builds using CloudFixture to pass. I have a branch that does some of the work, but I haven't updated in a while. Waiting for this to be merged.
    • [ ] Add system test to make sure cron archiving (via cli + web) works when another piwik domain is used. Requires --dry-run parameter to CronArchive so tests don't become too slow.
@mnapoli commented on April 10th 2015 Contributor

I have only read the description for now (I know about the first commits of the PR though, but haven't read the rest yet): :+1: this is awesome, I hope it will all work out well (huge complexity with BC with what plugins use, tests messing up the environments, different entry points to the app with different environments…). But this is definitely the way to go and it will make future refactorings easier.

10 Plugin\Manager added to DI

Great. Small detail but we always end up aliasing Piwik\Plugin\Manager to either Plugin\Manager or PluginManager because Manager alone is confusing and conflicts with other classes. This is not directly related to the PR but adding it to the container means that we will be able to inject it using the class name, which means the current problem will be even more visible. I think one of the next step would be to rename that class to something better, I have created a separate ticket to discuss this: #7656

@mnapoli commented on April 10th 2015 Contributor

OK I have reviewed all commits as much as I could and added a few comments. I get the overall goal and I'm definitely :+1: on what's happening here.

It's a really big diff, and it prevents reviewing each detail in every change. But we discussed that refactoring at length and did many attempts and I can say it's impossible to do this refactoring in small pull requests. So it won't get better than that, I'm really glad you managed to do it.

Regarding the build it seems there are a few system tests failing.

The only thing I'm not too much a fan of is the interfaces GlobalSettingsProvider and PluginList: they both have one implementation. I would say we turn them to an interface if/when it's needed because I don't see a big value in having them now. But that's just a detail.

@diosmosis commented on April 15th 2015 Member

When/if merged, should be merged at beginning of 2.14 rather than right before 2.13. Changing milestone.

@diosmosis commented on May 1st 2015 Member

@mnapoli I merged IniPluginList w/ PluginList and IniSettingsProvider w/ GlobalSettingsProvider, do you think you could do another review? Not many changes from last time, though.

@mnapoli commented on May 7th 2015 Contributor

@diosmosis I reviewed the latest commits where you merged the interfaces/classes, I'm definitely +1 with that change. All this isn't public API anyway so we can always change it whenever we like later.

I've checked out the branch, had a look around. The diff is still huge, but I'm +1 for merging: the longer we wait the higher the probability is we never merge this. We have to merge it when we start 2.14 to be able to continue the work on this topic.

Just one thing: I don't remember if there was a reason to have IniPluginList and GlobalSettingsProvider as 2 separate classes. It seems (from the changes in the PR) that those 2 classes are actually really similar and that we need to break the encapsulation of GlobalsSettingsProvider to access IniPluginList which isn't the best. Would it make sense to merge them? We could also do it in a separate pull request to avoid adding more and more… (as always, it's not API, we can change it later)

Except that, needs a rebase (master is green for now which is great), green tests and then I think it's OK to merge.

@diosmosis commented on May 7th 2015 Member

Re IniPluginList + GlobalSettingsProvider: This is mostly thinking ahead for composer. The list of plugins to load doesn't have to come from INI file forever. I don't know what you mean by 'break encapsulation', can you explain?

@mnapoli commented on May 7th 2015 Contributor

I don't know what you mean by 'break encapsulation', can you explain?

Code that uses GlobalSettingsProvider sometimes need to access the underlying IniPluginList object and does so by calling the getIniFileChain() getter: the globalsettingsprovider doesn't entirely encapsulate the inipluginlist object.

OK not a big deal, especially if it will be useful later. Let's merge asap.

@diosmosis commented on May 7th 2015 Member

Code that uses GlobalSettingsProvider sometimes need to access the underlying IniPluginList object and does so by calling the getIniFileChain() getter: the globalsettingsprovider doesn't entirely encapsulate the inipluginlist object.

It's not supposed to encapsulate it. IniPluginList uses the GlobalSettingsProvider as its source, so IniPluginList depends on GlobalSettingsProvider which depends on the IniFileChain. At least that's how it's supposed to be. I haven't looked at the code in a while.

We can discuss this later though. I'll merge after the smaller changes you noted above.

@mnapoli commented on May 7th 2015 Contributor

Ahh sorry I meant IniFileChain instead of IniPluginList! ;)

@diosmosis commented on May 7th 2015 Member

FYI, one other note: I can remove the singleton nature of IniFileChain after this step is finished.

@diosmosis commented on May 7th 2015 Member

Only UI test failures are some Installation tests, and new screenshots look more accurate than existing. Will copy them over after merging.

This Pull Request was closed on May 7th 2015
Powered by GitHub Issue Mirror