PHP 7.2 warning in wp_kses_attr()

[andrei0x309]

The function: wp_kses_attr() outpus a warning on newer PHP 7.2.

this function:

<?php
function wp_kses_attr($element, $attr, $allowed_html, $allowed_protocols) {
        if ( ! is_array( $allowed_html ) )
                $allowed_html = wp_kses_allowed_html( $allowed_html );

        // Is there a closing XHTML slash at the end of the attributes?
        $xhtml_slash = '';
        if (preg_match('%\s*/\s*$%', $attr))
                $xhtml_slash = ' /';

        // Are any attributes allowed at all for this element?
        if ( ! isset( $allowed_html[ strtolower( $element ) ] ) || true === $allowed_html[ strtolower( $element ) ] || count( $allowed_html[ strtolower( $element ) ] ) == 0 ) {
                return "<$element$xhtml_slash>";
        }

        // Split it
        $attrarr = wp_kses_hair($attr, $allowed_protocols);

        // Go through $attrarr, and save the allowed attributes for this element
        // in $attr2
        $attr2 = '';
        foreach ( $attrarr as $arreach ) {
                if ( wp_kses_attr_check( $arreach['name'], $arreach['value'], $arreach['whole'], $arreach['vless'], $element, $allowed_html ) ) {
                        $attr2 .= ' '.$arreach['whole'];
                }
        }

        // Remove any "<" or ">" characters
        $attr2 = preg_replace('/[<>]/', '', $attr2);

        return "<$element$attr2$xhtml_slash>";
}

can be changed to this:

this function:

<?php
function wp_kses_attr($element, $attr, $allowed_html, $allowed_protocols) {
        if ( ! is_array( $allowed_html ) )
                $allowed_html = wp_kses_allowed_html( $allowed_html );

        // Is there a closing XHTML slash at the end of the attributes?
        $xhtml_slash = '';
        if (preg_match('%\s*/\s*$%', $attr))
                $xhtml_slash = ' /';

        // Are any attributes allowed at all for this element?
        if ( ! isset( $allowed_html[ strtolower( $element ) ] ) || true === $allowed_html[ strtolower( $element ) ] || empty( $allowed_html[ strtolower( $element ) ] ) ) {
                return "<$element$xhtml_slash>";
        }

        // Split it
        $attrarr = wp_kses_hair($attr, $allowed_protocols);

        // Go through $attrarr, and save the allowed attributes for this element
        // in $attr2
        $attr2 = '';
        foreach ( $attrarr as $arreach ) {
                if ( wp_kses_attr_check( $arreach['name'], $arreach['value'], $arreach['whole'], $arreach['vless'], $element, $allowed_html ) ) {
                        $attr2 .= ' '.$arreach['whole'];
                }
        }

        // Remove any "<" or ">" characters
        $attr2 = preg_replace('/[<>]/', '', $attr2);

        return "<$element$attr2$xhtml_slash>";
}

I have also forked today your git repo and write a change the file here is the link: ​https://github.com/andrei0x309/WordPress/commit/6d6d784481c3875f4da08f8655e6d0f5dfa78fe1

Thanks in advance.

[SergeyBiryukov]

Hi @andrei0x309, welcome to WordPress Trac! Thanks for the report.

To clarify, the warning is Warning: count(): Parameter must be an array or an object that implements Countable, and the suggested change is to replace count( $allowed_html[ strtolower( $element ) ] ) == 0 with empty( $allowed_html[ strtolower( $element ) ] ),

[SergeyBiryukov]

I think we can combine the empty() check with ! isset(), see 43312.patch​.

[andrei0x309]

Replying to SergeyBiryukov:
Thanks for replying, I've seen your patch and is perfect, it removes the warning.

[SergeyBiryukov]

Tickets are closed when a commit is made to WordPress core :)

[soulseekah]

Errors (false does not implement Countable) on PHP 7.2 without 43312.patch, passes with patch

[ocean90]

@andrei0x309 Is it correct that the value which had triggered the warning was false? If so, doesn't that mean that the element is actually not allowed? While 43312.patch​ makes sense, false isn't really a supported value.

[soulseekah]

Hmm, agreed, false should not really be supported, the default $allowedtags structure lists elements with no supported attributes with an empty array (array()). What's the use case here @andrei0x309, how did you find out about this warning? Is core code calling this incorrectly?

Perhaps we should scan the plugin directory, @SergeyBiryukov for the incorrect usage/structure of $allowedtags, etc.

[andrei0x309]

Sorry guys I did a double check, @ocean90 is right, this warning was triggered when wp_kses was used with the unsupported false value.

