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

Summary

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

Test Plan

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

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

aklapper created this revision.May 20 2024, 20:32

Owners added a reviewer: O1: Blessed Committers.May 20 2024, 20:32

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptMay 20 2024, 20:32

Harbormaster completed remote builds in B1294: Diff 2035.May 20 2024, 20:32

aklapper requested review of this revision.May 20 2024, 20:32

This probably needs more work but seems to function locally.

Replace a call with a variable

Harbormaster completed remote builds in B1295: Diff 2036.May 20 2024, 20:44

Note to myself: Still need to test resulting URI when a custom logo is set

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".

Move policy.allow-public check from method itself to calling the method

Harbormaster completed remote builds in B1296: Diff 2038.May 21 2024, 11:52

In D25668#18281, @avivey wrote:

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?)

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.

Add PHPDoc; tested that return value is always plain string, also with custom logo

Harbormaster completed remote builds in B1297: Diff 2039.May 21 2024, 12:22

aklapper edited the test plan for this revision. (Show Details)May 21 2024, 13:05

valerio.bozzolan added inline comments.Jun 10 2024, 14:59

src/applications/maniphest/controller/ManiphestTaskDetailController.php
263	Maybe this is useful? PhabricatorMarkupEngine::summarizeSentence($desc)

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.

Harbormaster completed remote builds in B1337: Diff 2100.Jun 10 2024, 15:13

aklapper marked an inline comment as done.Jun 10 2024, 15:14

aklapper mentioned this in Z1: Phorge.Jul 16 2024, 20:20

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.

aklapper edited the test plan for this revision. (Show Details)Jul 27 2024, 15:22

valerio.bozzolan added inline comments.Jul 28 2024, 09:19

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

valerio.bozzolan awarded a token.Jul 28 2024, 09:23

aklapper edited the test plan for this revision. (Show Details)Jul 30 2024, 10:41

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

Harbormaster completed remote builds in B1477: Diff 2301.Jul 30 2024, 10:46

aklapper marked 3 inline comments as done.Jul 30 2024, 10:47

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!

valerio.bozzolan rescinded a token.Jul 31 2024, 07:30

valerio.bozzolan awarded a token.

This seems ready. Just the proposed inline things if you like to. Thanks.

This revision is now accepted and ready to land.Jul 31 2024, 09:02

aklapper edited the summary of this revision. (Show Details)Aug 1 2024, 11:47

aklapper edited the test plan for this revision. (Show Details)

What Valerio says: Only render OGP tags for anonymous access to public objects; refactor

Harbormaster completed remote builds in B1485: Diff 2310.Aug 1 2024, 11:52

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 🕵️‍♀️