Following discussion in #2784687: Make diff building more modular I realised that the easiest way to provide more extensible control over the process of building diffs, is to allow the configured settings that control the process to be easily modified by the calling context.
DiffEntityComparison and DiffBuilderManager are both guided by stored configuration. It's possible to override that configuration value by value, but that's a somewhat ugly and brittle process. Much more elegant would be to have those settings provided by a new service - DiffBuildSettings - implementing a defined interface, which could be replaced by an alternative service implementing the same interface. This settings code is fairly simple to work with whereas DiffBuilderManager contains much more arcane plugin management logic.
Most ambitiously, loading the settings from a service allows for contrib to replicate the diff field settings ui in order to give a ui to control the building of alternative styles of diffs. As long as the configuration from such a UI stored its values using the same config structure diff.plugins.yml uses, having a DiffBuildSettings service makes it trivial to build a diff using this alternative config.
| Comment | File | Size | Author |
|---|---|---|---|
| #2 | diff-DiffBuildSettings-2786969-2.patch | 22.09 KB | jonathanshaw |
Comments
Comment #2
jonathanshawLet's see what testbot says ...
Patch is based on top of patch from #2766035: Define default plugins #7/#10
Comment #3
jonathanshawComment #4
miro_dietikerSee #2784687-13: Make diff building more modular
I don't think a service adds that great value.
It can be only overridden once and contrib + custom can not collaborate.
Also, diff already has at least 3 related services.
I think we should flesh out cleaner separation internally and have nice methods.
There are many ways to implement cleaner interfaces and drop quite some code.
Comment #5
miro_dietikerDemoting for now.
Postponing on some cleanup. Let's revisit after our clean-ups.
Or even won't fix with the hook?
Comment #6
jonathanshawI was imagining that custom or contrib would dynamically (not globally) override with their version of the service.
I'm not sure how else you can easily allow contrib or custom to implement a completely different choice of builders, and especially fields to display, but only in their custom context not on the main route.
Comment #7
miro_dietikerThat's not possible. It's one service and the definition cached and only once instanciated.
You can not swap it out dynamically, except you create a custom service and wire it back to the original code conditionally.
But again, only one custom thing can do that.
Comment #8
miro_dietikerI'm pretty happy with the current setup and the flexibility of the plugins and would not add this for the release.
If real demand for a specific case comes up we can always add it.
Objections?
Comment #9
miro_dietiker