`$wpdb->prepare` returns blank without notice if less number of arguments are passed. should give a notice or exception

[thekt12]

1.A Right Case:

<?php
$wpdb->prepare("select * from wp_posts where ID=%d and post_type=%s", array("1","post"))

Returns:
select * from wp_posts where ID=1 and post_type='post'
which is right

2.A Right Case:

<?php
$wpdb->prepare("select * from wp_posts where ID=%d and post_type=%s",1,"post")

Returns:
select * from wp_posts where ID=1 and post_type='post'
which is also right

3.A Semi Right Case:

<?php
$wpdb->prepare("select * from wp_posts where ID=%d and post_type=%s", array("1","post","xyz"))

Returns:
select * from wp_posts where ID=1 and post_type='post'
understood it ignore extra attributes(but could give a notice- i do understand that it is hard to find out in this case)

But :

3.A Wrong Case (Case with less argument passed in ) :

$wpdb->prepare("select * from wp_posts where ID=%d and post_type=%s", array("1"))

Returns:
blank
It should either give a notice or an exception. If it returns blank, it actually hides the actual problem.

How serious is this issue?

Think of the following snippet:

<?php
//this will return a blank without exception
$statement = $wpdb->prepare("select * from wp_posts where ID=%d and post_type=%s", array("1"));

// This will also return blank if blank string is passed in
$results = $wpdb->get_results($statement);

/* so the developer assumes that the result for this query is blank
 instead of the fact that this is caused due to a wrong number of argument that he has passed in. 
This is bound to happen in a large query.  
*/

I am yet to figure out the actual way but following code (though not fool proof) will work:

<?php
public function prepare($query, $args)
    {
        if (is_null($query)) {
            return;
        }

        // This is not meant to be foolproof -- but it will catch obviously incorrect usage.
        if (strpos($query, '%') === false) {
            _doing_it_wrong('wpdb::prepare', sprintf(__('The query argument of %s must have a placeholder.'), 'wpdb::prepare()'), '3.9.0');
        }

        $args = func_get_args();
        array_shift($args);

        // If args were passed as an array (as in vsprintf), move them up
        if (is_array($args[0]) && count($args) == 1) {
            $args = $args[0];
        }

        foreach ($args as $arg) {
            if (! is_scalar($arg) && ! is_null($arg)) {
                _doing_it_wrong('wpdb::prepare', sprintf('Unsupported value type (%s).', gettype($arg)), '4.8.2');
            }
        }

        $query = str_replace("'%s'", '%s', $query); // in case someone mistakenly already singlequoted it
        $query = str_replace('"%s"', '%s', $query); // doublequote unquoting
        $query = preg_replace('|(?<!%)%f|', '%F', $query); // Force floats to be locale unaware
        $query = preg_replace('|(?<!%)%s|', "'%s'", $query); // quote the strings, avoiding escaped strings like %%s
        $query = preg_replace('/%(?:%|$|([^dsF]))/', '%%\\1', $query); // escape any unescaped percents
        array_walk($args, array( $this, 'escape_by_ref' ));


// my modification to wp-includes/wp-db.php function prepare()
        $statement = @vsprintf($query, $args);

        if(empty($statement) && !empty($query)) 
        {
            _doing_it_wrong('wpdb::prepare', "Improper prepare statemnt. Check the number of argument passed in" , '4.*.*');
        }
             

        return $statement;
    }

will try to provied a better solution for this

[thekt12]

Patch For the issue

[thekt12]

I am not able to figure out how to write a unit test if a notices ic called or not :/
I have added some test cases to the core prepare statement and where I am testing the return values but how to test if the notices function is called or not?

[jdgrimes]

@thekt12 For testing the notices, you can use the @expectedIncorrectUsage annotation. Examples: ​https://codesymphony.co/wordpress-unit-test-suite-introduces-expectedincorrectusage/

[thekt12]

Test Cases

[thekt12]

Proper Test File

[thekt12]

Actuall File

[thekt12]

