Page MenuHomePhorge

Add Open Graph protocol meta tags to Maniphest task pages
ClosedPublic

Authored by aklapper on May 20 2024, 20:32.

Details

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
Branch
T15472ogp (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1296
Build 1296: arc lint + arc unit

Event Timeline

This probably needs more work but seems to function locally.

Replace a call with a variable

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

aklapper marked an inline comment as done.

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

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

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, &quot;Lorem adipisicing ea tempor consequat pariatur.&quot; Lorem veniam labore esse elit aliqua tempor. Ipsum es..."
  • Now using PhabricatorMarkupEngine::summarizeSentence, result is:
    • content="Dolor excepteur voluptate cupidatat, &quot;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.

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

  • 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

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!

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 test plan for this revision. (Show Details)

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

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

Make the PHPDoc and the method name shamelessly admit its real intentions to the whole world out there