Skip to:
Content

BuddyPress.org

Opened 11 years ago

Closed 10 years ago

Last modified 10 years ago

#5472 closed defect (bug) (fixed)

xprofile_insert_field -> type - 'option' not allowed?

Reported by: shanebp's profile shanebp Owned by: boonebgorges's profile boonebgorges
Milestone: 2.1 Priority: normal
Severity: normal Version: 1.9.2
Component: Extended Profile Keywords: needs-unit-tests
Cc: shane@…

Description

When using a script to populate xprofile fields, this works:

   // in a loop
   xprofile_insert_field( array(
       'field_group_id'    => 1,
       'parent_id'     => $some_id,
       'type'          => 'selectbox',
       'name'          => $some_name,
       'option_order'  => $i
    ));

But option_order is not set in the db.
Perhaps because 'type' is selectbox so option_order is ignored in
class BP_XProfile_Field->save() ?

But if 'type' => 'option', nothing is inserted.
Perhaps because it fails here ?

// in function xprofile_insert_field()
if ( !in_array( $type, (array) $bp->profile->field_types ) )

Populating xprofile selectboxes is a common chore.
Being able to set the 'option_order' is important.
I've been side-stepping it by using $wpdb->insert.
But I was going to do a plugin re djpaul's comment in this thread:
http://buddypress.org/support/topic/timestamps-in-members-timezone/#post-176014
So I probably should use the 'BP Approved' approach - which isn't possible unless I missing something.

Attachments (2)

5472.unit-test.diff (832 bytes) - added by boonebgorges 11 years ago.
5472.01.patch (1.6 KB) - added by boonebgorges 10 years ago.

Download all attachments as: .zip

Change History (27)

#1 @imath
11 years ago

Hi shanebp

I know DJPaul is working on profile API, you should check #5220 and test the last patch he posted to see if the improvements introduced is matching your need.

#2 @shanebp
11 years ago

  • Component changed from Core to XProfile

imath - thanks for the pointer. Ton of work being done in 5520!

Version 0, edited 11 years ago by shanebp (next)

#3 @shanebp
11 years ago

Tested against BP 2.0 beta.

Same issues persist:

xprofile_insert_field -> type does not allow 'option'

xprofile_insert_field -> option_order is ignored

#4 @boonebgorges
11 years ago

  • Milestone changed from Awaiting Review to 2.1

Thanks for the additional info, shanebp.

option_order is not set in the db.
Perhaps because 'type' is selectbox so option_order is ignored in
class BP_XProfile_Field->save() ?

Correct. It's saved only for children. I wouldn't classify this as a bug - the correct field type for your use case is not 'selectbox' but is instead 'option'.

But if 'type' => 'option', nothing is inserted.

Perhaps because it fails here ?

Yup, this diagnosis is correct. Either we should add 'option' to the permitted field types, or we should make a special exception in xprofile_insert_field() for 'option'. Moving to 2.1 to solve it one way or another. I'm also attaching a unit test that demonstrates the failure.

#5 @DJPaul
10 years ago

  • Keywords dev-feedback added

Considering BP_XProfile_Field->save() looks in POST for option values, and that this approach is pretty much built around our current UI for adding/remove fields, rather than being more general and abstracted, I think adding option as a permitted field type is the best way forward.

If agreed, this should be a quick change to make, since Boone's already written the unit test. :)

#6 @boonebgorges
10 years ago

Thanks, DJPaul. Only tricky bit is that we now define our field_types whitelist by looking at the registered BP_XProfile_Field_Type classes. I don't think it makes sense to have one of these for 'option', since it's not really a field type. 5472.01.patch seems like a reasonable place to make the manual modification to the whitelist. An alternative is to change the relevant check in xprofile_insert_field() to

if ( !in_array( $type, (array) $bp->profile->field_types ) && 'option' !== $type )

This latter technique is more conservative, but it might also fail to address other edge cases for programatically creating fields. DJPaul, I'd like your feedback on this before going forward.

#7 @shanebp
10 years ago

The patch works for me with BP 2.0.1

Here's an insert Countries selectbox function:
https://gist.github.com/shanebp/119ffd879acc56c324ab

#8 @DJPaul
10 years ago

Boone, I agree with your patch. Code looks like it will fallback and use the BP_XProfile_Field_Type_Placeholder class type internally where we need to use a validation option, so I think it will be OK.

#9 @boonebgorges
10 years ago

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

In 8518:

