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

One notice for all missing db tables #506

Merged
merged 3 commits into from May 7, 2014

Conversation

Projects
None yet
3 participants
@shadyvb
Copy link
Contributor

commented May 6, 2014

Before:
download

/cc @fjarrett

@shadyvb

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2014

Worth noting that Stream should probably skip any queries if it found that its tables aren't there.

stream.php Outdated
}
}
if ( $missing_tables ) {
$database_message .= sprintf( '%s %s', __( 'The following table(s) are not present in the WordPress database: ', 'stream' ), implode( ', ', $missing_tables ) );

This comment has been minimized.

Copy link
@lukecarbis

lukecarbis May 7, 2014

Contributor

Could we use _n here?

This comment has been minimized.

Copy link
@shadyvb

shadyvb May 7, 2014

Author Contributor

Addressed in aefb3a3

@lukecarbis

This comment has been minimized.

Copy link
Contributor

commented on stream.php in aefb3a3 May 7, 2014

We should esc_html the implode.

This comment has been minimized.

Copy link
Contributor Author

replied May 7, 2014

It'll always be database tables, i don't think we need to do that.

This comment has been minimized.

Copy link
Contributor

replied May 7, 2014

Can't hurt though, and it will make Travis happy. It would also future-proof us if we ever decided to filter the results of the WP_Stream_DB::get_table_names() function.

This comment has been minimized.

Copy link
Contributor Author

replied May 7, 2014

I don't think we'll be adding filters for table names, we always control that, we're even moving off that quite soon. And i'm not keen about adding extra function calls where we don't need.. we really need to tone down the performance impact as much as we can, don't you think ?

This comment has been minimized.

Copy link
Contributor

replied May 7, 2014

Leaving it unescaped doesn't bother me as much as it bothers our friendly neighbourhood Continuous Integration service.

Maybe an // xss ok is in order.

But how much of a performance impact would an extra esc_html have?

This comment has been minimized.

Copy link
Contributor Author

replied May 7, 2014

Yea, i need to fix that for Travis, didn't notice the complaining.

Escaping doesn't really have a big performance impact, but saving bits and pieces off here and there might make a difference.

I'll fix this tonight, thanks for the heads up.

fjarrett added a commit that referenced this pull request May 7, 2014

Merge pull request #506 from x-team/issue-no-tables-notice-fix
One notice for all missing db tables

@fjarrett fjarrett merged commit e40732d into develop May 7, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@fjarrett fjarrett deleted the issue-no-tables-notice-fix branch May 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.