Skip to:
Content

BuddyPress.org

Opened 13 years ago

Closed 13 years ago

#3447 closed defect (bug) (fixed)

Audit use of extract( $r, EXTR_SKIP )

Reported by: dontdream's profile dontdream Owned by:
Milestone: 1.5 Priority: normal
Severity: normal Version: 1.5
Component: Core Keywords:
Cc:

Description

In 'bp-xprofile-template.php', function bp_has_profile:

Only show empty fields if we're on the Dashboard, or we're on a user's profile edit page, or this is a registration page

The problem is that a profile search plugin I wrote for the Members page, BP Profile Search, needs to show also empty fields in its search form, so this is a blocker for my plugin.

Could you please fix this? Thanks!

Change History (12)

#1 follow-up: @boonebgorges
13 years ago

  • Severity changed from blocker to normal
  • Type changed from defect to enhancement

If by "fix this" you mean revert the change, then no :) It was done for a reason, which has to do with zebra striping as well as some performance issues.

We introduced a new parameter in bp_has_profile(), 'show_empty_fields'. If you need to show those fields, then pass this parameter with the value true.

If this doesn't work, please share a bit more information about how your plugin works. It's possible that we could put a filter in there to help.

#2 in reply to: ↑ 1 @dontdream
13 years ago

Thanks for your reply.

I found the 'hide_empty_fields' parameter in bp_has_profile(), but it doesn't work because it's overridden inside the function, effectively ignoring the value received by the caller.

#3 @boonebgorges
13 years ago

That shouldn't be the case. That test at the beginning of bp_has_profile() http://buddypress.trac.wordpress.org/browser/trunk/bp-xprofile/bp-xprofile-template.php#L156 only sets the *default* value for show_empty_fields. In other words, that logic only matters if you *don't* pass an explicit show_empty_fields value. Whatever you pass to the function will override this default.

Give it a try and let me know if it's successful.

#4 @DJPaul
13 years ago

Boone means hide_empty_fields instead of show_empty_fields, I'm sure :)
Also there's hide_empty_groups which you may also want to try setting, too.

#5 follow-up: @boonebgorges
13 years ago

er, yeah - what DJPaul said :)

#6 in reply to: ↑ 5 @dontdream
13 years ago

I found why bp_has_profile() ignores the hide_empty_fields parameter it receives, that's because inside the function it uses:

extract( $r, EXTR_SKIP );

instead of:

extract( $r, EXTR_OVERWRITE );

Since $hide_empty_fields already exists, it is not extracted.

Sorry I didn't realize this before.

Last edited 13 years ago by dontdream (previous) (diff)

#7 @DJPaul
13 years ago

Good catch! We have a lot of EXTR_SKIP in BP. We should audit them all.

#8 @boonebgorges
13 years ago

  • Component changed from XProfile to Core
  • Milestone changed from Awaiting Review to 1.5
  • Summary changed from new 'hide empty profile fields' feature to Audit use of extract( $r, EXTR_SKIP )
  • Type changed from enhancement to defect

Yes, good catch! To be extra safe, we should probably use EXTR_OVERWRITE in addition to different variable names when setting up function defaults, eg $hide_empty_fields_default

#9 @DJPaul
13 years ago

I'm not sure how useful changing the default variable names would be. Also, as overwrite is the default for the extract function, I think this should be a relatively simple change -- I'll take a look tomorrow unless i'm beaten to it

#10 @boonebgorges
13 years ago

When you set the default with a value like $hide_empty_fields, and then use extract( $r, EXTR_SKIP );, $rhide_empty_fields? will not be extracted, but $hide_empty_fields will retain its previous value. If we leave out EXTR_SKIP, it's irrelevant - but in any case it seems like bad practice to use the same variable name, in the same function scope, for two different purposes.

Version 0, edited 13 years ago by boonebgorges (next)

#11 @DJPaul
13 years ago

More I think about this, the issue is the default name collision as Boone's eloquently described, rather than SKIP vs OVERWRITE. Working on patch.

#12 @djpaul
13 years ago

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

(In [4958]) Correct handling of default value of the profile field loop's hide_empty_fields.
Also correct potential PHP warnings.
Fixes #3447

Note: See TracTickets for help on using tickets.