A few questions: should this affect Special:NewProperty?
Should it also prevent undo/restore/rollback, if the resulting content has labels and descriptions that are the same?

For Special:NewProperty yes it is part of the acceptance criteria so it should also fix that one.

For rollback, I think that it should not disallow it even if it will set the item/property into a kinda invalid. Otherwise, a vandal might introduce a change that we want to revert, and if the item/property had same label&description before that point, the editor's only option is to make a new edit instead of rolling back. That sounds less ideal for editors, I think. Sounds sensible to you?

Change 504283 had a related patch set uploaded (by Greta WMDE; owner: Greta Doçi):
[mediawiki/extensions/Wikibase@master] Prevent label = description to Special:NewProperty

https://gerrit.wikimedia.org/r/504283

Greta and I tried yesterday to implement this in the SetLabel and SetDescription APIs. Their parent class ModifyEntity has a convenient validateEntitySpecificParameters method, where you can validate the array $preparedParameters against the EntityDocument $entity and raise an error if the new label/description in the $preparedParameters matches the description/label in the $entity. However, we found that it didn’t work correctly in the case where you set both label and description to the same new value in the UI. That’s because in that case, the second API request (to set the description) specifies baserevid as the revision ID of the original loaded page, not the new revision ID returned by the first API request (to set the label); and ModifyEntity loads the entity at the base revision and runs validateEntitySpecificParameters against that, so it would check if the new description doesn’t match the old label, rather than the new one in the latest revision.

Also, this approach requires separate implementations in Special:SetLabel, Special:SetDescription, Special:SetLabelDescriptionAliases, wbeditentity, and possibly more places that we forgot.

An alternative place to implement this check would be deep in the bowels of the entity saving code (at the point where we actually have the parent revision), e. g. in MediawikiEditEntity::attemptSave(), which ends up calling the EditFilterMergedContent hook. But this might also affect undo/restore/rollback…

In T212869#5114173, @Lucas_Werkmeister_WMDE wrote:

But this might also affect undo/restore/rollback…

I don't think (as an editor) that's a problem. The wrong edit, and the one that should be reverted, is the one with identical label and description. Unless low-level data looses its integrity or Wikidata crashes (I don't think so) in the very unlikely event of reverting to an edit with identical label and description, I think this option would be equally great.

Why not do this as part of ChangeOpDescription and ChangeOpLabel? I know some places don't use this, but this seems to be the right place to me.

Change 504283 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Prevent label = description to Special:NewProperty

https://gerrit.wikimedia.org/r/504283

Unfortunately the ChangeOp approach seems to have the same problem: it uses the base revision, not the parent revision, to check if the label and the description are the same :(

Change 506172 had a related patch set uploaded (by Greta WMDE; owner: Greta Doçi):
[mediawiki/extensions/Wikibase@master] Add LabelDescriptionNotEqualValidator

https://gerrit.wikimedia.org/r/506172

Change 506181 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Add CompositeFingerprintValidator

https://gerrit.wikimedia.org/r/506181

Change 506182 had a related patch set uploaded (by Lucas Werkmeister (WMDE); owner: Lucas Werkmeister (WMDE)):
[mediawiki/extensions/Wikibase@master] Use LabelDescriptionNotEqualValidator in TermValidatorFactory

https://gerrit.wikimedia.org/r/506182

In T212869#5122970, @Lucas_Werkmeister_WMDE wrote:

Unfortunately the ChangeOp approach seems to have the same problem: it uses the base revision, not the parent revision, to check if the label and the description are the same :(

But you still went with it (indirectly, as the FingerprintValidator you're adding is used there)? Does that cause problems elsewhere? Is there a task for that?

Sorry to come late to this one. I can still see one problem here even if we find a way to tell the second request to look at the right revision id.

The problem comes from the fact that UI is making two separate requests for what appears to be an atomic action (save all changed terms) to the editor. While that haven't caused any issues before now, and probably was decided to be done that way so that changing labels, descriptions and aliases get their own edit revisions separate from each other (for easier rollbacks and more atomic history on level of term edits), bubbling up this kind of separation up to the UI is problematic in this case.

Why is that a problem now?
Let's assume that second request (description in this case) is "fixed" and will look at the parent revision id instead of baserevid and it will detect the duplication correctly and fail.
In that case, the label will be changed, but the description will not, which is not the right thing to do here from UX perspective. The editor is editing both, and press save expecting both to be saved or nothing to be saved if they did a mistake or didn't know they can't use same label and description. Their next step will be to go and rollback their edit to the label, that if they even noticed that it was saved after they get an error message (due to the failing second request)

How to solve UX issue?
If that part of editing does not work without javascript anyway, we can add some client-side validation. If not, then we have to do one request for saving (can we use wbeditentity endpoint instead for instance?).

In T212869#5135925, @hoo wrote:

In T212869#5122970, @Lucas_Werkmeister_WMDE wrote:

Unfortunately the ChangeOp approach seems to have the same problem: it uses the base revision, not the parent revision, to check if the label and the description are the same :(

But you still went with it (indirectly, as the FingerprintValidator you're adding is used there)? Does that cause problems elsewhere? Is there a task for that?

We might have tested something wrong – with the changes linked above, I can’t reproduce this problem anymore.

In T212869#5135995, @alaa_wmde wrote:

The problem comes from the fact that UI is making two separate requests for what appears to be an atomic action (save all changed terms) to the editor. While that haven't caused any issues before now, and probably was decided to be done that way so that changing labels, descriptions and aliases get their own edit revisions separate from each other (for easier rollbacks and more atomic history on level of term edits), bubbling up this kind of separation up to the UI is problematic in this case.

I’m pretty sure it has caused issues before now, we just haven’t addressed them yet. Part of the reason why it’s not great to use wbeditentity for these edits is that it results in a very generic edit summary (compare T67846 or T191885), but that will be tackled soon as part of the termbox hike, which already uses wbeditentity: T220696: [Story] Create better edit summaries for wbeditentity API endpoint

However, I don’t think that needs to block this task. For now, it’s okay if the label edit goes through and the description is blocked (my understanding is that these cases are often mistakes by confused users, where the label is correct but the description isn’t), and we can switch to wbeditentity later, once it has better edit summaries.

@Lucas_Werkmeister_WMDE gold 👍

Just wanted to raise awareness of that aspect, and seems I was lacking behind the current awareness around the topic.. that's awesome news!

Change 506172 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add LabelDescriptionNotEqualValidator

https://gerrit.wikimedia.org/r/506172

Change 506181 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Add CompositeFingerprintValidator

https://gerrit.wikimedia.org/r/506181

Change 506182 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Use LabelDescriptionNotEqualValidator in TermValidatorFactory

https://gerrit.wikimedia.org/r/506182

I tested it on beta and prod and both seems to work, but now I'm a bit confused. Is it already live, or did I test the wrong thing?

In T212869#5152180, @Lea_Lacroix_WMDE wrote:

I tested it on beta and prod and both seems to work, but now I'm a bit confused. Is it already live, or did I test the wrong thing?

It is already live and it should work on both. I guess if you followed the steps written in the description, then you tested it right.

This feature should be switched off until T259323 is solved.