humanmade/Custom-Meta-Boxes

Custom CMB inherits from Group CMB

Closed this issue · 9 comments

datnguyen-sc commented

With current change here. If we create a custom CMB by inherit Group CMB, it will not display data properly.

datnguyen-sc commented

@tareiking, @mikeselander: Can take a look a this please?

mikeselander commented

@datnguyen-sc I believe this may be correct - group CMBs handle data differently from other field types since the field is effectively a wrapper for other fields.

Can you provide a full array of fields and expected/occurring behavior and I can dig in a little more today?

datnguyen-sc commented

Hi @mikeselander: for built in CMB, it is working fine. However for a custom CMB like below, it will not work.

class Custom_Group extends \CMB_Group_Field {
	
	public function get_default_args() {
		return array_merge(
			parent::get_default_args(),
			array(
				'fields' => array(
					array(
						'id'   => 'key',
						'name' => esc_html__( 'Key', 'test' ),
						'type' => 'text',
					),
					array(
						'id'         => 'value',
						'name'       => esc_html__( 'Value', 'test' ),
						'type'       => 'text_area',
					),
				),
			)
		);
	}
}

If we create a new field with type = Custom_Group, things will break.

mikeselander commented

@datnguyen-sc what things break, specifically?

datnguyen-sc commented

@mikeselander: CMB will not displayed correctly since it is expecting an array values however it are passing a single value, since type === 'group' is failed.

mikeselander commented

OK @datnguyen-sc I've added a filter above the array mapping that allows you to modify which fields are considered to be group type fields and should have their data modified before pushing into the field class.

Can you try out https://github.com/humanmade/Custom-Meta-Boxes/pull/396/files and filter cmb_group_field_types and let me know if that helps you out?

datnguyen-sc commented

@mikeselander: That should solve the problem I beleive. Please let me know when it is merged.

mikeselander commented

@datnguyen-sc so you've tried it and it works?

This issue will automatically close when the PR is merged so you'll know by notification 👍

datnguyen-sc commented

@mikeselander: Yes, it's working for me

  • 👍1