Opened 14 years ago
Last modified 4 years ago
#16101 new defect (bug)
Numeric term fields are strings
Reported by: | foofy | Owned by: | |
---|---|---|---|
Milestone: | Priority: | normal | |
Severity: | normal | Version: | |
Component: | Taxonomy | Keywords: | needs-unit-tests has-patch |
Focuses: | Cc: |
Description
The numeric fields (term_id, parent, etc.) on term objects are strings. I only noticed this because term_exists() uses is_int() to determine if the $term value is an ID or slug.
Only ticket I could find about this is #5381.
sanitize_term() should fix this, but it bails early in the "raw" context. sanitize_term_field() sanitizes the numeric fields in every context. I don't see a reason for sanitize_term() to bail early so I made a patch that takes that out. The patch also adds some missing fields to the list of those to be sanitized and changes term_exists() to use is_numeric() (which will correctly identify strings containing only numbers) instead of is_int().
Attachments (2)
Change History (16)
#2
@
14 years ago
- Cc foofy@… added
I think it would be fine to leave that as is_int() since the problem goes away if the term objects are properly sanitized.
#3
follow-up:
↓ 4
@
14 years ago
- Keywords dev-feedback added
- Milestone changed from Awaiting Review to Future Release
Can use ctype_digit( $term )
without making changes to sanitize_term*()
.
Casting everything to (int) could change the output of XML-RPC requests which can break clients, so we need to be super careful there.
#4
in reply to:
↑ 3
@
14 years ago
Replying to mdawaffe:
Can use
ctype_digit( $term )
without making changes tosanitize_term*()
.
Nacin tells me some hosts disable Ctype, which is ridiculous (on the hosts' parts, not on Nacin's).
#5
@
12 years ago
- Cc ian_dunn@… added
- Keywords changed from has-patch dev-feedback to has-patch dev-feedback
This just bit me in the ass, because I took the term_id from get_terms() and passed it to get_term_link() as $term. get_term_link() assumed that $term was a slug because it failed is_int(), and since there's no term named "237", it returned a WP_Error().
It seems to me like casting the values to int in get_term*() is the proper fix, even if it requires patching/testing XML-RPC code. If there's a consensus behind that approach, I'll try to find time to write and test a new patch over the next couple weeks.
This ticket was mentioned in Slack in #core by boone. View the logs.
9 years ago
This ticket was mentioned in Slack in #docs by majemedia. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by howdy_mcgee. View the logs.
4 years ago
#14
@
4 years ago
- Keywords has-patch added; needs-patch removed
It seems to me that every instance of is_int()
in taxonomy.php is testing against passed input. I agree with Michael that we could use ctype_digit()
for all these instances. I suppose the only drawback is that term slugs specifically cannot be integers which seems like a fair trade for more flexible user input.
Timothy pointed out in slack that ctype_digit()
is already used in a few places already so the previous mention of host support may be moot.
The latest patch converts 6 instances of is_int()
to ctype_digit()
.
From the PHP manual on is_numeric():
This could cause subtle bugs.