docs/adr/0004-make-apply-return-changeopresult.md
# 4) Make ChangeOp::apply() return ChangeOpResult {#adr_0004}
Date of submission: 2019-06-21
Last updated: 2019-07-04
Discussion Deadline: 2019-07-09
## Status
accepted
## Context
`wbeditentity` summary message for Wikibase Items and Properties provides
very little information on what have been changed on the entity, with the main
message in the summary being "Item Changed".
In [T220696], we want to update those summary
messages generated for Wikibase Items and Properties when using `wbeditentity`
to be more detailed and informative.
Wikibase Lexeme achieved generating detailed summary messages by:
1. making its implementations update the summary object passed to
the change op instance with granular information about the change.
2. implementing [SummaryAggregator] class and using it internally in some
[ChangeOp] implementations (some of those that are non-leaf nodes and
contain further change ops) in order to combine those summary messages
generated by leaf nodes (the effective change ops in the tree).
## Considered actions
### I. Reusing current pattern
We considered doing the same thing for Wikibase, but that seemed a little off
with regards to clean design.
The main two places where this off-feeling came from are:
1. a tree hierarchy is often implemented for structural purposes, and then
accessed through visitors for logic (implementing the [Visitor Pattern]).
2. currently [ChangeOp] is concerned with:
a. representing a change that can be applied to an entity
b. applying that change to an entity
c. generating summary messages as a result of applying it to an entity
### II. Visitor pattern design
We then considered:
1. using [Visitor Pattern] to implement our detailed summary generation
for Wikibase Items and Properties;
2. changing [ChangeOp] design in a way that:
1. allows a visitor class to operate over change ops hierarchy and attain
the information it needs from it to complete its job. These information
include whether the change op has been applied or discarded (no-op).
2. is not a breaking change ([WikibaseLexeme] should continue to operate with
no touch to it).
3. does not extract the logic of applying a change op to an entity outside
of it (remember logic can be implemented as a visitor) as it is out
of scope and it isn't clear whether it will have enough justification.
This solution comes with some cost of initial implementation and
refactoring of current [ChangeOp] design and relevant implementations within
Wikibase and [WikibaseLexeme].
The benefits of this solution is that it:
1. uses a more standard pattern with tree structures.
2. extracts the concern of summary messages generation out of
[ChangeOp], simplifying its design and implementations and scoping down
the domain and dependencies of [ChangeOp].
## Decision
We decided to go with the second option, the visitor pattern design.
In order to achieve that, we will make [ChangeOp::apply()] return
[ChangeOpResult] that is defined by the following IDL:
```php
// encapsulates the result of applying a change op to an entity document
interface ChangeOpResult {
// the id of the entity document that the change op was applied to
EntityId getEntityId();
// whether the entity document was actually changed in any way
// as a result of applying the change op to it
bool isEntityChanged();
}
```
### Next steps
1. Change [ChangeOp::apply()] to return [ChangeOpResult]
2. Provide needed implementations of [ChangeOpResult] that can capture the result
of applying the different [ChangeOp] implementations.
Example:
[ChangeOpLabel] will probably return something like [ChangeOpLabelResult] that
is defined as:
```php
interface ChangeOpLabelResult : ChangeOpResult {
string getOldLabel();
string getNewLabel();
string getLanguageCode();
}
```
3. Update implementations of [ChangeOp] in Wikibase and [WikibaseLexeme] to conform
with the changes to the interface.
## Consequences
There is one consequence of going with visitor pattern design.
Wikibase and [WikibaseLexeme] will have two separate ways to achieve similar requirements.
This should be treated as short-term inconsistency of design and should be
mitigated by a follow-up refactoring task that changes
Lexeme implementation into reusing the visitor pattern similar to Wikibase.
## Notes
Changing [ChangeOp] interface in Wikibase will result in failing CI jobs
due to the fact that Wikibase CI will also run [WikibaseLexeme] tests
as part of its pipeline. This creates a circular situation for implementations
of [ChangeOp] interface.
The way to go around it is to go in this order:
1. update the changed methods in [ChangeOp] interface in [WikibaseLexeme]
implementations of that interface.
2. update the methods in [ChangeOp] interface in Wikibase, and make that change
depend on the change from 1 above. Implementations of [ChangeOp] in Wikibase
itself can be updated with the new methods in this step or a separate follow
up step.
Although this situation is not good enough as a reason, but it slightly hints
that [ChangeOp] concepts might better be moved out into a separate library that
can then be versioned and depended on as per proper mechanisms from Wikibase
and [WikibaseLexeme].
----
[T220696]: https://phabricator.wikimedia.org/T220696
[Visitor Pattern]: https://en.wikipedia.org/wiki/Visitor_pattern
[WikibaseLexeme]: https://www.mediawiki.org/wiki/Extension:WikibaseLexeme
[ChangeOp]: @ref Wikibase::Repo::ChangeOp::ChangeOp
[ChangeOpLabel]: @ref Wikibase::Repo::ChangeOp::ChangeOpLabel
[ChangeOpLabelResult]: @ref Wikibase::Repo::ChangeOp::ChangeOpLabelResult
[ChangeOp::apply()]: @ref Wikibase::Repo::ChangeOp::ChangeOp::apply()
[ChangeOpResult]: @ref Wikibase::Repo::ChangeOp::ChangeOpResult
[SummaryAggregator]: @ref Wikibase::Lexeme::MediaWiki::Api::Summary::SummaryAggregator