Page MenuHomePhabricator

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

Authored by epriestley on Feb 18 2016, 5:52 PM.

Details

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.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

epriestley updated this revision to Diff 36916.Feb 18 2016, 5:52 PM
epriestley retitled this revision from to Fix an issue with editing pre-space objects using a form with no visibility controls.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: chad.
epriestley added inline comments.Feb 18 2016, 5:53 PM
src/applications/policy/editor/PhabricatorPolicyEditEngineExtension.php
104–105

This basically boils down to:

$space_phid = $object->getSpacePHID();
if ($space_phid) {
  return $space_phid;
}

// This is the meaning of `null`.
return "THE DEFAULT SPACE PHID";
chad accepted this revision.Feb 18 2016, 6:07 PM
chad edited edge metadata.
This revision is now accepted and ready to land.Feb 18 2016, 6:07 PM

WMF is verifying this on their end, I'll kick it in once they confirm it.

Works now at least on the task I had issues with.

Looks like it worked, I'll wait for one more confirmation to be sure.

got a second confirmation. it's fixed :)

Thanks everyone for such a quick fix!

20after4 accepted this revision.Feb 18 2016, 7:06 PM
20after4 added a reviewer: 20after4.
This revision was automatically updated to reflect the committed changes.