WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.55k stars 482 forks source link

get_categories `type => link` argument is deprecated #1845

Open dingo-d opened 4 years ago

dingo-d commented 4 years ago

Not 100% sure should this go in the DeprecatedParametersSniff or DeprecatedParameterValuesSniff. Basically the get_categories() function has a deprecated argument

array( 'type' => 'link')

Which, if used, will throw a notice to use

array( 'taxonomy' => 'link_category')

instead.

I think this should be added in one of those two but I'm not sure which one tbh 😬

I'm happy to make a PR 🙂 Since I'm making a core patch to change the docblock of that function as well.

jrfnl commented 4 years ago

Sounds like one for the DeprecatedParameterValues sniff as it's about the value of the parameter, not the parameter itself.

dingo-d commented 4 years ago

I think the sniff should be modified a bit to include the part where the parameter to check is a key in the array. From what I've seen in the sniff currently, it only handles where the parameter is a certain value.

So setting

'get_categories' => array(
    1 => array(
        'type' => array(
            'alt'     => 'the "link" type argument',
            'version' => '3.0.0',
        ),
    ),
),

in the $target_functions won't work, as it's interpreted as get_categories( 'type' ). I'll see if I can work on it today if it's ok 🙂

jrfnl commented 4 years ago

You may even want to split the logic for array functions off into a separate function and have a flag in the functions list property to say that something is an array and needs the "other" set of logic.

I'll see if I can work on it today if it's ok slightly_smiling_face

Of course :+1:

dingo-d commented 4 years ago

You may even want to split the logic for array functions off into a separate function and have a flag in the functions list property to say that something is an array and needs the "other" set of logic.

This is a good tip because otherwise, I'd have to change that property quite a bit.

dingo-d commented 4 years ago

Sigh... I've spent 3 hours trying to figure out how to check if the argument parameter is an array, and then how to check each parameter and if one of it is type and if its value is link, without luck.

I've organized the $target_functions like

...
'get_bloginfo' => array(
    'type' => 'string',
    'position' => 1,
    'values' => array(
        'home' => array(
            'alt'     => 'the "url" argument',
            'version' => '2.2.0',
        ),
        'siteurl' => array(
            'alt'     => 'the "url" argument',
            'version' => '2.2.0',
        ),
        'text_direction' => array(
            'alt'     => 'is_rtl()',
            'version' => '2.2.0',
        ),
    ),
),
'get_categories' => array(
    'type'     => 'array',
    'position' => 1,
    'values'   => array(
        'type' => array(
            'alt'     => 'the "link" type argument',
            'version' => '3.0.0',
        ),
    ),
),
...

so it's easier to distinguish the type. Then I've changed the process_parameters method to:

public function process_parameters( $stackPtr, $group_name, $matched_content, $parameters ) {
    $this->get_wp_version_from_cl();
    $param_count = \count( $parameters );

    foreach ( $this->target_functions[ $matched_content ] as $parameter_value ) {
        $current  = $this->target_functions[ $matched_content ];
        $position = $current['position'];
        // Stop if the position is higher then the total number of parameters.
        if ( $position > $param_count ) {
            break;
        }

        // If parameter type is string, process it and check it. If it's array we need to check every argument key.
        if ( 'string' === $current['type'] ) {
            $this->process_parameter( $matched_content, $parameters[ $position ], $current['values'] );
        } else {
            // Determine if the parameter is indeed an array, bail if not.

        }
    }
}

and I got stuck 😞