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.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T5867: Move Quick Create to MenuItemEngine
- Commits
- rP8a85ee7c1575: Add CustomPHID to PhabricatorProfileMenuEngineConfiguration
Not wired to anything, but pulling up project menu, editing, all still works.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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 | |
Comment Actions
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.
Comment Actions
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.
Comment Actions
I would be semi-tempted to write the profileObject as the customPHID for global changes... or maybe that's stupid.
Comment Actions
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. | |
| src/applications/search/engine/PhabricatorProfileMenuEngine.php | ||
|---|---|---|
| 266–267 | oh right | |
Comment Actions
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.