Skip to:
Content

BuddyPress.org

Opened 13 years ago

Closed 4 years ago

#1269 closed enhancement (maybelater)

Create a BP_Loop_Template base class that all template classes can inherit from

Reported by: rvenable's profile rvenable Owned by: johnjamesjacoby's profile johnjamesjacoby
Milestone: Priority: normal
Severity: normal Version:
Component: Templates Keywords: needs-refresh, trac-tidy-2018
Cc: mercijavier@…

Description

Most of the classes in the templatetags.php files (for instance, BP_Blogs_User_Blogs_Template or BP_Groups_User_Groups_Template) all use common functionality for looping through an array of items. This functionality could be put into a base class. New template classes could be made simply by extending the base class and creating their own custom constructor. This would make it easier for plugin developers to quickly create new template classes.

I've attached a class I made that does what I am talking about (made from the example class in the BP Skeleton Component).

Attachments (5)

loop-template.php (3.4 KB) - added by rvenable 13 years ago.
Base class BP_Loop_Template
1269.01.patch (18.9 KB) - added by imath 7 years ago.
1269.02.patch (40.7 KB) - added by imath 7 years ago.
Updated to r9260
1269.unittests.patch (50.1 KB) - added by imath 7 years ago.
Unit tests to use before and after 1269.03.patch
1269.03.patch (133.5 KB) - added by imath 7 years ago.
The patch :)

Download all attachments as: .zip

Change History (27)

@rvenable
13 years ago

Base class BP_Loop_Template

#1 @DJPaul
13 years ago

If this was accepted by the powers-that-be, it'd have to be written to work with PHP4.

#2 @petronic
13 years ago

I agree with rvenable. Some kind of template class would really make developing much easier, without boring repetitive steps.

#3 @DJPaul
12 years ago

  • Milestone changed from 1.2 to Future Release

#4 @johnjamesjacoby
12 years ago

  • Component set to Core
  • Owner set to johnjamesjacoby
  • Status changed from new to assigned

Assigning to myself to go along with component API for 1.3.

#5 @boonebgorges
8 years ago

  • Milestone changed from Future Release to 2.0.1
  • Priority changed from minor to normal
  • Severity set to normal

I am sad that this ticket got lost for so many years. It is a great idea.

Doing this correctly will mean writing lots of unit tests for the existing template loops, as they are all based on each other in some way, but have all been modified in ways that'll make them ideosyncratic. That said, I think it's worth doing, both because it'll make development easier for devs, but because it'll result in the removal of a fairly large amount of redundant code in BP.

Let's see if we can make some progress here for 2.1.

#6 @imath
8 years ago

  • Milestone changed from 2.0.1 to 2.1

This is great! While working on Skeleton, i thought it would be nice to have a "generic" folder for custom components directory page with an index / generic-loop / generic-entry but this piece of the puzzle was missing. I think having this class would be awesome!

#7 @mercime
8 years ago

  • Cc mercijavier@… added

#8 @DJPaul
8 years ago

  • Milestone changed from 2.1 to 2.2

#9 @DJPaul
8 years ago

  • Milestone changed from 2.2 to 2.3

#10 @DJPaul
8 years ago

  • Component changed from Core to Theme / Template Parts

#11 @imath
7 years ago

Gave it a look today, there's a lot of loops, and some can use very specific logic.
For instance :

  • getting the groups is using 3 different functions in case of invite / single groups / or groups
  • most of the queries are building an array lookings like this array( 'items' => $items, 'total' => $total ) , most of the time the first argument is a plural form relating to the component eg: activities, groups, blogs, notifications, but members is not used in favor of users
  • while most of the loops are offering a consistent action when loop start and ends, notifications is specific eg: do_action( 'group_loop_start' ), do_action( 'member_loop_start' ), do_action( 'activity_loop_start' ) > not plural forms, but do_action( 'notifications_loop_start' ) > plural form

I think we should progressively add core loops but offer plugin devs an abstract class they can use in their plugins. Then we will be able to iterate and improve it thanks to plugin devs feedbacks.

1269.01.patch is a first example with the blogs component.
I've first built unit tests before editing the BP_Blogs_Template class
then tested it extending the BP_Template_Loop class

@imath
7 years ago

#12 @boonebgorges
7 years ago

imath - Thanks for your research and work on this!

