Page MenuHomePhabricator

Basic Favorites application
ClosedPublic

Authored by chad on Jan 9 2017, 5:38 AM.

Details

Summary

Ref T5867. Rough in a Favorites application, not wired to anything.

Test Plan

tbd. currently 404s so... I messed up something. Tossing up to read.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

  • don't think this is needed

Maaaaaybe this should just be part of Flags, since you just need a PHID + some controllers? But code looks fine.

This revision is now accepted and ready to land.Jan 9 2017, 3:06 PM
src/applications/favorites/application/PhabricatorFavoritesApplication.php
26

I think this route is correct, but it 404s no matter what I put in, I assume then ProfileMenuRouting is 404'ing somewhere. Whats a good way to debug this?

I think it's working -- try /favorites/global/item/edit/ for example?

btw I didn't read the test plan at all lul

so /favorites/global/item/ should just return... the menu itself?

For projects, at least, it's just a 404. You could put the menu rendering there, but it's also fine for it to just 404 and for the rendering to be somewhere else.

chad planned changes to this revision.Jan 9 2017, 7:27 PM

Will toss up again here, some minor buggypoos.

Profile menu items can not be generated without an object context. is the current error when bringing up an edit page.

Show me the code hitting that error?

Unhandled Exception ("Exception")	
Profile menu items can not be generated without an object context.
Depth	Library	File	Where
7	phabricator	applications/transactions/editengine/PhabricatorEditEngine.php : 832	PhabricatorProfileMenuEditEngine::newEditableObject()
6	phabricator	applications/search/engine/PhabricatorProfileMenuEngine.php : 771	PhabricatorEditEngine::buildResponse()
5	phabricator	applications/search/engine/PhabricatorProfileMenuEngine.php : 151	PhabricatorProfileMenuEngine::buildItemEditContent()
4	phabricator	applications/favorites/controller/PhabricatorFavoritesMenuItemController.php : 19	PhabricatorProfileMenuEngine::buildResponse()
3	phabricator	aphront/configuration/AphrontApplicationConfiguration.php : 269	PhabricatorFavoritesMenuItemController::handleRequest()
2	phabricator	aphront/configuration/AphrontApplicationConfiguration.php : 181	AphrontApplicationConfiguration::processRequest()
1		/var/www/html/dev/core/lib/phabricator/webroot/index.php : 17	AphrontApplicationConfiguration::runHTTPRequest()

Oh, sorry, I mean update this revision so I can patch it locally and take a look?

sure, it's only one line change tho

This revision is now accepted and ready to land.Jan 9 2017, 7:42 PM

I feel like my applicationquery isn't returning a full object.

pasted_file (318×692 px, 37 KB)

That's valid, applications just don't have any properties.

The error you're hitting is "normal", in the sense that /edit/ doesn't do anything. Here's /edit/ in Projects:

https://secure.phabricator.com/project/1/item/edit/

I'll fix that to 404.

Things look better if you try /favorites/global/item/configure/.

  • add constants
  • check for type
  • build basic manage item
  • 404s now when I try to add something

Pushed this a little further but getting 404's when I try to add or configure an item now.

src/applications/favorites/engine/PhabricatorFavoritesProfileMenuEngine.php
15

This needs to either link to /favorites/global/ or /favorites/personal/ depending on whether a custom PHID is set.

Omg I checked that like 5 times and I always thought I set it. I need more caffeine.

  • fix adding/removing global items
  • fix saving custom_phid
src/applications/search/storage/PhabricatorProfileMenuItemConfiguration.php
35–46

@epriestley is this correct way? it works, and I think I understand why.

The custom PHID stuff looks right to me.

Since you just set customPHID directly when you create the object and never change it (and it probably doesn't make sense to ever change it?) you could also get rid of all the transaction stuff, I think -- nothing will ever actually apply those transactions or interact with that code. Unless you're planning to let users reassign items to other users or something.

This was the only means I could set the customPHID, through the initialization, is there another way?

I think this is the best way to do it (on initialization).

You can also do it by overriding willApplyTransactions() on EditEngine and injecting new transactions when the object is created, or by adding hidden fields which generate the transactions. Both of these methods are much more complicated and don't provide any benefits in this case.

This revision was automatically updated to reflect the committed changes.

I guess I should pull cat-facts

less ye have cat facts in ur menu and we get buggo reports