After merging, QueuedTracking will have to be modified, as it still uses the TestingEnvironment.addHooks event.

Looks like this PR depends on #8026, there are some odd access related failures. Waiting for that one.

Coming back again on the topic of "environment manipulators": the interface comment says:

Only used by the testing environment setup code, shouldn't be used anywhere else.

So if I understand correctly:

• we have an interface that is meant to be implemented only by one specific class
• the Environment has an array of "manipulators" (link) whereas it should contain only one entry

If that's correct then maybe Environment could have a single object instead of an array, and we could get rid of the interface?

Regarding TestingEnvironmentVariablesDefinitionSource, it's a nice and clean implementation I'm happy to see that extending PHP-DI works so easily ;) However would it make more sense (i.e. simpler) to just take all the testing env variables, dump them into an array (and we add the prefix here) and just pass the array to be added to the container by the environment?

That would avoid the custom definition source + this custom code for tests in the ContainerFactory

If that's correct then maybe Environment could have a single object instead of an array, and we could get rid of the interface?

We could just have one, I was hoping an array would look like less of a hack.

As for the interface, I think it's a good idea to keep it. Otherwise, Environment.php would reference a test file, namely the environment manipulator I added. I don't think application code should do that if possible.

However would it make more sense (i.e. simpler) to just take all the testing env variables, dump them into an array (and we add the prefix here) and just pass the array to be added to the container by the environment?

Where would this be done? Would we be able to override the vars in provideContainerConfig()?

Note: It would only avoid the lines that add the definition source.

Where would this be done? Would we be able to override the vars in provideContainerConfig()?

I'm not sure. But since the testing env manipulator does provide container entries to the environment (e.g. the entries defined in fixtures or test classes), it could also provide those entries through an array instead of a custom definition source? Maybe it's not possible, I was deep into the code yesterday but now I'm out of it and maybe it doesn't happen at the same time…

For the interface I'm OK to keep it I see your point with coupling the core code to test code, for the array of env manipulators I would say let's turn it into a single dependency it's that's ok with you? I'd rather keep this core code as simple as we can so that it's easy to understand

Maybe it's not possible, I was deep into the code yesterday but now I'm out of it and maybe it doesn't happen at the same time…

It's not easily possible it seems. Using a definition source, we can return null for variables that don't exist in the json file. Can't do that if we just add the variables in an array.

Ready for another review.

Looks good to merge, except for the the Travis build: 2 failures in SystemTests: https://travis-ci.org/piwik/piwik/jobs/65642678#L524 Is it the random failure or something else?

Ah those are queued tracking errors, I have to modify the plugin...

QueuedTracking PR: matomo-org/plugin-QueuedTracking#8

The build won't pass, there's one failure left. I tried to fix it, but can't seem to figure out what the problem is.