Fix an issue with editing pre-space objects using a form with no visibility controls

@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge

phorge phabricator

fork

Summary:
WMF ran into this after their update. Here's the setup:

- When you enable Spaces, we leave all existing objects set to `null`, which means "these belong to the default space". This is so we don't have to go update a trillion objects.
- New objects get set to the default space explicitly (`PHID-SPCE-...`) but older ones stay with `null`.
- If you edit an older object (like a task) from the time before Spaces, //and// the form doesn't have a Visbility/Spaces control, we would incorrectly poplate the value with `null` when the effective value should be the default space PHID.
- This caused a "You must choose a space." error in the UI.

Instead, populate the control with the effective value instead of the literal database value. This makes the edit go through cleanly.

Also add a note about this for future-me.

Test Plan:
- Disabled "Visibility" control in task edit form.
- Edited an old task which had `null` as a `spacePHID` in the database.
- Before patch: UI error about selecting a Space.
- After patch: edit goes through cleanly.

Reviewers: chad, 20after4

Reviewed By: chad, 20after4

Subscribers: 20after4, aklapper

Differential Revision: https://secure.phabricator.com/D15306

epriestley 10 years ago af1fef24 dc7d0b4a

+9 -1

2 changed files

expand all

src

applications

policy

editor

PhabricatorPolicyEditEngineExtension.php

transactions

storage

PhabricatorApplicationTransaction.php

+4 -1

src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php

··· 101 101 if ($capability == PhabricatorPolicyCapability::CAN_VIEW) { 102 102 $type_space = PhabricatorTransactions::TYPE_SPACE; 103 103 if (isset($types[$type_space])) { 104 + $space_phid = PhabricatorSpacesNamespaceQuery::getObjectSpacePHID( 105 + $object); 106 + 104 107 $space_field = id(new PhabricatorSpaceEditField()) 105 108 ->setKey('spacePHID') 106 109 ->setLabel(pht('Space')) ··· 114 117 ->setConduitDescription( 115 118 pht('Shift the object between spaces.')) 116 119 ->setConduitTypeDescription(pht('New space PHID.')) 117 - ->setValue($object->getSpacePHID()); 120 + ->setValue($space_phid); 118 121 $fields[] = $space_field; 119 122 120 123 $space_field->setPolicyField($policy_field);

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

··· 527 527 // TODO: Remove this eventually, this is handling old changes during 528 528 // object creation prior to the introduction of "create" and "default" 529 529 // transaction display flags. 530 + 531 + // NOTE: We can also hit this case with Space transactions that later 532 + // update a default space (`null`) to an explicit space, so handling 533 + // the Space case may require some finesse. 534 + 530 535 if ($this->getOldValue() === null) { 531 536 return true; 532 537 } else {

Configure Feed

Configure Feed