Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 6 years ago

#5926 closed enhancement (maybelater)

getting an activity permalink instead of a shortlink

Reported by: djpaul's profile DJPaul Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Activity Keywords: needs-patch, trac-tidy-2018
Cc:

Description

Our existing bp_activity_get_permalink method returns the permalink for a single activity item, right? Actually, that's not true, because if you look at the function, it returns what I'd actually describe as a short link. For example:

bp_activity_get_permalink(567) will return http://example.com/activity/p/567/.
That URL will redirect to http://example.com/members/admin/activity/567/.

I think bp_activity_get_permalink should return the canonical permalink (the second link), not the first. It would be more intuitive, bring it inline with WP's get_permalink which I'd guess many people who extend BP are familiar with, and finally give us a way to get the canonical link for a piece of activity content without having to dig into $_SERVER.

If this proposed change is accepted, I suggest we rename bp_activity_get_permalink to bp_activity_get_shortlink, and build a real bp_activity_get_permalink.

Attachments (3)

5926.01.patch (12.7 KB) - added by DJPaul 10 years ago.
5926.02.patch (12.8 KB) - added by DJPaul 9 years ago.
5926.03.patch (12.6 KB) - added by DJPaul 9 years ago.

Download all attachments as: .zip

Change History (27)

#1 @djpaul
10 years ago

In 9061:

Tests: update activity tests to use bp_activity_get_permalink to get a link to an activity item rather than manually constructing a link from an activity ID.

See #5926

#2 @djpaul
10 years ago

In 9062:

Activity: when redirecting unauthenticated users away from a single activity permalink, make sure the value of the redirect_to parameter has been URL encoded.

Also improves the readability of this section of code by moving it out of the ternary operator.

See #5926

#3 @djpaul
10 years ago

In 9063:

Activity: refactor part of bp_get_activity_latest_update to use bp_activity_get_permalink to get a link to an activity item rather than manually constructing the link.

See #5926

@DJPaul
10 years ago

#4 @DJPaul
10 years ago

  • Keywords has-patch added

5926.01.patch is what I'm suggesting. It renames bp_activity_get_permalink to bp_activity_get_shortlink, and bp_activity_get_permalink is re-built to return the canonical URL instead. I've swapped some existing use out for the new shortlink function places where this function is used (where I think people might care) -- purely for aesthetic backwards consistency, rather than any technical reason.

#5 @DJPaul
9 years ago

  • Keywords commit added

This ticket was mentioned in Slack in #buddypress by djpaul. View the logs.


9 years ago

#7 @DJPaul
9 years ago

  • Keywords needs-refresh 2nd-opinion added; has-patch commit removed

Have asked tw2113 for an 2nd opinion regarding phpDoc for the functions and actions.

This ticket was mentioned in Slack in #buddypress by tw2113. View the logs.


9 years ago

#9 follow-up: @tw2113
9 years ago

My VCS-fu must not be strong enough yet, because I'm having trouble getting the patch to apply cleanly to an uptodate copy of trunk. I figure worst case scenario, I type up the phpdocs for the hooks that can be applied manually after someone with some more experience gets the patch refreshed.

#10 @tw2113
9 years ago

To add to that, [9194] reworked some of the innards of bp_activity_get_permalink as well, for example.

#11 in reply to: ↑ 9 @boonebgorges
9 years ago

Replying to tw2113:

My VCS-fu must not be strong enough yet, because I'm having trouble getting the patch to apply cleanly to an uptodate copy of trunk. I figure worst case scenario, I type up the phpdocs for the hooks that can be applied manually after someone with some more experience gets the patch refreshed.

It does not apply cleanly, probably because of [9194]. If you're able to refresh it, it'd be helpful. (This is a manual process - you'll have to manually merge the chunks of the patch that do not cleanly apply. No VCS-fu involved.)

#12 @tw2113
9 years ago

My question at this point is do we keep imath's version of bp_activity_get_permalink for example, or do we need to discuss which direction the function goes going forward. I know Paul is proposing renaming it completely.

#13 @DJPaul
9 years ago

  • Milestone changed from 2.2 to 2.3

I don't think I'll have time to refresh the patch for 2.2, but I'll get it done for 2.3.

#14 @DJPaul
9 years ago

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

@DJPaul
9 years ago

#15 @DJPaul
9 years ago

5926.02.patch is refreshed for current trunk. The new bp_activity_get_shortlink does duplicate most of bp_activity_get_permalink, but I am struggling to think of a nice way of avoiding the duplication.

#16 @DJPaul
9 years ago

  • Keywords has-patch added; needs-refresh removed

#17 @DJPaul
9 years ago

I guess I could try adding another parameter to permalink() that sets whether to return the shortlink or not, and make shortlink() into a kind of wrapper.

#18 @DJPaul
9 years ago

  • Milestone changed from 2.3 to 2.4

I forgot about this. I'll commit to this for 2.4.

@DJPaul
9 years ago

#19 @DJPaul
9 years ago

  • Keywords needs-testing needs-unit-tests added; 2nd-opinion removed
  • Milestone changed from 2.4 to 2.5

I have refreshed the patch for current trunk so it applies cleanly (the since version is out of date) but one of the new unit tests is failing, and I think it's something to do with our test runner's go_to function which is pretty gnarly (it might not handle redirects?), and I don't have time/desire to figure it out for 2.4.

#20 @DJPaul
8 years ago

  • Milestone changed from 2.5 to 2.6

#21 @DJPaul
8 years ago

  • Keywords needs-patch added; has-patch needs-testing needs-unit-tests removed
  • Milestone changed from 2.6 to Future Release

This patch doesn't apply cleanly anymore, and last I knew, it was breaking the tests. Punting.

#22 @DJPaul
8 years ago

  • Owner DJPaul deleted

#23 @DJPaul
6 years ago

  • Keywords trac-tidy-2018 added

We're closing this ticket because it has not received any contribution or comments for at least two years. We have decided that it is better to close tickets that are good ideas, which have not gotten (or are unlikely to get) contributions, rather than keep things open indefinitely. This will help us share a more realistic roadmap for BuddyPress with you.

Everyone very much appreciates the time and effort that you spent sharing your idea with us. On behalf of the entire BuddyPress team, thank you.

If you feel strongly that this enhancement should still be added to BuddyPress, and you are able to contribute effort towards it, we encourage you to re-open the ticket, or start a discussion about it in our Slack channel. Please consider that time has proven that good ideas without contributions do not get built.

For more information, see https://bpdevel.wordpress.com/2018/01/21/our-awaiting-contributions-milestone-contains/
or find us on Slack, in the #buddypress channel: https://make.wordpress.org/chat/

#24 @DJPaul
6 years ago

  • Milestone Awaiting Contributions deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.