Add 'option' to the whitelist of xprofile field types

The 'option' field type is not full-fledged, in the sense that it has an
associated BP_XProfile_Field_Type class; it is used only to save the values
that are child options of type 'radio', 'select', 'multiselect' and other
option-supporting field types. It is important that 'option' thus be added
manually to the field type whitelist, so that option fields can be created
programatically.

Fixes #5472

#10 @shanebp
10 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Found an odd issue...

When using xprofile_insert_field to create options, I guess we need to use 'field_order' instead of 'option_order' because
if ( $this->type_obj->supports_options ) {
will fail for field_types[] = 'option'; ... ?

This script works fine the first time
https://gist.github.com/shanebp/119ffd879acc56c324ab

But if you delete the Country field and then run the script again, the display order does not follow the field_order. On the second run, all the database fields are exactly the same, except for the 'id' field which is auto-incremented.

If I delete the Country field ( via page=bp-profile-setup ) and then manually set the auto-increment value for the bp_xprofile_fields table to the next unused id, and then run the script, the display order follows the field_order. I don't understand why that is.

Perhaps this request has opened a can of worms.
And it might be best to roll-back the change-set.
And for programatically inserting fields, use xprofile_insert_field to create the type and $wpdb->insert to create the options.

Last edited 10 years ago by shanebp (previous) (diff)

#11 @boonebgorges
10 years ago

  • Keywords needs-unit-tests added; dev-feedback removed

Thanks for the feedback, shanebp.

And for programatically inserting fields, use xprofile_insert_field to create the type and $wpdb->insert to create the options.

No, this is unacceptable. The fact that it's not possible to do what you're asking is a failure of our API.

I'm afraid I don't really understand what you're describing with the autoincrement stuff. What we really need here is unit tests that demonstrate the failure, so we'll have something to build against.

#12 @shanebp
10 years ago

I don't really understand what you're describing with the autoincrement stuff.

Say the countries script inserts 200 countries.
And say the database row, for the field type = selectbox, has an id of 10.
Then the last countries option row id would be 210.
And the display order of the options follows the field_order.

Then delete the Country field, via page=bp-profile-setup, and run the countries script again.
The database row, for the field type = selectbox, now has an id of 211.
Then the last countries option row id would be 411.
But the display order of the options does not follow the field_order.

If I delete the Country field again, and manually change the database bp_xprofile_fields table's auto-increment value to 10 and run the script again, the the display order of the options will follow the field_order.

Not sure if this matters, but I notice in your field test, you are using this to create the parent:
$parent = $this->factory->xprofile_field->create
Instead of xprofile_insert_field

If you change your unit test to:

  • use a loop to insert a handful of options and increment the field order
  • delete the parent and all the options
  • run the insert parent and options again

The test will not fail, but the display order will not ( afaik ) follow the field_order.

I sorry, but I'm not sure how to write a unit test that reveals if display order honors the field_order.

Last edited 10 years ago by shanebp (previous) (diff)

#13 @shanebp
10 years ago

A gut feeling says this issue is connected to having to use 'field_order' rather than 'option_order' when using a loop to add options to a Parent.

#14 @shanebp
10 years ago

Or maybe there is a cache that needs busting when programatically inserting fields ?

#15 @boonebgorges
10 years ago

A gut feeling says this issue is connected to having to use 'field_order' rather than 'option_order' when using a loop to add options to a Parent.

Yes. https://buddypress.trac.wordpress.org/browser/tags/2.0.1/bp-xprofile/bp-xprofile-classes.php#L729 Children are fetched by option_order. field_order has nothing to do with it.

if ( $this->type_obj->supports_options ) {

I guess you're referring to BP_XProfile_Field::save()? I'm not sure how that would affect this particular issue.

I tested your script using option_order instead of field_order when inserting country names, and it appears to work fine. Have you tested this?

#16 @shanebp
10 years ago

I tested your script using option_order instead of field_order when inserting country names, and it appears to work fine. Have you tested this?

Yes. The option_order value for all rows is 0.
I believe due to BP_XProfile_Field::save() check re supports_options

But this works:

  • set the Parent 'order_by' to 'custom'
  • use array_reverse in the foreach options loop
  • don't bother with field_order or option_order in the loop

Then the display order is preserved even if you delete the whole field and run the script again.

I've updated the gist:
https://gist.github.com/shanebp/119ffd879acc56c324ab

#17 @boonebgorges
10 years ago

I believe due to BP_XProfile_Field::save() check re supports_options

Can you please clarify what you mean by this? In the case of the Country field, the field type *does* support options. And in the case of the individual country 'option' fields, the logic in the supports_options block is not relevant anyway. Or maybe I'm missing something? Can you please restate the exact steps that need to be taken to reproduce the bug that you're seeing, along with a restatement of what the bug is? (what you think you should be seeing, and exactly what you are seeing, and where - eg, URLs)

#18 @shanebp
10 years ago

Can you please clarify what you mean by this?

Sorry, but I'm just guessing as to why the option_order field is saved as 0 for every row when inserting options in a loop.

	$i = 0; 
			
	foreach (  $countries as $country ) {
				
		xprofile_insert_field( array(
			'field_group_id'	=> 1,
			'parent_id'		=> $country_list_id,
			'type'			=> 'option',
			'name'			=> $country,
			'option_order'          => $i 
		));
		$i++;
	}

Even if I hard-code 'option_order' to 123, it still gets inserted as 0.

#19 @shanebp
10 years ago

btw - I'm not using trunk.
Just the 5472.01.patch on BP: 2.0.1

#20 @boonebgorges
10 years ago

Sorry, but I'm just guessing as to why the option_order field is saved as 0 for every row when inserting options in a loop.

This is where I'm getting lost. When I put your code into bp-custom.php, and then visited ?page=bp-profile-setup, all options were created with the correct option order. Maybe I'm not attempting to reproduce the problem in the same way that you're doing it? Exact steps to reproduce would be very, very helpful.

btw - I'm not using trunk.
Just the 5472.01.patch on BP: 2.0.1

Can you try with trunk? We are trying to determine if there is a problem to fix for BP 2.1.

If it does turn out that there are discrepancies between what you're seeing and what I'm seeing when following the same steps, it's likely that there's something (a plugin of some sort?) on your installation that's causing issues. Again, exact steps to reproduce (including environment information) is of the essence here.

#21 @shanebp
10 years ago

Versions:

  • WP: 3.9.1
  • theme 2013
  • BP: trunk
  • PHP 5.4.28
  • MYSQL: 5.5.37

No other plugins.

This function is the only thing in bp-custom.php:

function bp_add_custom_country_list() {
 
  if ( !xprofile_get_field_id_from_name('Country') && 'bp-profile-setup' == $_GET['page'] ) {
 
		$country_list_args = array(
		       'field_group_id'  => 1,
		       'name'            => 'Country',
		       'description'	 => 'Please select your country',
		       'can_delete'      => false,
		       'field_order' 	 => 2,
		       'is_required'     => false,
		       'type'            => 'selectbox',
		       'order_by'	     => 'default'
 
		);
 
		$country_list_id = xprofile_insert_field( $country_list_args );
 
		if ( $country_list_id ) {
 
			$countries = array(
				"United States",			
				"Afghanistan",
				"Albania",
				"Algeria",
				"Andorra",
				"Angola"
			);
			
			$i = 0; 
			
			foreach (  $countries as $country ) {
				
				xprofile_insert_field( array(
					'field_group_id'	=> 1,
					'parent_id'		=> $country_list_id,
					'type'			=> 'option',
					'name'			=> $country,
					'option_order'   => $i
				));
				$i++;
				
			}
 
		}
	}
}
add_action('bp_init', 'bp_add_custom_country_list');
  • I navigate to wp-admin/users.php?page=bp-profile-setup
  • the script runs
  • for every row 'option_order' is zero ( the default value for that field)
  • if I hard-code 'option_order' to 123, it still gets saved as zero
  • for what it's worth: if I add 'field_order' => $i, the correct values are stored in the field_order column

#22 @shanebp
10 years ago

Here is a sql dump of the parent row and the option rows:
http://philopress.com/fuw_bp_xprofile_fields.sql

Last edited 10 years ago by shanebp (previous) (diff)

#23 @boonebgorges
10 years ago

I've verified that BP_XProfile_Field::save() was not recording option_order. This appears to be an oversight from when the feature was first introduced. I'm about to commit a unit test and a fix.

#24 @boonebgorges
10 years ago

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

In 8535:

Record 'option_order' when passed through xprofile_insert_field()

Previously, this value was not recorded in the main part of BP_XProfile_Field.
Instead, the requisite option_order checking only happened when a parent field
was being saved via the UI.

Fixes #5472

#25 @shanebp
10 years ago

Thanks Boone.

Note: See TracTickets for help on using tickets.