WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#42040 closed defect (bug) (fixed)

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

Reported by: thekt12 Owned by: pento
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.8.2
Component: Database Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

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

Attachments (6)

wp-db.diff (1.6 KB) - added by thekt12 4 years ago.
Patch For the issue
test42040.diff (77.3 KB) - added by thekt12 4 years ago.
Test Cases
test42040-new.diff (4.2 KB) - added by thekt12 4 years ago.
Proper Test File
test42040-new.2.diff (3.2 KB) - added by thekt12 4 years ago.
Actuall File
42040-patch-v2.diff (1.6 KB) - added by thekt12 4 years ago.
Patch v2 --> Replaced == with strcmp
42040-one-assertion-per-test-testcases-v1.diff (4.1 KB) - added by thekt12 4 years ago.
One assertion per test case.

Download all attachments as: .zip

Change History (19)

#1 @thekt12
4 years ago

  • Severity changed from normal to major

#2 @swissspidy
4 years ago

  • Component changed from Query to Database
  • Severity changed from major to normal

@thekt12
4 years ago

Patch For the issue

#3 @thekt12
4 years ago

  • Keywords has-patch added

#4 @swissspidy
4 years ago

  • Keywords needs-unit-tests added

#5 @thekt12
4 years ago

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?

#6 @jdgrimes
4 years ago

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

@thekt12
4 years ago

Test Cases

@thekt12
4 years ago

Proper Test File

@thekt12
4 years ago

Actuall File

#7 @thekt12
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@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
4 years ago

Patch v2 --> Replaced == with strcmp

@thekt12
4 years ago

One assertion per test case.

#8 @thekt12
4 years ago

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)

#9 @pento
4 years ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to pento
  • Status changed from new to assigned

#10 @pento
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#11 @pento
4 years ago

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.

#12 @pento
4 years ago

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.

#13 @thekt12
4 years ago

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.

Note: See TracTickets for help on using tickets.