@swissspidy test42040-new.2.diff is the actual test file. If possible please remove other test files as I have accidentally uploaded changes made to the config file.

@jdgrimes thanks for the share it was the one i really needed, but I have found a new bug in that testing @expectedIncorrectUsage usage.

Let me explain

Case 1:

<?php

            /**
             * @ticket 42040
             * @expectedIncorrectUsage wpdb::prepare
             */
            public function test_prepare_invalid_args_count()
            {
                global $wpdb;

                // this assertion fails as it dose not returns notice
                $prepared = @$wpdb->prepare("SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s", 1, "admin", "extra-arg"); 
                $this->assertEquals("SELECT * FROM $wpdb->users WHERE id = 1 AND user_login = 'admin'", $prepared);

                // This returns argument count mismatch notice
                $prepared = @$wpdb->prepare("SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s", 1, "admin", "extra-arg"); 
                $this->assertEquals("SELECT * FROM $wpdb->users WHERE id = 1 AND user_login = 'admin'", $prepared);

             }

The above case returns

FAILURES!
Tests: 1, Assertions: 3, Failures: 1.

Which is correct

Case 2:

<?php

            /**
             * @ticket 42040
             * @expectedIncorrectUsage wpdb::prepare
             */
            public function test_prepare_invalid_args_count()
            {
                global $wpdb;

                // this assertion fails as it dose not returns notice
                $prepared = @$wpdb->prepare("SELECT * FROM $wpdb->users WHERE id = %d AND user_login = %s", 1, "admin", "extra-arg"); 
                $this->assertEquals("SELECT * FROM $wpdb->users WHERE id = 1 AND user_login = 'admin'", $prepared);

                // This gives two notices, one - due to the use of array as parameter and second due to argument mismatch 
                $prepared = @$wpdb->prepare("SELECT * FROM $wpdb->users WHERE id = %d and user_nicename = %s and user_status =%d and user_login = %s", array( 1 ), "admin", 0); 
                $this->assertEquals("", $prepared);

             }

OK (1 test, 3 assertions)

Whereas it should actually give me

FAILURES!
Tests: 1, Assertions: 3, Failures: 1.

@expectedIncorrectUsage is only checking the number of times _doing_it_wrong is called from the test function and matches it to the number of assertion in that unit test function(also valid if the number of assertion is less than the call to _doing_it_wrong).
It should be actually testing if an assertion is producing a notice or not and number of notices produced by an assertion.

I'll create a new ticket for this bug.

[thekt12]

Patch v2 --> Replaced == with strcmp

[thekt12]

One assertion per test case.

[thekt12]

Added new patch for this ticket. (42040-patch-v2.diff).

Also, asper the discussion in #42045 , i have change my test cases to one assertion per test.

(42040-one-assertion-per-test-testcases-v1.diff)

[pento]

In 41662:

Database: Throw a notice if wpdb::prepare() is called with an incorrect number of arguments

wpdb::prepare() currently gives no information if the number of arguments passed doesn't match the number of placeholders in the query. This change gives an explicit notice that the call was incorrect.

Also fixes an enrelated term meta test that was triggering this new notice.

Props thekt12 for the initial patch.
Fixes #42040.

[pento]

Thank you for your work on this bug, @thekt12!

I've reworked your patch a bit, (with 4.9 coming up, I wanted to get it in as quickly as possible).

The major changes I made were:

• Change the preg_match_all() to only match valid placeholders, to avoid the counting loop.
• Pass the error message through __(), and reword it so that we don't need plural variations.
• Merge the individual tests into a single test with a dataProvider.

[pento]

In 41663:

Database: Fix some PHP errors introduced in [41662].

PHP < 5.4 requires a $matches parameter to be passed to preg_match_all()

wpdb::prepare() can be called before translations are loaded, so needs appropriate wp_load_translations_early() calls.

See #42040.

[thekt12]

Got to learn a lot of new things from your changes. Will keep all this in mind while writing a path. And grouping the test cases using dataProvider was definitely a new knowledge for me. Thanks, @pento and good to know that this bug will probably be fixed in v4.9.