Design considerations

[Adirelle]

Since I started using and hacking PHPCI, several things in the source are tingling my OOP sense. I think most of these things come from legacy code and contributions of people with various backgrounds and experience in OOP. Though they work, I feel these parts should need some love to pay the technical debt. Here are a few points I noticed (and you have probably also noticed):

• The PluginFactory implements a nice dependency inject mechanism using a Pimple container. Yet most of the plugins use the Builder directly, because it is basically big facade to most of the components. I think each components (CommandExecutor, Interpolator, Logger) should be described and used an interface and then made available through the PluginFactory. This way the dependencies of each plugin would be made apparent and there would be less pressure on the Builder itself. (See Interface Segregation Principle, the I of SOLID)
• The Builder has more than one responsibility: build all the components needed to build, being a facade to those components, run the build process itself. These responsibilities should be broken down in smaller classes: the first responsibility could be assumed by a BuilderFactory, the second point should be fixed by a proprer usage of interfaces and the PluginFactory (see previous point), leaving only the third responsibility to the Builder. This would ease the testing of the Builder and make it a more manageable class. (See the Single Responsibility Pinciple, the S of SOLID).
• The BuilderLogger doesn't add a great value over the Monolog Library, e.g. what this class and the LoggedBuildContextTidier do could be easily implemented using processors, handlers and/or loggers. However, he still introduces a second logger-like interface which isn't compatible the Psr/Log/LoggerInterface. So it cannot implements LoggerInterface and cannot be used wherever a LoggerInterace is accepted. This adds unneed complexity. I think this class should be dropped completely and its feature reimplemented using Monolog.
• The BaseCommandExecutor is a service, so it should be stateless. However, it depends on global states: the process environment (which should be fixed by #885) and the current working directory (which I'll try to address in #884) and also states stored in the Builder (!) through protected and public (!!) attributes. This can lead to command being affected by the settings of previous plugins. In this case, either a fresh CommandExecutor should be provided to each plugin so they can reconfigure it if need be without impacting the following plugins, either the execution parameters (verbosity, working directory, environment) should be passed its the executeCommand method.
• There is something with the buildPath attribute and the removeWorkingDirectory method. They are needed in several places: in the Builder itself, but also in the RunCommand to cleanup stale builds. I would attach them to the Build itself, and maybe persist the buildPath so one can track where the build should be even if the build root is reconfigured.
• I know you are aware of this: the Build subclasses should be dropped in favor of a strategy pattern to handle the project types. This could help to break some circular references in the object graph, e.g. the Builder is associated to a Build but the Build also uses the Builder.

I can contribute to fix these points but I would like to know what you, PHPCI collaborators, think about this.

Thanks for reading.

[mavimo]

@Adirelle First of all.. Thanks for the AWESOME analysis.

I agree with all points and I know this should be a big refactoring on all parts of PHPCI application, but i think it's required in order to make the application evolve and remove some technical debt accumulated on time.

I would also suggest:

• Move DIC outside the PluginFactory to be used also for other services described in the application; for example actually we need manually instantiate services (eg for command and I see a lot of this approach on MRs).
• Split code into component should force also to cover with unit test (a lot of component actually are untested/untestable)

If also other are agree I also like to help on this refactoring on my spare time, but IMHO we need to define a list of task and prioritize (to manage dependencies, ..) before start on it.

[dancryer]

@Adirelle & @mavimo - Seconded, this is a great analysis.

I agree we need to define a plan first, and then I'd want to see the following:

1. These tasks need to be handled as best as possible in isolation. I don't want to see one big PR with a refactoring of the entire application.
2. I'd like to see this work spread out over a few releases to ensure stability.

[mavimo]

@dancryer I agree.

I suggest to start with somethink simple like:

1. There is something with the buildPath attribute and the removeWorkingDirectory method.
2. Replace BuilderLogger with Monolog (and Psr\Log\LoggerInterface)

The 1 is WIP in #881 The 2 is "easy" and don't impact a lot on other parts

[Adirelle]

@dancryer, @mavimo I have several PR in progress, and I think I'll wait until they either get merged or rejected before working forwards on other points. Some of them impact the Builder and the plugins and I am almost sure they will cause conflicts.

About the Logger, I think we should define some guidelines beforehands:

• which levels should use the plugins to report information (e.g. verbose output as INFO, normal output as NOTICE, ...),
• how the loggers actually filter them, for examples: debug: all, verbose: level >= INFO, normal: level >= NOTICE, quiet: >= ERROR...

[Adirelle]

@mavimo: I would like to see a global DIC to be created. In my opinion, this could be done in a few steps:

• Create two containers: one outer, global container for the whole application and an inner, build-specific container. The inner container is a temporary, "short-lived" container which contains only build-specific services, falling back to the outer container when a service is not found. This inner container should be quickly done by extending Pimple, overriding the ArrayAcces methods.
• Create a DI helper that creates services using reflection, similar to what the PluginFactory does, but for any component. It could be used for both the outer and inner containers.
• Create a "container.php" that would contains the default container services and parameters for the application and call it from the bootstrap.
• Track down calls to static methods and constructors and replace them.

[Adirelle]

After hacking around PHPCI the last few days, I think we would have to do some cleanup prior to adding features/fixing certain bugs. The biggest issue is the dependency of several components, including all plugins, to the bloated Builder. I think trimming down the Builder should be a primary focus.

Moreover, it appears that the coding practices differ highly from a plugin to another one, depending on their author, which doesn't help refactoring them. It seems a lot of copy-pasta have been used, instead of using a common base class.

Another outstanding point is that there is no standard way to deal with plugin options, so every plugin author uses its own way. We should think about this.

[Adirelle]

So I've started to clean the plugins to eventually remove their dependency to Builder: https://github.com/Block8/PHPCI/compare/master...Adirelle:plugin-cleanup

I don't try to fix bugs nor completely remove the dependency to Builder, and I haven't introduced any breaking changes. However, I feel some things could be moved into the Plugin interface to lighten the constructor, like a setOptions method: this would allow to remove the $options from the constructor (and the associated Plugin Factory shenanigan). Something should be done about options: setting defaults and checking values is a common task. There already are libraries handling this in a neat way. For example, we could use Symfony's OptionResolver, though it would cause BC in some cases.

I'm also hesitating about the way to handle the plugins' build settings: I was thinking about adding a "BuildSettingsAware" interface. Another solution is to merge the plugin's build settings into every stage options. This wouldn't require another interface but it would be less clean.

At this point, I can start to clean Build's subclasses in the same way, but I would like some feedback on this points.