Review / questions

[joehoyle]

Like it, so much cleaner than the “old” plugin that is typically used to do this. I have a few questions etc below. I did this while offline so sorry for no links, however having said that, you can’t comment on lines for committed stuff, unless maybe you do a new branch from the first hash, then create a pull-request to do a review of the whole project!

• Rather than bootstrap() calling startup() seems it would make more sense to name bootstrap perform_checks() or something, then call startup() from the controlling caller code.
• define( 'DOMAIN_MAPPING', 1 ); is this meant to be a boolean?
• dbDelta wuuuuut… moving on…
• DB schema, as most of core does, you have crossed terminology with “site” and “blog”, e.g. ' WHERE blog_id = %d', $site are you trying to use the same table schema as the old plugin?
• class-mapping.php:364 check for WP_Error, however in seems get_by_domain() can only return an instance or null
• in create() it seems you only try create the domain mapping table the fist time a domain mapping is attempted to be added. Given that this needs to be done in the a dmi (from that I can tell from create_table()) do you think that could be problematic? Thinking wp-cli commands, install scripts etc.
• I couldn’t work out why you were rolling for own pages for the admin (header and footer), is it not possible to use the add menu page functions in network admin?
• Is the use of $_REQUEST necessary though out the admin? I always wince a little when it is always $_GET / $_POST exclusively.
• I didn’t see anything to do with redirecting the site path / subdomain to the mapped domain, that might be a handy feature.
• The check for COOKIE_DOMAIN is potentially a bit full-on as without WP_DEBUG I think 99% of people wold miss the X-Mercator header, and you’ll just get a white screen 500 error. Would it be so bad to show this message if ! WP_DEBUG

[rmccue]

Rather than bootstrap() calling startup() seems it would make more sense to name bootstrap perform_checks() or something, then call startup() from the controlling caller code.

Will fix.

define( 'DOMAIN_MAPPING', 1 ); is this meant to be a boolean?

This is for compatibility.

dbDelta wuuuuut… moving on…

Core uses it, so it's a pretty stable API that does exactly what I want. :)

DB schema, as most of core does, you have crossed terminology with “site” and “blog”, e.g. ' WHERE blog_id = %d', $site are you trying to use the same table schema as the old plugin?

Indeed, the idea is to use exactly the same schema here. The only difference is an extra index on domain, which seems like an obvious addition (thanks dbDelta!). FWIW, I checked and WP.com also has an index on it, plus they have some extra columns that we don't need.

class-mapping.php:364 check for WP_Error, however in seems get_by_domain() can only return an instance or null

This is intentional, to match the format of the other ::get*() methods. We might need to return a WP_Error in the future.

in create() it seems you only try create the domain mapping table the fist time a domain mapping is attempted to be added. Given that this needs to be done in the a dmi (from that I can tell from create_table()) do you think that could be problematic? Thinking wp-cli commands, install scripts etc.

(Whereby "a dmi" you mean "admin" 😉)

It should be OK, the only problem is that it needs to be loaded post-WP-bootstrap, as wp-admin/includes/upgrade.php includes files that have calls to __() (sigh).

Alternatively, we can call wp_load_translations_early() just before loading that file, but I'd rather see if we need to first.

I couldn’t work out why you were rolling for own pages for the admin (header and footer), is it not possible to use the add menu page functions in network admin?

They're added to the inline submenu for the edit site page, rather than on the full page.

Is the use of $_REQUEST necessary though out the admin? I always wince a little when it is always $_GET / $_POST exclusively.

I'd agree, but most of these can actually be given by either GET or POST.

(Worth mentioning that unlike PHP's use of $_REQUEST, WP forces it to be just GET + POST, not including cookies or session variables.)

I didn’t see anything to do with redirecting the site path / subdomain to the mapped domain, that might be a handy feature.

That's intentional right now; this would break Happytables, for example. That said, I do want to do some sort of canonical redirection, but will look at that soonish.

The check for COOKIE_DOMAIN is potentially a bit full-on as without WP_DEBUG I think 99% of people wold miss the X-Mercator header, and you’ll just get a white screen 500 error. Would it be so bad to show this message if ! WP_DEBUG

Will fix.

[rmccue]

Fixed aforementioned issues. :)