Make WordPress Core

Opened 11 years ago

Closed 9 years ago

#22781 closed defect (bug) (fixed)

setUserSetting cannot handle hyphens

Reported by: nacin's profile nacin Owned by: wonderboymusic's profile wonderboymusic
Milestone: 4.4 Priority: normal
Severity: normal Version: 2.7
Component: Options, Meta APIs Keywords: 3.6-early has-patch
Focuses: Cc:

Description

sanitize_key() can, setUserSetting should be able to as well. It's not that sanitize_key() plays a role, but it sets expectations for what is allowed in keys across core. Both keys and values should be able to accept a hyphen.

The post-thumbnail image size cannot be remembered, for example, if it is added to image_size_names_choose.

Attachments (2)

22781.diff (3.0 KB) - added by wonderboymusic 9 years ago.
22781.2.diff (1.3 KB) - added by kitchin 9 years ago.
Fix js, better php tests

Download all attachments as: .zip

Change History (11)

#1 @nacin
11 years ago

  • Keywords 3.6-early added

#2 @jond3r
11 years ago

  • Cc jond3r@… added

Note that setUserSetting() is a JavaScript function (in utils.js).

I also also found a similar PHP function: set_user_setting() (in option.php).
Maybe that one also needs a fix?

(I found it because at first my search filter was set to *.php, so I couldn't find setUserSetting.)

I also experienced the not remembering thing, by the way.

#3 @nacin
11 years ago

Yes, the entire API (which spans PHP and JS) will need fixing.

#4 @cbaldelomar
11 years ago

I just came across this bug in my plugin development. The bug occurs on line 147 of the file /wp-includes/js/utils.js

n = name.toString().replace(/[^A-Za-z0-9_]/, ''), v = value.toString().replace(/[^A-Za-z0-9_]/, '');

To fix this problem, we will need to add a hypen to the regular expression list when storing to variable {v}.

n = name.toString().replace(/[^A-Za-z0-9_]/, ''), v = value.toString().replace(/[^A-Za-z0-9_-]/, '');

It is also interesting to note that this javascript replace method will only replace the first occurrence of the string. So if I added an image size with the name "my-custom-image-size", the original line of code would return "mycustom-image-size". I do not think this would have been the developers intended purpose. This can be easily fixed by adding the 'g' flag in the replace function.

n = name.toString().replace(/[^A-Za-z0-9_]/g, ''), v = value.toString().replace(/[^A-Za-z0-9_-]/g, '');

I would be more than happy to submit my very first patch to the Wordpress core. However, I'm not sure if this needs to be discussed more as an acceptable solution. What do you all think?

#5 @nacin
10 years ago

  • Component changed from General to Options and Meta

#6 @wonderboymusic
9 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 4.4
  • Owner set to wonderboymusic
  • Status changed from new to assigned

@wonderboymusic
9 years ago

#7 @wonderboymusic
9 years ago

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

In 33840:

User Settings: allow dashes in get|set_user_setting() in PHP and get|setUserSetting() in JS.

Add unit tests - there were none. Mock set_user_setting() since it won't run due to headers_sent() being true.

Fixes #22781.

@kitchin
9 years ago

Fix js, better php tests

#8 @kitchin
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Do not know how to unit test the js.

Last edited 9 years ago by kitchin (previous) (diff)

#9 @wonderboymusic
9 years ago

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

In 33947:

After [33840], JS regex needs the g modifier.

Add another unit test case.

Props kitchin.
Fixes #22781.

Note: See TracTickets for help on using tickets.