Add OGP <meta> tags to Maniphest task pages when the task is publicly accessible and anonymously accessed. See https://ogp.me/
Based on rP2c72c2b924ffa3f8a49dbec636a2cdca3bae004f reverted in rP49b57eae7df52c189aef1d973823c697fc97fd4b.
Closes T15472
Details
Details
- Reviewers
valerio.bozzolan - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15472: Support Open Graph protocol (at least in Maniphest)
- Commits
- rP89778dc9e617: Add Open Graph protocol meta tags to Maniphest task pages
- Use the default Phorge logo, open a Maniphest task, look at the headers in its HTML.
- Set a custom Phorge logo via config/edit/ui.logo/.
- Access a task with "View Policy: All Users" while logged in: No OGP headers included.
- Access a task with "View Policy: Public" while logged in: No OGP headers included.
- Access a task with "View Policy: All Users" while logged out: No OGP headers included; "Access Denied: Restricted Maniphest Task" displayed.
- Access a task with "View Policy: Public" while logged out: OGP headers included.
- Access a task with "View Policy: Public" while logged out with a task description and a task without a task description: OGP headers included.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- T15472ogp (branched from master)
- Lint
Lint Passed - Unit
Tests Passed - Build Status
Buildable 1296 Build 1296: arc lint + arc unit
Event Timeline
Comment Actions
need to also test the result of renderPhabricatorLogo() when no logo set, when a custom logo is set, and when a custom logo is set but hidden (maybe. Maybe the logo file should be Attached to something public, so this situation can't really happen?)
| src/applications/maniphest/controller/ManiphestTaskDetailController.php | ||
|---|---|---|
| 228–231 | I prefer to do this test where calling the method, so that this method doesn't have "surprising behavior". | |
Comment Actions
In my understanding, renderPhabricatorLogo() now calls PhabricatorCustomLogoConfigType::getLogoURI($viewer) which covers all these cases as its fallback serves the file /rsrc/image/logo/project-logo.png.
When that file does not exist, every Phorge page will throw EXCEPTION: (RuntimeException) filemtime(): stat failed.
| src/applications/maniphest/controller/ManiphestTaskDetailController.php | ||
|---|---|---|
| 263 | Maybe this is useful? PhabricatorMarkupEngine::summarizeSentence($desc) | |
Comment Actions
Use PhabricatorMarkupEngine::summarizeSentence() instead of mb_substr().
PhabricatorMarkupEngine::summarizeSentence($desc) sets ->setMaximumGlyphs(128) "close enough" to the 140 that I deliberately chose (plus I didn't test much mb_substr's relation with UTF8).
But it cuts pretty early (and has issues like https://phabricator.wikimedia.org/T130557 ):
- Using mb_substr, result is:
- content="Dolor excepteur voluptate cupidatat, "Lorem adipisicing ea tempor consequat pariatur." Lorem veniam labore esse elit aliqua tempor. Ipsum es..."
- Now using PhabricatorMarkupEngine::summarizeSentence, result is:
- content="Dolor excepteur voluptate cupidatat, "Lorem adipisicing ea tempor consequat pariatur." />
I don't have a strong opinion, I guess I am fine switching to summarizeSentence for now because likely better thought-out/tested regarding UTF8 etc. In a future step we could always introduce a second function call parameter to define the number of max glyphs.
Comment Actions
Replying to a comment in Z1 from a week ago: When Phorge's curtain doesn't allow someone to access the content, it shows either a 404 ("Access Denied: Restricted Maniphest Task") or the login dialog. In both cases, no OGP meta tags are leaked.
| src/applications/maniphest/controller/ManiphestTaskDetailController.php | ||
|---|---|---|
| 217 | We can clarify that this adds tags, but another method gets data (?) | |
| 223–230 | To separate data and rendering process, We can have a method that just returns values, example in line 223 here: https://we.phorge.it/differential/diff/2286/ And a method that append tags with the correct logic, example in line 254 here: https://we.phorge.it/differential/diff/2286/ Do you like it? This is probably the minimal necessary change to make this minimally extensible. (So in the future we may want to move just addOpenGraphProtocolMetadataTags() in the parent class) Feel free to say "Yes whatever, but please amend yourself - asdlol - I just want to shipit" | |
| 258 | 0 is already false | |
Comment Actions
- Split getting values and rendering meta tags into separate private methods
- Fix description length check
Left as a future exercise: Make more generic to not only cover task objects
Comment Actions
Tested again. This feature is beautiful. Thanks again. Let's increase the Wealth token.
| src/applications/maniphest/controller/ManiphestTaskDetailController.php | ||
|---|---|---|
| 215–216 | The website may allow public but the task may be private. Let's check $task instead then current environment. Also, let's check $viewer to exclude logged-in users. At this point, is probably better to introduce a method like: /** * Check if this page should show Open Graph tags * @param PhabricatorUser $viewer Viewer that may want Open Graph tags * @param object $object * @return bool */ private function shouldShowOpenGraph(PhabricatorUser $viewer, $object) { // Don't waste time adding OpenGraph tags to logged-in users. if ($viewer->getIsStandardUser()) { return false; } // Show OpenGraph tags only to public objects. return $object->getViewPolicy() === PhabricatorPolicies::POLICY_PUBLIC; } Bonus point: and we generalized even more! | |
Comment Actions
What Valerio says: Only render OGP tags for anonymous access to public objects; refactor
Comment Actions
I like ping-pong :D
| src/applications/maniphest/controller/ManiphestTaskDetailController.php | ||
|---|---|---|
| 228 | Nice. I think showOpenGraphMetadata() can be confused with an order to show something; like buildHeaderView() that builds something ⚒️ Evaluate getShowOpenGraphMetadata() or similar for this getter, like other getters 🕵️♀️ | |
Comment Actions
Make the PHPDoc and the method name shamelessly admit its real intentions to the whole world out there