Page MenuHomePhabricator

Update Phurl to use EditEngine
ClosedPublic

Authored by jcox on Sep 19 2016, 6:11 PM.
Tags
None
Referenced Files
F14370619: D16573.id.diff
Fri, Dec 20, 6:09 PM
Unknown Object (File)
Thu, Dec 19, 9:18 AM
Unknown Object (File)
Sun, Dec 15, 5:33 PM
Unknown Object (File)
Sun, Dec 15, 5:33 PM
Unknown Object (File)
Sun, Dec 8, 7:00 AM
Unknown Object (File)
Sun, Dec 8, 5:56 AM
Unknown Object (File)
Thu, Dec 5, 8:00 AM
Unknown Object (File)
Nov 19 2024, 1:06 PM
Tokens
"Piece of Eight" token, awarded by yelirekim.

Details

Summary

Fixes T10673. Set up Phurl to use Edit Engine. There's no way this is all I needed to do to get it working, so I'll be making another pass at it and testing more thoroughly...

Test Plan

Ran through the Phurl URL creation/edit/deletion process.

Diff Detail

Repository
rP Phabricator
Branch
PhurlEditEngine (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 13772
Build 17790: Run Core Tests
Build 17789: arc lint + arc unit

Event Timeline

jcox retitled this revision from to Update Phurl to use EditEngine.
jcox updated this object.
jcox edited the test plan for this revision. (Show Details)

Edit engine is deceitfully magical, and you may have indeed done everything you needed to here.

epriestley added a reviewer: epriestley.

This looks correct to me.

In this diff, you should change PhabricatorPhurlController->buildApplicationCrumbs() to use EditEngine to generate the "Create Thing" link. You can follow ManiphestController->buildApplicationCrumbs(). Without this change, the "Create URL" button won't automatically update to reflect the availability of multiple forms if users later define them.

In this diff, you should also change PhabricatorPhurlApplication->getRoutes() to use $this->getEditRoutePattern('edit/') to generate the edit route, instead of hard-coding edit/. You can look at PhabricatorPasteApplication for an example. You should be able to remove the create/ route after making this change. (It looks like Paste also has a defunct create/ route which could be safely removed). Without this change, stuff lik Edit URLConfigure FormHTTP Parameters won't work properly.

In this or a future diff, you should change PhabricatorPhurlURLViewController to use EditEngine for commenting. D16222 has some extra stuff, but the changes to PhamePostViewController->buildCommentForm() in that change are generally reasonable as a model. Or just look at PhabricatorPasteViewController for a possibly-simpler example. After making this change, delete PhabricatorPhurlURLCommentController and the route for it (these will be fully handled by EditEngine, and no longer need a separate route). This change will enable the "Change Subscribers" and "Change Project Tags" actions when adding a comment, improve the draft code, and reduce the total amount of code in the codebase.

This revision now requires changes to proceed.Sep 19 2016, 6:41 PM
jcox edited edge metadata.

Updated based on CR comment

epriestley edited edge metadata.

Pretty sure this covers everything, we can follow up if we both missed something.

This revision is now accepted and ready to land.Sep 19 2016, 8:12 PM
src/applications/phurl/editor/PhabricatorPhurlURLEditEngine.php
57

I changed this from getApplicationURI('edit/') to getApplicationURI('url/edit/') to get the edit+create links all working. Is my thinking correct here or should it be using a similar mechanism that the routes use rather than hard-coding this?

src/applications/phurl/editor/PhabricatorPhurlURLEditEngine.php
57

This is fine, some applications have /app/edit/ and others have /app/thing/edit/. Mostly depends on app complexity / which app got copy-pasted to create a new app. In theory we should probably always have it for consistency, but some apps like Paste are very unlikely to have multiple/secondary objects.

This revision was automatically updated to reflect the committed changes.