Page MenuHomePhabricator

[Maintenance] Do not bind against LabelsProvider in ChangeOps
Closed, ResolvedPublic5 Estimated Story Points



In this situation it sounds like it would be better to bind against FingerprintProvider, as a FingerprintValidator is about to be used. So Fingerprint makes sense in the context, as the labels and descriptions need to be validated together.
However, FingerprintValidator is badly named, as it only actually validates label and description combinations, and doesn't validate a Fingerprint object, rather parts of it.

I would propose that we rename FingerprintValidator to LabelsDescriptionsValidator and then ChangeOps must bind against LabelsProvider and DescriptionsProvider which seems fine.

Acceptance criteria🏕️🌟:

  • We have looked at and evaluated the TODO
  • We have either removed the TODO (situation is okay right now), or we have implemented some change (if it doesn't require rewriting the world)

Event Timeline

alaa_wmde renamed this task from [Maintenance] Move validations outside apply to [Maintenance] Do not bind against LabelsProvider in ChangeOps.May 10 2019, 2:11 PM
alaa_wmde updated the task description. (Show Details)
Addshore lowered the priority of this task from Low to Lowest.Jan 15 2020, 11:33 AM
Addshore updated the task description. (Show Details)
Addshore moved this task from Incoming to Needs Work on the Wikidata-Campsite board.
Addshore subscribed.

Needs a better description of what to do instead..

Prio session notes: This ended up at the bottom of the prio list as one of the things we know we want to tackle at some point, but realistically it'll sit at the bottom of the list forever.

Addshore set the point value for this task to 5.Jun 23 2021, 10:45 AM

So FingerprintValidator is already gone, the class is now LabelDescriptionNotEqualValidator:

		// TODO: Don't bind against LabelsProvider here, rather use general builders for validators
		if ( $entity instanceof LabelsProvider ) {
			$validator = $this->termValidatorFactory->getLabelDescriptionNotEqualValidator();

If we want to use a validator that’s explicitly targeting labels and descriptions, and we’re in a description change op, then binding against LabelsProvider actually seems okay to me, to be honest. (Likewise for binding against DescriptionsProvider in ChangeOpLabel.)

I personally think the code is okay now, and we could just remove the two TODO comments.

It sounds good to me. Please remove that TODO.

Change 702610 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Remove TODOs from two ChangeOp classes

Change 702610 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Remove TODOs from two ChangeOp classes