search

CMB2 / CMB2-User-Select

Select a user to associate with your object
21 stars 10 forks source link

Remove hardcoded table prefix for _user_level meta key. #4

Closed sandrodz closed 7 years ago

sandrodz commented 7 years ago

I can see new input, but search/autocomplete list never comes up.

screen shot 2017-06-06 at 6 04 53 pm
tw2113 commented 7 years ago

Check your browser console for possible js errors that may be causing conflicts. Also PHP errors could be playing a part in preventing returned results from AJAX

sandrodz commented 7 years ago

@tw2113 there are no errors in console. Returned data is simply empty:

screen shot 2017-06-06 at 6 24 21 pm
sandrodz commented 7 years ago

@tw2113 debugged it, problem is that meta_query key _user_level doesn't use table prefix, its hardcoded.

screen shot 2017-06-06 at 6 48 43 pm
tw2113 commented 7 years ago

I'm looking at a pretty stock WP install for my local dev and wp_user_level is what WordPress core uses for the meta key for user levels. That's not a custom key that CMB2 set, for example.

However, if you had changed that in the past somehow, then that would be a contributing factor.

sandrodz commented 7 years ago

@tw2113 WordPress changes key name based on table prefix. So key should use prefix.

screen shot 2017-06-06 at 6 54 40 pm
tw2113 commented 7 years ago

ahhh. Good point.

With that said, so that you're not editing the plugin directly, you could use the filter right below it to safely modify the key to fetch.

$user_args = apply_filters( 'wds_cmb2_user_select_search_args', $user_args, $this );

sandrodz commented 7 years ago

I think simpler/cleaner solution would be to make plugin compatible with WordPress option of changing table name prefix. Security conscience users will bump into this. Using filter is not an option, I randomly generate table prefixes on new install.

sandrodz commented 7 years ago

This could be a simple fix:

screen shot 2017-06-06 at 7 10 42 pm
sandrodz commented 7 years ago

@tw2113 I can do PR if you'd like.

tw2113 commented 7 years ago

Just saying that it's technically possible to do it via the filter already provided. That said, PR it up, and we'll take it in.

jtsternberg commented 7 years ago

The user level thing needs to go away entirely. #3 is on the right path.

sandrodz commented 7 years ago

yep #3 looks great. Why was not it merged?

tw2113 commented 7 years ago

because it slipped through the cracks and no one had taken care of it yet.

sandrodz commented 7 years ago

Is there an ETA for that PR? I looked through the code, nothing particularly problematic jumps out. But I could be wrong.

tw2113 commented 7 years ago

Whenever someone gets around to it, to be honest. I'd appreciate some testing just in case, for what it's worth.

tw2113 commented 7 years ago

Merged

sandrodz commented 7 years ago

Readme needs to be updated as well.

sandrodz commented 7 years ago

Can you please also release a new version?

tw2113 commented 7 years ago

Good catch on the readme, that has been updated. Bumped/tagged.