Make WordPress Core

Opened 14 years ago

Last modified 2 years ago

#13867 assigned enhancement

New filter for comment RSS feed's title

Reported by: shidouhikari's profile shidouhikari Owned by: killua99's profile killua99
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Feeds Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

I'd like to be able to customize comments titles in RSS feed.

Currently it's hardcoded and has no way to be changed, so I added 2 new filters so that plugins can edit them.

I've tested and patch is working for me.

Attachments (9)

feed-rss2-comments.php.diff (1.9 KB) - added by shidouhikari 14 years ago.
Adds 2 filtters to allow plugins to edit comments titles in RSS feed
13867.diff (2.6 KB) - added by DrewAPicture 9 years ago.
+ hook docs
13867.2.diff (2.6 KB) - added by stevenkword 9 years ago.
updates hook doc
13867.3.patch (2.8 KB) - added by killua99 7 years ago.
13867.3.diff (2.9 KB) - added by telmoteixeira 6 years ago.
13867.4.diff (3.1 KB) - added by telmoteixeira 6 years ago.
13867.4.test.diff (2.6 KB) - added by telmoteixeira 6 years ago.
13867.5.diff (7.2 KB) - added by audrasjb 2 years ago.
Feeds: Add a hook to filter RSS2 and ATOM Comments feeds page title
13867.6.diff (7.2 KB) - added by audrasjb 2 years ago.
Feeds: Add a hook to filter RSS2 and ATOM Comments feeds page title

Download all attachments as: .zip

Change History (46)

@shidouhikari
14 years ago

Adds 2 filtters to allow plugins to edit comments titles in RSS feed

#1 @nacin
14 years ago

  • Keywords early removed
  • Milestone changed from 3.1 to Future Release

These can already be filtered through gettext filters.

#2 @shidouhikari
14 years ago

  • Cc shidouhikari added

As we talked in IRC, using gettext one would need to test for 'Comment on %1$s by %2$s' and 'By: %s'...

'By: %s' isn't something very exact for title filtering. A specific filter is simpler to handle and has no risk of other 'By: %s' somewhere else being messed. Also, if in a future version one of those texts are changed, anything relying on them would get broken.

With a specific filter, just let core do its work and then filter the result.

And sorry about the early, I thought it means patches in an early new milestone :p

#3 @nacin
14 years ago

  • Milestone changed from Future Release to Awaiting Triage
  • Type changed from feature request to enhancement

Per IRC.

#4 @nacin
13 years ago

  • Milestone changed from Awaiting Triage to Future Release

#5 @chriscct7
9 years ago

Patch still looks good

#6 @DrewAPicture
9 years ago

  • Keywords needs-docs needs-patch added; has-patch removed

A few things here:

  • Both filters could probably use better names
  • Both would need hook documentation for consideration
  • As we need to make sure whatever output is passed through ent2ncr() it might be better to do it just-in-time when the filter value is output, and remove the others from the existing sprintf lines.

@DrewAPicture
9 years ago

+ hook docs

#7 follow-up: @DrewAPicture
9 years ago

  • Keywords has-patch added; needs-docs needs-patch removed
  • Milestone changed from Future Release to 4.2

Seems like with all of the RSS filters we already have, we'd have one or two in place we could reuse, but I'm not really seeing viable alternatives to the ones proposed in the patch.

13867.diff refreshes the 5-year-old patch and adds hook docs. Moving to 4.2 for consideration.

#8 in reply to: ↑ 7 ; follow-up: @stevenkword
9 years ago

Drew, I'm taking a serious look at the way we are applying filters to RSS. I'm finding that we are using too many catchall functions, and I think we need to re-examine the XML and RSS syntax rules. I'm currently putting together a proposal that relates to this ticket.

I'm currently thinking we need to have filters for individual encodings such as proposed in #19998, but I want to hold further comment until I have my head better wrapped around the big picture.

Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#9 in reply to: ↑ 8 @mdgl
9 years ago

Replying to stevenkword:

Drew, I'm taking a serious look at the way we are applying filters to RSS. I'm finding that we are using too many catchall functions, and I think we need to re-examine the XML and RSS syntax rules. I'm currently putting together a proposal that relates to this ticket.

I agree our whole approach to encoding XML/RSS could do with a bit of a re-vamp, probably on a new ticket. It's actually quite a tricky problem and for a long time we have tended just to throw CDATA blocks around until things appear to work.

See #3670 for the beginnings of some ideas about a more general esc_xml() abstraction. Also related #28816.

