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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

chad created this revision.Jan 6 2017, 7:07 PM
epriestley edited edge metadata.Jan 6 2017, 7:55 PM

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.

chad added inline comments.Jan 6 2017, 7:57 PM
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:

This patch with no edits to the data reverts it:

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 updated this revision to Diff 41244.Jan 7 2017, 4:10 AM
chad marked 3 inline comments as done.
  • fixes per comments, remove object attachment
chad added a comment.Jan 7 2017, 4:11 AM

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

chad updated this revision to Diff 41245.Jan 7 2017, 4:14 AM
  • derp
Harbormaster completed remote builds in B15132: Diff 41245.
epriestley accepted this revision.Jan 7 2017, 4:27 PM

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
chad added inline comments.Jan 7 2017, 5:14 PM
src/applications/search/engine/PhabricatorProfileMenuEngine.php
266–267

oh right

chad added a comment.Jan 7 2017, 5:38 PM

btw this says I have unsubmitted comments

chad added a comment.Jan 7 2017, 5:39 PM

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.

chad added a comment.Jan 7 2017, 5:40 PM

oix k thx :3

This revision was automatically updated to reflect the committed changes.