Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

#7149 closed defect (bug) (fixed)

Templates: locating empty array of template(s) errors undefined variable

Reported by: Offereins Owned by: Offereins
Milestone: 2.8 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch commit
Cc: lmoffereins@…

Description

In order to prevent loading a specific template part, in my case the 'activity/post-form' (a brilliant thought of mine to make the activity stream contain only actual site activity), I empty the array of templates to locate through filtering bp_get_template_part.

Thus follows sending an empty array into bp_locate_template(). However, here the array will be looped, creating new variables in the loop that do not exist outside of it. Beyond the loop these variables ($template_name being the first) are then used, but since the loop was never run, the variable appears to be undefined.

What I expect is that the template loading dies silently when there appear to be no templates requested.

To fix this, I suggest to only run bp_locate_template() when the list of templates is non-empty. Patch is attached.

Attachments (4)

7149.patch (597 bytes) - added by Offereins 3 years ago.
7149.01.patch (595 bytes) - added by Offereins 3 years ago.
7149.02.patch (600 bytes) - added by Offereins 3 years ago.
Please ignore 7149.01.patch
7149.03.patch (603 bytes) - added by Offereins 3 years ago.
Return false, not an empty string

Download all attachments as: .zip

Change History (14)

@Offereins
3 years ago

#1 @Offereins
3 years ago

  • Component changed from Templates to Core

#2 @DJPaul
3 years ago

  • Keywords reporter-feedback added
  • Milestone changed from Awaiting Review to 2.7

Easily re-produced:

wp> bp_locate_template(array())
PHP Notice: Undefined variable: template_name in /srv/www/buddypress.dev/src/wp-content/plugins/buddypress/src/bp-core/bp-core-template-loader.php on line 138

@Offereins Thanks for the report and patch. How do you feel about making this function return an empty string if a template can't be found? If so, let's put a since tag on the PHPDoc explaining the return type change.

I know it returns a bool already, but I think that's just bad function design. An empty string is a false-y value, so it'd probably won't break anything unless someone's doing a type-strict check on the output of bp_locate_template. We/you would need to check the instances of this function in BuddyPress core to check if this breaks anything.

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


3 years ago

#4 @Offereins
3 years ago

  • Keywords reporter-feedback removed

@DJPaul Thanks for confirming the bug. In reply to your suggestion: returning a false-y string is okay with me. Most return value evaluations of the function are loosely compared, but I've found the following strict-type checks for output of bp_locate_template():

  • bp-core/bp-core-template-loader.php @ line 175 in bp_locate_template_asset()
  • bp-groups/classes/class-bp-group-extension.php @ line 974 in BP_Group_Extension::setup_edit_hooks() (compares empty string)

Furthermore, bp_locate_template() constructs the return value for:

  • bp_get_template_part() of which no value evaluations are found, but is widely used
  • bp_displayed_user_get_front_template() which has a strict type check in:
    • bp-members/bp-members-template.php @ line 1379 in bp_displayed_user_front_template_part()
  • bp_groups_get_front_template() which has a strict type check in:
    • bp-groups/bp-groups-template.php @ line 4189 in bp_groups_front_template_part()

My conclusion is that this propagates to somewhat more functionality than you'd originally guess.

Last edited 3 years ago by Offereins (previous) (diff)

#5 @mercime
3 years ago

  • Milestone changed from 2.7 to Future Release

Sorry, have to move this to future release.

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


3 years ago

#7 @DJPaul
3 years ago

  • Milestone changed from Future Release to 2.8

OK, let's just get this patch in for 2.8, @Offereins. We'll need to add the { } brackets, but that's just a minor tweak.

@Offereins
3 years ago

@Offereins
3 years ago

Please ignore 7149.01.patch

@Offereins
3 years ago

Return false, not an empty string

#8 @Offereins
3 years ago

@DJPaul See attached, 7149.03.patch is tha one.

#9 @DJPaul
3 years ago

  • Keywords commit added
  • Owner set to Offereins
  • Status changed from new to assigned

This looks good, @Offereins! I'll let you commit it once your access comes through.

#10 @offereins
3 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 11267:

Stop locating an empty set of templates.

Bail early in bp_locate_template() when there are no files to look for.
This prevents PHP from showing errors when an empty array is provided. Use
case for this is when by hooking into the bp_get_template_part filter
you'd like to prevent the locating of a template.

Fixes #7149.

Note: See TracTickets for help on using tickets.