sanitize_term_field missing default $context value

[dilipbheda]

As per the inline documentation of the sanitize_term_field() function, a default $context value should be passed in the function arguments.

[audrasjb]

Hello and thank you for the ticket.
I may be wrong but to be honest, given the default condition passes through a simple else statement, I cannot see much value to pass the default value. It is not used anywhere in the function.

[dilipbheda]

@audrasjb You're right, but I noticed that the documentation mentions a Default value, which implies that if the last parameter is not passed, it should default to the 'display' context.
Ref: ​https://tinyurl.com/22neh87o

Or we can remove the default value mentioned in the inline docs.

[Presskopp]

Defining the default $context = 'display' avoids errors if the function gets called without the $context parameter so it's making sense to me. For that reason it will be possible to also call the function with only the first 4 parameters, if needed. Also it is clearer in regards to the inline documentation.

[audrasjb]

You make a point @Presskopp. Let's add the default then.

Looks good to me :+1:

[rollybueno]

Hey @audrasjb, PR ​https://github.com/WordPress/wordpress-develop/pull/8670 only updates the $context to $context = 'display' as per https://core.trac.wordpress.org/ticket/63257#comment:4.

I believe this is ready for commit.

[welcher]

The was reviewed in the 6.9 bug scrub today. The linked PR has some failing unit tests. If we can get those passing before RC1 next week, this can probably be ready for commit.

[SirLouen]

I think it's mostly ready to be deployed. @Presskopp made a good point, but I would have preferred to see some code to reproduce any scenario affecting this, plus some unit testing that proves this is required, would do even better.

@dilipbheda could you bring your PR up to date with the trunk branch? This will ensure the patch applies cleanly and will also run the test suite again (which failed for some reason, but the logs are no longer available to understand why).

refreshing the patch along with phpcbf

original credits : @dilipbheda

Thanks for the PR. However, this PR seems to contain unnecessary changes.

Let me close this PR, and I'd like to move #10484 forward.

could you bring your PR up to date with the trunk branch?

I handled it.

[wildworks]

Just one quick question: if we add a default value, will it affect consumers who are already using the "term_{$field}" or "{$taxonomy}_{$field}" filters?

For example, consumers may be using hooks such as the following:

add_filter( 'term_name', function( $value, $term_id, $taxonomy, $context ) {
    if ( empty( $context ) ) {
        return 'something';
    }
    return $value;
}, 10, 4 );

[mindctrl]

@wildworks that's a great question and seems possible to me.

Given your question on the Trac ticket, I think we should punt this one and leave room for discussion and discovery.

[wildworks]

Just one quick question: if we add a default value, will it affect consumers who are already using the "term_{$field}" or "{$taxonomy}_{$field}" filters?

For example, consumers may be using hooks such as the following:

add_filter( 'term_name', function( $value, $term_id, $taxonomy, $context ) {
    if ( empty( $context ) ) {
        return 'something';
    }
    return $value;
}, 10, 4 );

As I understand it, when sanitize_term_field is called without a context, the $context value passed to the above filter changes, which affects backward compatibility.

I'm not sure why the get_term_field() and sanitize_term() functions have default values ​​while the sanitize_term_field() function doesn't, but further investigation is needed. Alternatively, it might be possible to close this as "wontfix".