Page MenuHomePhabricator

Add CustomPHID to PhabricatorProfileMenuEngineConfiguration
ClosedPublic

Authored by chad on Jan 6 2017, 7:07 PM.

Details

Summary

Ref T5867, adds a customPHID field, nullable, and lets you query by it... i think? Not fully able to grok all the EditEngine stuff, but I think this is the right place for the query.

Test Plan

Not wired to anything, but pulling up project menu, editing, all still works.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

I'm surprised this works as-is, but let me poke at it and make sure I'm not crazy. Everything generally looks good, just a couple of minor things that I think are issues...

src/applications/search/engine/PhabricatorProfileMenuEngine.php
257

I expect this will fatal when there is no customObject, because you will query by withCustomPHIDs(array()), which will resolve to a %Ls on array() and fatal ("%Ls conversion must have at least one element in the array you pass").

Instead, add this constraint only if there is a custom object.

src/applications/search/query/PhabricatorProfileMenuItemConfigurationQuery.php
109–127

I think this will do the wrong thing for items with no custom PHID. It will incorrectly reject them because it can't load the objects, but it's valid to have no customPHID.

I also think we probably don't need this -- it will make the query a little slower and I think we won't ever actually access the custom objects, and don't need to perform policy checks on them.

src/applications/search/engine/PhabricatorProfileMenuEngine.php
257

Yeah, I thought it should too, seems like ... it's not fataling the page, but adding custom items doesn't work. Let me see, maybe I uploaded old revision

Yeah, neither of these fatal, they just drop all the customizations. Specifically, I think this is happening:

  • The unconditional withCustomPHIDs() causes us to query for customPHID IN (''), which never matches anything (IN ('') does not match NULL).
  • The customObject filtering would drop anything which managed to load.

So the effect is that any customizations are lost, although nothing fatals.

e.g. master shows a custom menu:

Screen Shot 2017-01-06 at 11.57.31 AM.png (1×1 px, 175 KB)

This patch with no edits to the data reverts it:

Screen Shot 2017-01-06 at 11.57.39 AM.png (1×1 px, 165 KB)

I think fixing those two inlines will resolve that.

chad planned changes to this revision.Jan 6 2017, 8:00 PM

I'll get back to you

Also, a couple diffs down the road we'll need a way to query "global items, plus items with a specific custom PHID". Ideally, that would just be one query.

We don't have a super great way to express that in the general case, since most applications don't support a query like that.

One way might be:

withCustomPHIDs(array $phids, $with_global)

Where the behavior is:

  • If $phids is nonempty and $with_global is true, query for customPHID IN (%Ls) OR customPHID IS NULL.
  • If $phids is nonempty and $with_global is false, query for customPHID IN (%Ls).
  • If $phids is empty and $with_global is true, query for customPHID IS NULL.
  • If $phids is empty and $with_global is false, don't apply any constraint. This is sort of unintuitive but otherwise -- if this means "no results" --$query->withIDs(array(1))->executeOne() to load a particular item won't work.
chad marked 3 inline comments as done.
  • fixes per comments, remove object attachment

I would be semi-tempted to write the profileObject as the customPHID for global changes... or maybe that's stupid.

It would probably be fine to do it like that, but it means we can never have, e.g., a user customize their own profile menu, since both the user's "global" menu and their "custom" menu would have the same customPHID. Maybe we'll never do that, but I think we usually tend to be better off keeping the database representation of things "clean" and doing some extra work in PHP, since it's a lot easier to change PHP code than migrate data.

src/applications/search/engine/PhabricatorProfileMenuEngine.php
266–267

This is fine for now, but just note that it will load the global config and all custom config for all users.

This revision is now accepted and ready to land.Jan 7 2017, 4:27 PM
src/applications/search/engine/PhabricatorProfileMenuEngine.php
266–267

oh right

btw this says I have unsubmitted comments

and I dont and my actions are blank too

I have a note about that locally, but basically the "drafts" indicator is still reading the old table. Should only affect changes where you made draft changes in the main comment area before the EditEngine upgrade, I think, and will go away once I clean that up and nuke the old table.

This revision was automatically updated to reflect the committed changes.