Vadim Gritsenko skrev:
Daniel Fagerstrom wrote:
If we want to factor out the pipeline functionality to an own block
that doesn't depend on the rest of Cocoon it will certainly be a lot
of work. But I think it would be worthwhile anyway as it would
increase usability and make the architecture easier to follow and
maintain.
I'm not that sure that pipelines should be extracted first. One would
think that it is a sitemap engine that is logically sitting on top of
pipelines.
+-----------------------------+
| Env Impl + Sitemap Servlet |
+-----------------------------+
| Sitemap Engine |
+-----------------------------+--------------------+
| Environment API + Pipelines | Sitemap Components |
+-----------------------------+--------------------+
| Container + Core Components |
+--------------------------------------------------+
Agree about your view, not certain that there is much use in splitting
up the sitemap stuff in sitemap engine and env impl + sitemap servlet
though.
We need names for the blocks, what about cocoon-sitemap,
cocoon-pipeline, cocoon-core (or cocoon-container) and
cocoon-sitemap-components?
We should IMO strive towards a layered architecture where we have a
sitemap (treeprocessor) block that depends on a pipeline block. Where
the pipeline block contains the pipeline, pipeline components and the
environment as well as numerous classes supporting them.
Sounds good. Only point is that we should start with outermost layer
first - extracting CocoonServlet first, following by sitemap engine,
and only then pipeline block.
Yes. Although I think we could split out the two upper layer in your
picture at once and split them in a servlet and engine part later if needed.
Next we need to plan the work so that it doesn't disturb the rest of the
work to much, and so that it can be performed incrementally. I think
that the Pipeline part is the largest part of Cocoon core so it would
probably be simplest to move away the sitemap, container and sitemap
component part from the core and keep the sitemap part.
The sitemap components should be the simplest part to move out.
Something that can be complicated and tedious to keep track on during
the work is all the dependencies in the poms that depend on cocoon-core.
One way would be to start with moving all code within cocoon-core to
e.g. cocoon-pipeline and let cocoon-core depend on cocoon-pipeline,
cocoon-sitemap, cocoon-sitemap, cocoon-container and
cocoon-sitemap-components. Then we can narrow the dependencies when we
have finished the work.
If some part of the work need larger or more complicated refactorings,
we probably need to do such work in a branch.
At last we of course need to be a number of people that are prepared to
work on it. I volunteer to do a part of the job.
Taking a look at the pipeline code there are some dependencies on
that would need to be removed. The ProcessingPipeline interface
depends on the class o.a.c.sitemap.SitemapErrorHandler that in turn
depend on treeprocessor stuff. It would be better to create an
interface e.g. PipelineErrorHandler that the ProcessingPipeline
depends on.
+1
There is also a Processor.InternalPipeline descriptor that is used
for the error handling within the pipeline implementations. It would
IMO be better to move this internal class from the Processor
interface to the components.pipeline package.
That's new for 2.2, I don't see it in 2.1. No objections for the move
here also.
Both these changes affects interfaces, but I would be surprised if
anyone have implemented Processor or ProcessingPipeline outside our
code base, so it shouldn't matter that much.
Since InternalPipelineDescription is 2.2 only, it was not released yet.
SitemapErrorHandler can implement PipelineErrorHandler without
affecting anybody.
The AbstractProcessingPipeline gets its SourceResolver through
EnvironmentHelper.getCurrentProcessor().getSourceResolver(), we need
to use dependency injection for it instead to make the implementation
easier to reuse outside the treeprocessor.
// TODO (CZ) Get the processor set via IoC
this.processor = EnvironmentHelper.getCurrentProcessor();
AbstractProcessingPipeline does not even use processor, only resolver.
(Did not check derived classes though).
OK, then we can start with this part of the work right away.
/Daniel
