Skip to content

Commit

Permalink
wp-env: Fix issue where docker & wp had different URLs (#20228)
Browse files Browse the repository at this point in the history
- Fixed: docker and WP had different URLs
   Caused by WordPress install not using the
   .wp-env.json custom port values, so it
   would redirect you to the default port.

- Added: validation function for numeric env vars
- Added: tests to validate port override behavior
  • Loading branch information
noahtallen authored Feb 14, 2020
1 parent e6603a6 commit 3a86ce1
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 5 deletions.
31 changes: 31 additions & 0 deletions packages/env/lib/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,10 @@ module.exports = {
config
);

config.port = getNumberFromEnvVariable( 'WP_ENV_PORT' ) || config.port;
config.testsPort =
getNumberFromEnvVariable( 'WP_ENV_TESTS_PORT' ) || config.testsPort;

if ( config.core !== null && typeof config.core !== 'string' ) {
throw new ValidationError(
'Invalid .wp-env.json: "core" must be null or a string.'
Expand Down Expand Up @@ -255,6 +259,33 @@ function includeTestsPath( source ) {
};
}

/**
* Parses an environment variable which should be a number.
*
* Throws an error if the variable cannot be parsed to a number.
* Returns null if the environment variable has not been specified.
*
* @param {string} varName The environment variable to check (e.g. WP_ENV_PORT).
* @return {null|number} The number. Null if it does not exist.
*/
function getNumberFromEnvVariable( varName ) {
// Allow use of the default if it does not exist.
if ( ! process.env[ varName ] ) {
return null;
}

const maybeNumber = parseInt( process.env[ varName ] );

// Throw an error if it is not parseable as a number.
if ( isNaN( maybeNumber ) ) {
throw new ValidationError(
`Invalid environment variable: ${ varName } must be a number.`
);
}

return maybeNumber;
}

/**
* Hashes the given string using the MD5 algorithm.
*
Expand Down
8 changes: 3 additions & 5 deletions packages/env/lib/env.js
Original file line number Diff line number Diff line change
Expand Up @@ -302,15 +302,13 @@ async function configureWordPress( environment, config ) {
commandOptions: [ '--rm' ],
};

const port = environment === 'development' ? config.port : config.testsPort;

// Install WordPress.
await dockerCompose.run(
environment === 'development' ? 'cli' : 'tests-cli',
`wp core install
--url=localhost:${
environment === 'development'
? process.env.WP_ENV_PORT || '8888'
: process.env.WP_ENV_TESTS_PORT || '8889'
}
--url=localhost:${ port }
--title='${ config.name }'
--admin_user=admin
--admin_password=password
Expand Down
70 changes: 70 additions & 0 deletions packages/env/test/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,76 @@ describe( 'readConfig', () => {
testsPort: 8889,
} );
} );

it( 'should throw an error if the port number environment variable is invalid', async () => {
readFile.mockImplementation( () =>
Promise.resolve( JSON.stringify( {} ) )
);
const oldPort = process.env.WP_ENV_PORT;
process.env.WP_ENV_PORT = 'hello';
try {
await readConfig( '.wp-env.json' );
} catch ( error ) {
expect( error ).toBeInstanceOf( ValidationError );
expect( error.message ).toContain(
'Invalid environment variable: WP_ENV_PORT must be a number.'
);
}
process.env.WP_ENV_PORT = oldPort;
} );

it( 'should throw an error if the tests port number environment variable is invalid', async () => {
readFile.mockImplementation( () =>
Promise.resolve( JSON.stringify( {} ) )
);
const oldPort = process.env.WP_ENV_TESTS_PORT;
process.env.WP_ENV_TESTS_PORT = 'hello';
try {
await readConfig( '.wp-env.json' );
} catch ( error ) {
expect( error ).toBeInstanceOf( ValidationError );
expect( error.message ).toContain(
'Invalid environment variable: WP_ENV_TESTS_PORT must be a number.'
);
}
process.env.WP_ENV_TESTS_PORT = oldPort;
} );

it( 'should use port environment values rather than config values if both are defined', async () => {
readFile.mockImplementation( () =>
Promise.resolve(
JSON.stringify( {
port: 1000,
testsPort: 2000,
} )
)
);
const oldPort = process.env.WP_ENV_PORT;
const oldTestsPort = process.env.WP_ENV_TESTS_PORT;
process.env.WP_ENV_PORT = 4000;
process.env.WP_ENV_TESTS_PORT = 3000;

const config = await readConfig( '.wp-env.json' );
expect( config ).toMatchObject( {
port: 4000,
testsPort: 3000,
} );

process.env.WP_ENV_PORT = oldPort;
process.env.WP_ENV_TESTS_PORT = oldTestsPort;
} );

it( 'should use 8888 and 8889 as the default port and testsPort values if nothing else is specified', async () => {
readFile.mockImplementation( () =>
Promise.resolve( JSON.stringify( {} ) )
);

const config = await readConfig( '.wp-env.json' );
expect( config ).toMatchObject( {
port: 8888,
testsPort: 8889,
} );
} );
} );

/**
Expand Down

0 comments on commit 3a86ce1

Please sign in to comment.