At the time when I had reported the issue I've disabled all plugins, but now after I took a more deep look I have seen that the problem was because the theme I have used called wp_kess like this:

<?php
wp_kses( __('--SOME HTML HERE--', 'theme' ), array( 'strong' => '' ))

Which should have been:

<?php
wp_kses( __('--SOME HTML HERE--', 'theme' ), array( 'strong' => array() ))

I don't know how many developers pass an unsupported value since before PHP 7.2 it was not an issue.

Thanks, and sorry for the ticket.

[SergeyBiryukov]

Even though false is not supported, I don't think it should throw a warning.

Some sites currently ​display this warning in public.

[soulseekah]

Agreed, if there's incorrect usage out there, we should adapt and make the extra check. The patch and the tests are good to go if you guys would like to merge them.

[SergeyBiryukov]

In 42860:

Formatting: Avoid a PHP 7.2 warning in wp_kses_attr() when one of $allowedtags elements is an uncountable value.

Props andrei0x309, soulseekah, SergeyBiryukov.
Fixes #43312.

[SergeyBiryukov]

In 42861:

Formatting: Avoid a PHP 7.2 warning in wp_kses_attr() when one of $allowedtags elements is an uncountable value.

Props andrei0x309, soulseekah, SergeyBiryukov.
Merges [42860] to the 4.9 branch.
Fixes #43312.

[apokalyptik]

In r42861 the new conditional logic is not equivalent to the old conditional logic.

  $ php -v
    PHP 7.0.27-0+deb9u1 (cli) (built: Jan  5 2018 13:51:52) ( NTS )

  $ cat test.php
    <?php
    $element = 'foo';
    $allowed_html = array( 'foo' => false );
    var_dump( ( ! isset( $allowed_html[ strtolower( $element ) ] ) || true === $allowed_html[ strtolower( $element ) ] || count( 
    $allowed_html[ strtolower( $element ) ] ) == 0 ) );
    $element_low = strtolower( $element );
    var_dump( ( empty( $allowed_html[ $element_low ] ) || true === $allowed_html[ $element_low ] ) );

  $ php ./test.php
    bool(false)
    bool(true)

as you can see above empty() returns true in places where isset() does not.

ironically this use of empty covers the omission of the empty array case. but once that is fixed you'll want to check if it's an array and if so then count and check to keep the logic consistent.

if the logic needs to be changed for what is and is not supported, then that's fine but kses is pretty important and so it shouldn't be hidden in a ticket about fixing warnings (which to me implies that it's about the warning and not the logic/functionality and therefor tends to get less scrutiny)

[soulseekah]

@apokalyptik Oooh, thanks. What's test case here? Why haven't one of the many possible test failures been triggered?

[apokalyptik]

@soulseekah I was simply asked to review the patch, and noted the difference on visual inspection of the diff. All I know is that the diff changes the logic of the code being touched: That given the same specific input the output of the code block differs pre and post patch.

[ocean90]

@apokalyptik What are you proposing here? Just renaming the ticket to something else? Unless I'm missing something, the output is the same. Mostly because wp_kses_attr_check() previously returned false in this case.

[apokalyptik]

I'm proposing that a patch pushed through on this ticked should keep the logic the same, piece for piece, and fix the php 7.2 warnings. If you think kses needs to function differently in some way... you should make another ticket and it should be addressed there... even the possibility of subtly breaking kses is an unacceptable risk when including a patch that ostensibly only silences an error.

[ocean90]

I still don't see how it's function differently now? Could you explain this with a real world use case please?

[soulseekah]

counting a boolean will always return 1. Be it true or false, doesn't matter. So the old logic is actually flawed. The wp_kses_attr did indeed not short-circuit in the first condition and fell through doing useless, extra work (wp_kses_hair, preg_replace). But if you look at the resulting string and what it is composed of, then you'll see that it is absolutely the same as in the case of short-circuiting.

return "<$element$attr2$xhtml_slash>"; is equal to return "<$element$xhtml_slash>";, $attr2 is an empty string of attributes, composed of $attrarr which is empty when false is passed.

Thus, the logic is absolutely identical, minus the extra work that nobody really cares about. If anything, WordPress became faster :)

[apokalyptik]

That is a breakdown I can get behind, the work for the proof needed to be done and it needed to be explicit and explained how this was still correct and functionally equivalent. Thanks very much :)

[SergeyBiryukov]

Right, while the logic of that particular piece has changed, the logic of the function is still the same. It just bails earlier, avoiding some extra work.

Thank you for raising the issue, it's good to have this documented.