Page MenuHomePhorge

Add HTML header for Atom/RSS discovery on Phame blog pages
ClosedPublic

Authored by aklapper on Jul 13 2023, 13:21.
Tags
None
Referenced Files
Unknown Object (File)
Wed, May 22, 17:57
Unknown Object (File)
Mon, May 20, 09:01
Unknown Object (File)
Sun, May 12, 22:20
Unknown Object (File)
Sun, May 12, 22:20
Unknown Object (File)
Wed, May 1, 18:43
Unknown Object (File)
Mon, Apr 29, 23:00
Unknown Object (File)
Wed, Apr 24, 18:27
Unknown Object (File)
Wed, Apr 24, 18:27

Details

Summary

Allow easier discovery and subscribing to Atom feeds of Phame blogs.

To be extra cautious, also make sure that the added line is a PhutilSafeHTML.

Original author: @20after4

Closes T15550

Test Plan

Go to /phame/blog/view/1/ and check the HTML source code. See an additional <link> item with type="application/atom+xml" in the <head> section.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

avivey requested changes to this revision.Jul 17 2023, 08:05
avivey subscribed.

I think this should be implemented using the getHead() method that's already defined.

src/view/page/PhabricatorStandardPageView.php
385–386

You can have PHP do the type checking, but I'm not sure that this is needed - phutil_implode_html already accepts unsafe strings and converts them to safe html.

This revision now requires changes to proceed.Jul 17 2023, 08:05

I think this should be implemented using the getHead() method that's already defined.

Could you elaborate a bit, please? I'm not sure what exactly not to do here, and what to do instead. Thanks in advance!

I think this should be implemented using the getHead() method that's already defined.

Could you elaborate a bit, please? I'm not sure what exactly not to do here, and what to do instead. Thanks in advance!

I was thinking about the getHead() that's being called in line 411, but now I see that it's already being overriden in PhabricatorStandardPageView.

I'm a little weary of adding addHeadItem() method to Page, because it opens up the space of places strange things can enter the head; I'd rather have a dedicated PageWithAlternateLink class (or mixin?) that will explicitly override getHead and add it, but that's not really possible either.

src/view/page/PhabricatorStandardPageView.php
385–386

Probably should bother - head items are not normal content, and can't ever be something that isn't explicitly crafted to work in a head element.

This revision is now accepted and ready to land.Aug 1 2023, 15:53