It's a bit annoying (although not surprising) that BP's loops are inconsistent in the ways you've described. But, as you note, we shouldn't let that hold us up. Core subclasses like BP_Groups_Template can just override additional parent methods to maintain backward compatibility, and where necessary, we can break up some of the big methods so that the bits that get overridden are smaller.

A couple principles going forward:

  • In cases where you're overriding a non-abstract class - that is, a specific component's template loop has to override a BP_Template_Loop method for backward compatibility or because it's got to do something special - let's make sure that the docblock explains the reasoning.
  • As noted above, if we find that we have to override something in BP_Template_Loop, let's try to make it as narrow as possible. If that means breaking up a big method so that it calls some smaller methods, then let's do it.
  • I think we'll need to maintain methods like rewind_blogs(), just in case anyone is calling them directly. We can turn them into wrappers for rewind_items() etc, and if you want, we can throw deprecated function notices (though I don't care one way or another about that).
  • Let's make sure that the docblocks for the BP_Template_Loop hooks are really good. These will be dynamic hooks, so we want to make sure that they're documented thoroughly.

#13 @imath
7 years ago

you're welcome :)

In 1269.02.patch, i'm trying to include 3 of your points. Adding the groups component didn't require to break up the set_loop() function into smaller parts so far. So i've left it the way it was. I'll see for the other components if we need to.

Seems to also work fine for the groups component, added unit tests for each get_items() possibilities. Though it made me wonder why are we setting a sortby argument in the pagination 'add_args' as it's not previously set ?

@imath
7 years ago

Updated to r9260

#14 @imath
7 years ago

  • Keywords has-patch added

I've been working on adapting every component's template loops (except for the legacy forums one) so that they extend the BP_Template_Loop.

Unlike the previous patches, this time i'm separating unit tests from the patch, because i've used them as a base to check i wasn't breaking anything. So you'll find the unit tests in 1269.unittests.patch.

You'll see in 1269.03.patch that some components use a very specific logic for their loops (the group invite one for instance), but the more complicated was the xProfile one. Actually BP_XProfile_Data_Template is somehow doing 2 loops in one (groups and fields). So i've chosen to split them :

  • BP_XProfile_Data_Template is mainly taking care of profile field groups
  • a new class BP_XProfile_Fields_Template is looping fields.

Everything went fine doing the split, but a template issue forced me to keep a bunch of "fields" method in BP_XProfile_Data_Template to avoid breaking profile loops for BP-Default or standalone BuddyPress themes.
The problem is located in 3 templates :

  • src/bp-templates/bp-legacy/buddypress/members/register.php
  • src/bp-templates/bp-legacy/buddypress/members/single/profile/edit.php
  • src/bp-templates/bp-legacy/buddypress/members/single/settings/profile.php

In these templates, the profile loop is not checking if the group has fields using bp_profile_group_has_fields(), they directly do a while ( bp_profile_fields() ) : bp_the_profile_field(); . So i've edited each of these templates adding the 'has_fields' check as i actually need it to init the BP_XProfile_Fields_Template class.

I think it's ready for testing, and i'd love to have your feedbacks.

Last edited 7 years ago by imath (previous) (diff)

@imath
7 years ago

Unit tests to use before and after 1269.03.patch

@imath
7 years ago

The patch :)

#15 follow-up: @r-a-y
7 years ago

This looks really really great, imath!

I'm going to try and look through this in the next few days.

I'm kind of scared about plugins that might have extended a BP_X_Template class. I know BP Groups Hierarchy extends BP_Groups_Template, but there might be others.

#16 in reply to: ↑ 15 @imath
7 years ago

Thanks a lot r-a-y for your feedback, and i agree about:

I'm kind of scared about plugins that might have extended a BP_X_Template class. I know BP Groups Hierarchy extends BP_Groups_Template, but there might be others.

I'll run some tests on this plugin to see how we can be more confident about it :)

#17 @imath
7 years ago

  • Keywords needs-refresh added

Just applyed the patch, it's outdated and really needs to be refreshed.

#18 @boonebgorges
7 years ago

  • Milestone changed from 2.3 to Future Release

This would be super cool, but needs a champion.

#19 @DJPaul
7 years ago

  • Keywords has-patch removed

#20 @DJPaul
6 years ago

  • Component changed from Appearance - Template Parts to Templates

#21 @DJPaul
4 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/

#22 @DJPaul
4 years ago

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