The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.We want to give the calling context (e.g. the controller or whatever) maximum control of the process of constructing the diff. This allows people to build diffs in different contexts with non-standard displays.
The logic that decides which fields to show, what diff builder to use, and then builds the diff, (all in entityParser and diffBuilderManager) happens as part of a single recursive process, walking through the tree of fields and fields on referenced entities. We don't first do a setup, deciding what fields to compare and what builders to use, and then secondly build the comparisons. Therefore the calling context can't inspect the setup before the build, and override the choice of plugin or the configured settings on a plugin or anything else.
We could split it up, but that would require walking the fields tree twice. First we would build up a setup array storing what we planned to compare, loading the configured builders and settings, and return that array to the controller (which could modify it how it wanted). Second the controller would call a different method that went through the setup array and built the comparisons.
We could also offer a hook or event that fires once a field with its builder, settings and data have been loaded, passes these subscribers and allows them to modify any of them.
Todo
- Decide preferred architectural direction
- Do it
Comments
Comment #2
jonathanshawLooking more at the code, I see we already walk the fields tree twice: once in EntityParser parseEntity and once in EntityComparison compareRevision.
The call to plugin->build() happens in EntityParser, but at the end, and parseEntity does not examine the contents of the result. So in principle it seems like it might not be too difficult to move this call from parseEntity to compareRevisions.
In which case parseEntity would assemble the fields, plugins and settings, and compareRevisions would do the string conversion and diffing. So a controller could call these in sequence and modify the results of parseEntity before passing it on to compareRevisions.
Comment #3
miro_dietikerPromoting this to critical since it sounds it affects our architecture.
I don't want to maintain a 2.x that breaks our interfaces just for some extensibility.
Comment #4
johnchqueI am confused, is it really needed to inspect the built array after building it? The main idea about this is to check every field to see if it should show a diff and select the plugin for it in DiffEntityParser::parseEntity . Shouldn't be easier to just add a hook call just before
$build = $plugin->build($field_items);in parseEntity? so we can check the settings for the field before building the whole array? We could also add a hook call in compareRevisions so we can decide to add any other filter or more info in there.And also @jonathanjfshaw please note that the loop over the fields in EntityParser::parseEntity and DiffEntityComparison::compareRevisions are totally different, in parseEntity we build the array with the values (also recursively) and in compareRevisions we loop over the built array. I would still vote for adding a hook call just before
$build = $plugin->build($field_items);. :)Comment #5
miro_dietikerHappy to demote it as proposed in roadmap to post release once we are consent this requirement won't break our architecture.
Comment #6
jonathanshawOne advantage to working with the full array rather than a hook on a particular field is that the whole array gives a bigger context, and it's conceivable a developer might want to show a field's diff differently depending on what is going on in other fields. However, this is rather an edge case - maybe not worth the time.
Would having 2 hooks be a good approach?
1) for each field, just before
$build = $plugin->build($field_items);in parseEntity()2) and for the whole array just after
in compareRevisions()
My biggest concern with hooks is that in my use case I don't want to affect how the diff is displayed in the normal diff route that uses genericRevisionController. I'll have my own controller (formatter actually in my case) and only want to modify the particular diffs whose building I have initiated. I'm not sure if hooks/events fit this.
Comment #7
jonathanshawStudying this more, I'm realising that the best solution for my use case seems to be to create a new service that extends DiffBuilderManager and replaces it. Instead of allowing diffBuilderManager to load the default config or built results, and then trying to modify what is returns, it seems better to use a more heavy-duty approach and take full control of the build process.
Given that the recent creation of DiffBuilderManager as a service allows for this possibility, I'm not sure you need to allow for much more extensibility in this area. Hooks etc. would suit developers would be a convenience for some developers who only wanted to make a minor tweak, but it would not open up fundamentally more control than you've already given.
Comment #8
miro_dietikerGood to hear. That's what i thought with my reference to standard patterns for extensibility / alterability.
Demoting this then. Still keeping pending to add a hook if someone is in need of it.
However, for us it's not important now and we don't expect any major change on the current service beside the cleanup.
But for now, without any hook or similar, we can mostly consider these things internal changes.
Comment #9
jonathanshawI created #2786969: Create a DiffBuildSettings service with proposal and patch to move the configured settings that guide the build process into a service, so they can easily be swapped out. This is significantly easier than swapping out the whole DiffBuilderManager with its relatively complex plugin management logic.
Comment #10
jonathanshawI created #2786969: Create a DiffBuildSettings service with proposal and patch to move the configured settings that guide the build process into a service, so they can easily be swapped out. This is significantly easier than swapping out the whole DiffBuilderManager with its relatively complex plugin management logic.
Comment #11
miro_dietikerThis is related to the diff appearance plugin i started to discuss in #2784381-10: Define how to display field labels on comparison
Comment #12
jonathanshawI think you may have confused issues?
#2784381-10: Define how to display field labels on comparison would seem to relate to #2785147: [META] Introduce a DiffLayout plugin system more than this issue.
This issue is mostly about controlling which fields to include and which field builders to use; both those issues are more about how to render and format the built comparison.
Comment #13
miro_dietikerI have updated this issue: #2785147: [META] Introduce a DiffLayout plugin system
It now proposes to introduce plugins to allow different display of diff.
Still, the field based diff plugins will use the DiffEntityComparision and if there is a need to customise this or alter the definitions, we could trigger a hook in compareRevisions such as:
That said, i'm not sure what exactly makes sense to alter. Some altering could make sense in parseEntity or in compareEntityRevisions.
I think we can further clean up and need to know about the real use cases to cover here.
A service is a one way path. It can only be replaced once. The hook would allow specific context aware alterations based on conditions and collaboration of multiple modules. I don't think it adds much real value.
What i discovered in severity, i really don't like again is the special treatment of the markdown case at so many different locations...
Will create some new issue to discuss this.
Demoting since as a hook, it doesn't create incompatibility.
And for now, let's get the internals cleaned up ASAP and make those spaghetti methods nice.
Comment #14
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 #15
jonathanshawSorry for slow reply, no objections. I should be paying more attention to diff again in next few weeks, I'll try the new architecture and see if I can do what i want.