This ticket was mentioned in Slack in #core by drew. View the logs.


9 years ago

#11 @ocean90
9 years ago

  • Milestone changed from 4.2 to Future Release

What would be an example use-case for this filter?

#12 @stevenkword
9 years ago

@ocean90 -- Changing the feed title to something that doesn't match the existing phrasing. e.g.) "Comments on: Hello World!" to something like "What People are Saying about Hello World!"

13867.diff still applies cleanly and 13867.2.diff updates the hook doc. I think this is ready to go.

@stevenkword
9 years ago

updates hook doc

#13 @stevenkword
7 years ago

  • Keywords needs-refresh added; has-patch removed

Patch no longer applies cleanly.

#14 @stevenkword
7 years ago

  • Keywords good-first-bug added

@killua99
7 years ago

#15 @killua99
7 years ago

Updating the code till the latest trunk

#16 @dingo_bastard
7 years ago

  • Keywords has-patch added

#17 @DrewAPicture
6 years ago

  • Owner set to killua99
  • Status changed from new to assigned

Looks like the attached patch here will need another refresh. Assigning this good-first-bug ticket to mark it as "claimed".

#18 @telmoteixeira
6 years ago

  • Keywords needs-unit-tests added; needs-refresh removed

Patch refreshed

#19 @telmoteixeira
6 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

After a closer look, created a new patch and added a unit test.

This ticket was mentioned in Slack in #core by sergey. View the logs.


3 years ago

#21 @SergeyBiryukov
3 years ago

  • Milestone changed from Future Release to 5.9

@audrasjb
2 years ago

Feeds: Add a hook to filter RSS2 and ATOM Comments feeds page title

#22 @audrasjb
2 years ago

  • Keywords dev-feedback added; good-first-bug removed

In 13867.5.diff, I partially rewrote the previous patch:

  • better function naming
  • only address main feed title, and not item titles to keep the ticket in a smaller scope
  • rewrote Unit tests
  • add support for ATOM feeds
  • add unit tests for ATOM feeds

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#24 @hellofromTonya
2 years ago

Thank you @audrasjb for updating and improving the patch.

I'm wondering: should the filtered titles, i.e. $feed_title, be escaped before echoing?

#25 @audrasjb
2 years ago

  • Keywords needs-refresh added; dev-feedback removed

Given <title> doesn't allow any HTML tag, I think you're right @hellofromTonya. Patch incoming in two mins :)

@audrasjb
2 years ago

Feeds: Add a hook to filter RSS2 and ATOM Comments feeds page title

#26 @audrasjb
2 years ago

  • Keywords commit added; needs-refresh removed

I think we're good to go now.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#28 @hellofromTonya
2 years ago

  • Keywords needs-refresh added; commit removed

The tests are failing missing test method:

1) Tests_Feed_Atom::test_feed_title
Error: Call to undefined method Tests_Feed_Atom::do_rss2_comments()

/var/www/tests/phpunit/tests/feed/atom.php:151

2) Tests_Feed_RSS2::test_feed_title
Error: Call to undefined method Tests_Feed_RSS2::do_rss2_comments()

/var/www/tests/phpunit/tests/feed/rss2.php:143

Removing commit and adding needs-refresh.

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 years ago

This ticket was mentioned in PR #1852 on WordPress/wordpress-develop by audrasjb.


2 years ago
#30

  • Keywords needs-refresh removed

#31 @audrasjb
2 years ago

  • Keywords early added
  • Milestone changed from 5.9 to Future Release

Wrong ticket, removing my comment

Last edited 2 years ago by audrasjb (previous) (diff)

#32 @audrasjb
2 years ago

  • Keywords early removed
  • Milestone changed from Future Release to 5.9

#33 @audrasjb
2 years ago

I addressed the missing tests issue in the above PR.
Marking this for commit.

This ticket was mentioned in Slack in #core by audrasjb. View the logs.


2 years ago

#35 @audrasjb
2 years ago

  • Keywords commit added

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


2 years ago

#37 @peterwilsoncc
2 years ago

  • Keywords commit removed
  • Milestone changed from 5.9 to Future Release

I'm going to bump this to a future release given the feature freeze today.

As noted on the PR:

  • there's an unrelated change to this ticket's purpose
  • I have some questions about the order of operations re: escaping.

There's no 6.0 milestone as yet but I think this is pretty close so can probably be added once the milestone is set up on trac.


Additional props due @costdev for feedback provided via slack.

Note: See TracTickets for help on using tickets.