Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 141 - Only load terms in dropdown filters for which records exist #183

Merged
merged 14 commits into from
Jan 31, 2014

Conversation

johnregan3
Copy link
Contributor

Fetches records that exist in database, then compares it to list of all terms for records. If record doesn't exist in database, it is disabled in search filters dropdowns.

Fixes #141

cc/ @fjarrett

} elseif ( $table == 'meta' ) {
$rows = $wpdb->get_results( 'SELECT ' . $column . ' FROM ' . $wpdb->streammeta . ' GROUP BY ' . $column, 'ARRAY_A' );
} else {
$rows = $wpdb->get_results( 'SELECT ' . $column . ' FROM ' . $wpdb->streamcontext . ' GROUP BY ' . $column, 'ARRAY_A' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnregan3 These database queries need to be using $wpdb->prepare so all input values are properly escaped.

http://codex.wordpress.org/Class_Reference/wpdb#Run_Any_Query_on_the_Database

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjarrett I thought so, but I couldn't get prepare() to work correctly. I even used the same Codex article. I was starting to spend too much time on this one thing, so I abandoned my effort. I can continue to work on this, or if you want, we can have someone else take a look on it. Would you like me to continue on this myself?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnregan3 Try this out:

$rows = $wpdb->get_results(
    $wpdb->prepare(
        'SELECT %s FROM %s GROUP BY %s',
        $column,
        $wpdb->streamcontext,
        $column
    ),
    'ARRAY_A'
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fjarrett I tried this previously, and just tested it again. For some reason, this is the message I get. It's the same as I was getting previously with $wpdb->prepare();

WordPress database error: [You have an error in your SQL syntax; check the manual that 
corresponds to your MySQL server version for the right syntax to use near 
''wp_stream' GROUP BY 'author'' at line 1]
SELECT 'author' FROM 'wp_stream' GROUP BY 'author'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnregan3 Sorry, try this:

$rows = $wpdb->get_results(
    $wpdb->prepare(
        "SELECT %s FROM {$wpdb->streamcontext} GROUP BY %s",
        $column,
        $column
    ),
    'ARRAY_A'
);

There is no need to escape the table name because it's not an input.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shadyvb @jonathanbardo @c3mdigital Can any of you help us figure out why prepare() won't work correctly here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, saw me mentioned here before but then I couldn't find the comment so I thought it was deleted, but it's just that the diff became invalid.

You can't use prepare here because it is only for preparing values, not MySQL identifiers. In other words, the above statement:

$wpdb->prepare(
    "SELECT %s FROM {$wpdb->streamcontext} GROUP BY %s",
    $column,
    $column
);

Would produce the string:

SELECT "foo" FROM wp_streamcontext GROUP BY "foo"

Which first of all would select the string "foo" and not the column of that name, and then "foo" isn't something which can be grouped by, AFAIK.

So prepare is not appropriate for this purpose.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh duh! Thanks @westonruter

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a blog post! This is the second time the question arise in Stream haha!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LOL


if ( 'stream' == $table ) {
$rows = $wpdb->get_results( 'SELECT ' . $column . ' FROM ' . $wpdb->stream . ' GROUP BY ' . $column, 'ARRAY_A' );
} elseif ( $table == 'meta' ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnregan3 Should be: 'meta' === $table

@johnregan3
Copy link
Contributor Author

Looks good, bro!

frankiejarrett added a commit that referenced this pull request Jan 31, 2014
Issue 141 - Only load terms in dropdown filters for which records exist
@frankiejarrett frankiejarrett merged commit a833ee6 into master Jan 31, 2014
@frankiejarrett frankiejarrett deleted the issue-141 branch January 31, 2014 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants