Documentation Code Review: What We Look For

Code Review: What We Look For

Overview #

Every line of code that is committed to WordPress.com VIP is reviewed by the VIP Team. We don’t do in-depth code reviews to add more time to or delay your launch schedules. We do these lengthy code reviews to help you launch successfully.

The goal of our reviews is to make sure that on launch, your site will be:

  • Secure, because pushing a site live with insecure code presents a liability to you and your whole userbase;
  • Performant, because going live and finding out that your code can’t handle the traffic levels that your site expects puts most of your launch efforts to waste.

We also review for development best practices to make sure that your site will continue to live on without significant maintenance costs or major issues when WordPress is upgraded.

Before submitting any code for review, please be sure to look through our Code Review Guidelines and Best Practices Documentation. The following is a checklist of items our VIP engineers look for when reviewing. Please note that this is a living list and we are adding and modifying it as we continue to refine our processes and platform.

We hope that by sharing this document, you will be able to better prepare your code for peer review.


↑ Top ↑

Blockers #

Blockers are items that need to be fixed before being committed to WordPress.com. Here’s a partial list of what can be a blocker:

Direct Database Queries #

Thanks to WordPress’ extensive API, you should almost never need to query database tables directly. Using WordPress APIs rather than rolling your own functions saves you time and assures compatibility with past and future versions of WordPress and PHP. It also makes code reviews go more smoothly because we know we can trust the APIs. More information.

Additionally, direct database queries bypass our internal caching. If absolutely necessary, you should evaluate the potential performance of these queries and add caching if needed.

↑ Top ↑

Database alteration #

The WordPress schema and API’s are roboust enough to handle almost any requirements. The core API’s are easy to use and add a performance layer to ensure that your site will run smoothly.

Creating or deleting tables, and schema modifications are not allowed. Themes and plugins should use the existing database tables and structure. Custom Post Types, Custom Taxonomies, post meta, etc. are incredibly flexible as an alternative to custom database tables.

↑ Top ↑

Filesystem writes #

Make sure that your code and plugins do not write to the filesystem. Since the WordPress.com network is distributed across many servers in multiple data centers, file system writes won’t work how they would in a single server environment. The core WordPress upload functions can handle any uploads you need to do.

↑ Top ↑

Validation, Sanitization, and Escaping #

Your code works, but is it safe? When writing code for the WordPress.com VIP environment, you’ll need to be extra cautious of how you handle data coming into WordPress and how it’s presented to the end user. Please review our documentation on validating, sanitizing, and escaping.

$_GET, $_POST, $_REQUEST, $_SERVER and other data from untrusted sources (including values from the database such as post meta and options) need to be validated and sanitized on save and escaped on output.

Nonces should be used to validate all form submissions.

Capability checks need to validate that users can take the requested actions.

It’s best to do the output escaping as late as possible, ideally as it’s being outputted, as opposed to further up in your script. This way you can always be sure that your data is properly escaped and you don’t need to remember if the variable has been previously validated.

Here are two examples. In order to keep this straight forward, we’ve kept them simple. Imagine a scenario with much more code between the place where $title is defined and where it’s used. The first example is more clear that $title is escaped.

$title = $instance['title'];

// Logic that sets up the widget

echo $before_title . esc_html( $title ) . $after_title;

 

$title = esc_html( $instance['title'] );

// Logic that sets up the widget

echo $before_title . $title . $after_title;

More information.

↑ Top ↑

Arbitrary JavaScript and CSS stored in options or meta #

To limit attack vectors via malicious users or compromised accounts, arbitrary JavaScript cannot be stored in options or meta and then output as-is.

CSS should also generally be avoided, but if absolutely necessary, it’s a good idea to properly sanitize it. See art-direction-redux, for an example.

↑ Top ↑

Uncached Functions #

WP provides a variety of functions that interact with the database, not all of which are cacheble. To ensure high performance and stability, please avoid using any of the functions listed on our Uncached Functions list.

↑ Top ↑

Whitelisting values for input/output validation #

When working with user-submitted data, try where possible to accept data only from a finite list of known and trusted values. For example:

$possible_values = array( 'a', 1, 'good' );
if ( ! in_array( $untrusted, $possible_values ) )
die( "Don't do that!" );

↑ Top ↑

Prefixing functions, constants, classes, and slugs #

Per the well-known WordPress adage: prefix all the things.

This applies to things obvious things such as names of function, constants, and classes, and also less obvious ones like post_type and taxonomy slugs, cron event names, etc.

↑ Top ↑

Creating a session writes a file to the server and is unreliable in a multi-server environment.

↑ Top ↑

Manipulating the timezone server-side #

Using date_default_timezone_set() and similar isn’t allowed because it conflicts with stats and other systems. Developers instead should use WordPress’s internal timezone support. More information.

↑ Top ↑

Removing the admin bar #

The WordPress.com admin bar cannot be removed as it’s integral to the user experience on WordPress.com.

↑ Top ↑

Working with wp_users and user_meta #

As a large multisite install, WordPress.com has a global users database.

Note that this means that you cannot create, edit, or delete users. For greater level of control, use Guest Authors.

This table is huge on WordPress.com and queries on it can easily cause performance issues. This includes doing any JOIN or meta operations against this set of tables. For parsing through a list of users, use get_users() and iterate in PHP.

For storing user additional user metadata, you should look at User Attributes.

↑ Top ↑

Caching large values in options #

This cache object (and any object in wp_cache in general) must not exceed 1MB. On WordPress.com, options are cached in memory to avoid database lookups, which speed things up. This is only effective if the cached object is kept small. Once the object reaches 1MB, it will no longer cache and requests are sent to the database servers, which, depending on the traffic of the site, can cause a flood of requests and have a severe impact on performance. More information. One option for getting around this problem is to use the wp_large_options plugin for large option values.

↑ Top ↑

Skipping Batcache #

Requests that prevent Batcache from caching the page like using $_GET params (e.g. ?abc=def) are not allowed, are you trying to make your site go down?! ;) More information.

You should try and implement your URLs using rewrite rules. If you’re having trouble, get in touch and we’ll help :)

↑ Top ↑

Ajax calls on every pageload #

Making calls to admin-ajax.php on every pageload will cause issues and need to be rethought. If you have questions, we would be happy to help work through an alternate implementation.

↑ Top ↑

Front-end db writes #

Functions used on the front-end that write to the database are not allowed. This is due to scaling concerns and can easily bring down a site.

↑ Top ↑

*_meta as a hit counters #

Please don’t use meta (post_meta, comment_meta, etc.) to track counts of things (e.g. votes, pageviews, etc.). First of all, it won’t work properly because of caching and due to race conditions on high volume sites. It’s also just a recipe for disaster and easy way to break your site.

↑ Top ↑

eval() and create_function() #

Both these functions can execute arbitrary code that’s constructed at run time, which can be created through difficult-to-follow execution flows. These methods can make your site fragile because unforeseen conditions can cause syntax errors in the executed code, which becomes dynamic. A much better alternative is an Anonymous Function, which is hardcoded into the file and can never change during execution.

If there are no other options than to use this construct, pay special attention not to pass any user provided data into it without properly validating it beforehand.

We strongly recommend using Anonymous Functions, which are much cleaner and more secure.

↑ Top ↑

switch_to_blog() #

Not something you should ever need to do in a VIP theme context. Use an API (XML-RPC, REST) to interact with other sites if needed.

↑ Top ↑

get_site_*() #

Not something you should ever need to do in a VIP context. Because WordPress.com is a multisite these are shared between all our sites. this means you cannot use functions like get_site_transient(), set_site_transient() or get_site_option() set_site_option()

↑ Top ↑

No LIMIT queries #

Using posts_per_page (or numberposts) with the value set to -1 or an unreasonably high number or setting nopaging to true opens up the potential for scaling issues if the query ends up querying thousands of posts.

You should always fetch the lowest number possible that still gives you the number of results you find acceptable. Imagine that your site grows over time to include 10,000 posts. If you specify -1 for posts_per_page, you’ll query with no limit and fetch all 10,000 posts every time the query runs, which is going to destroy your site’s performance. If you know you’ll never have more than 15 posts, then set posts_per_page to 15. If you think you might have more than 15 that you’d want to display but doubt it’d hit more than 100 ever, set the limit to 100. If it gets much higher than that, you might need to rethink the page architecture a bit.

↑ Top ↑

Cron schedules less than 15 minutes or expensive events #

Overly frequent cron events (anything less than 15 minutes) can significantly impact the performance of the site, as can cron events that are expensive.

↑ Top ↑

Flash (.swf) files #

Flash (.swf) files are not permitted on WordPress.com as they often present a security threat (largely due to poor development practices or due to bugs in the Flash Player) and vulnerabilities are hard to find/detect/secure. Plus, who needs Flash? :)

↑ Top ↑

Incorrect licenses #

Non-GPL compatible themes or plugins are not allowed on WordPress.com. WordPress code is licensed under the GNU Public License v2 (GPL2) and all theme and plugin code needs to be GPL compatible or custom code you’ve written in-house—split or proprietary licenses are not allowed. The reasoning for this is that you, and we, need to have the legal rights to modify the code if something is broken, insecure, needs optimization, etc.

↑ Top ↑

Ignore development only files #

We ask that you .svnignore any files that are use exclusively in local development of your theme, these include but are not limited to .git, .gitignore, config.rb, sass-cache, grunt files, PHPUnit tests, etc.

↑ Top ↑

VIP Requirements #

Every theme must include a VIP attribution link and wp_head() and wp_footer() calls.

↑ Top ↑

Unprefixed Functions, Classes, Constants, Slugs #

Long-standing WordPress best practice. Always namespace things in code to avoid potential conflicts. See Prefix Everything.

↑ Top ↑

404 Pages with Database Queries #

The 404 page needs to be the fastest on the site, as it’s never cached. That means that a traffic spike from a broken link can cause performance problems if the 404 page is doing db lookups.

↑ Top ↑

Debug code or output #

VIP themes should not contain debug code and should not output debugging information. If you’re encountering an issue that can’t be debugged in your development environment, we’ll be glad to help troubleshoot it with you.


↑ Top ↑

Proceed with Caution #

The following approaches should be considered carefully when including them in your VIP theme or plugin.

↑ Top ↑

Remote calls #

Remote calls such as fetching information from external APIs or resources should rely on the WordPress HTTP API (no cURL) and should be cached. More information.

↑ Top ↑

Using __FILE__ for page registration #

When adding menus or registering your plugins, make sure that you use an unique handle or slug other than __FILE__ to ensure that you are not revealing system paths.

↑ Top ↑

Functions that use JOINS, taxonomy relation queries, -cat, -tax queries, subselects or API calls #

Close evaluation of the queries is recommended as these can be expensive and lead to performance issues. Queries with known problems when working with large datasets:

  • category__and, tag__and, tax_query with AND
  • category__not_in, tag__not_in, and tax_query with NOT IN
  • tax_query with multiple taxonomies
  • meta_query with a large result set (e.g. looking for only posts with a thumbnail_id meta on a large site, looking for posts with a specific meta value on a key)

↑ Top ↑

Custom roles #

For best compatibility between environments and for added security, custom user roles and capabilities need to be managed via our helper functions.

↑ Top ↑

Caching constraints #

As we’re running batcache, server side based client related logic will not work. This includes things like logic based on $_SERVER['REMOTE_ADDR'] or similar. This should be switched to a js based approach or a cookie needs to be set and a batcache exception based on the cookie needs to be made.

Because Batcache caches fully rendered pages, per-user interactions on the server-side can be problematic. This means usage of objects/functions like $_COOKIE, setcookie,$_SERVER['HTTP_USER_AGENT'], and anything that’s unique to an individual user cannot be relied on as the values may be cached and cross-pollution can occur.

In most cases, any user-level interactions should be moved to client-side using javascript. More information.

↑ Top ↑

Using extract() #

extract() should never be used because it is too opaque and difficult to understand how it will behave under a variety of inputs. It makes it too easy to unknowingly introduce new variables into a function’s scope, potentially leading to unintended and difficult to debug conflicts.

↑ Top ↑

Using $_REQUEST #

$_REQUEST should never be used because it is hard to track where the data is coming from (was it POST, or GET, or a cookie?), which makes reviewing the code more difficult. Additionally, it makes it easy to introduce sneaky and hard to find bugs, as any of the aforementioned locations can supply the data, which is hard to predict.

Much better to be explicit and use either $_POST or $_GET instead.

↑ Top ↑

Not Using the Settings API #

Instead of handling the output of settings pages and storage yourself, use the WordPress Settings API as it handles a lot of the heavy lifting for you including added security.

Make sure to also validate and sanitize submitted values from users using the sanitize callback in the register_setting call.

↑ Top ↑

Using Page Templates instead of Rewrites #

A common “hack” in the WordPress community when requiring a custom feature to live at a vanity URL (e.g. /lifestream/) is to use a Page + Page Template. This isn’t ideal for numerous reasons:

  • Requires WordPress to do multiple queries to handle the lookup for the Page and any additional loops your manually run through.
  • Impedes development workflow as it requires the Page to be manually created in each environment and new developer machines as well.

↑ Top ↑

Using closing PHP tags #

All PHP files should omit the closing PHP tag to prevent accidental output of whitespace and other characters, which can cause issues such as ‘Headers already sent‘ errors. This is part of the WordPress Coding Standards.


↑ Top ↑

Performance Considerations #

We want to make sure that your site runs smoothly and can handle any traffic load. As such, we check your site for performance considerations such as: are remote requests fast and cached? Does the site request more data than needed?

↑ Top ↑

Uncached Pageload #

Uncached pageloads should be optimized as much as possible. We will load different pages and templates on your theme uncached, looking for slow queries, slow or timed out remote requests, queries that are overly repeated, or function routines that are slow.

↑ Top ↑

Memcache hits/misses #

Memcache misses or over-additions/updates can be a sign of potential problems.

    • Clear your cache on a page
    • In the “Memcache” tab in the Debug Bar, check the ratio of sets/adds vs gets. You should see almost as many sets/adds as gets.
    • Reload the page (without clearing the cache)
    • In the “Memcache” tab in the Debug Bar, check the ratio of sets/adds vs gets. You should see (almost) no sets/adds.
    • Reload the page a few more times and check the set/add-to-get ratio; the former should continue to remain 0 or minimal.

If you’re still seeing sets/adds in subsequent pages, that means that something is constantly being added to the cache that shouldn’t.