Fix translation registration for admin interface (#304)

* use bash instead of sh for bin/setup

* make add-hosts.sh work better in a non-Mac developemnt environment

* Update apache2-custom.sh to improve file ownership handling

* change default for SKIP_PLUGIN_CHOWN

* fix setup script

* fix bug in translations registration and test it

Author: mlwilkerson

bin/setup

@@ -1,4 +1,4 @@
-#!/bin/sh
+#!/bin/bash
 
 DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
 source $DIR/../.env
@@ -7,9 +7,9 @@ source $DIR/.resolve-container.bash
 
 if [ ! -f .env.local ]; then
   echo "Missing local env file: .env.local"
-  echo "\tIt must exist and set env var WP_ADMIN_EMAIL to some real email address."
-  echo "\tThis is the address WordPress will use for its admin email."
-  echo "\tIt's not checked in to git. It's .gitignore'd."
+  echo "  It must exist and set env var WP_ADMIN_EMAIL to some real email address."
+  echo "  This is the address WordPress will use for its admin email."
+  echo "  It's not checked in to git. It's .gitignore'd."
   exit 1
 else
   . .env.local
@@ -21,7 +21,37 @@ if [ "$?" != "0" ]; then
   echo "WARNING: your docker container will not resolve the wp.test hostname"
 fi
 
+# The install request below (and, later, your browser) reach WordPress by the
+# WP_DOMAIN host name. add-hosts.sh above only maps it *inside* the container;
+# it does nothing for this host. curl works around that with --resolve below, so
+# setup succeeds regardless. Your browser has no such shortcut, though, so warn
+# when WP_DOMAIN isn't mapped on this host to avoid a silent "can't reach the
+# site" foot gun. See DEVELOPMENT.md, "Add the wp.test host name".
+WP_HOST="${WP_DOMAIN%%:*}"
+
+host_resolves() {
+  if command -v getent >/dev/null 2>&1; then
+    getent hosts "$1" >/dev/null 2>&1 && return 0
+  fi
+  grep -qiE "[[:space:]]$1([[:space:]]|\$)" /etc/hosts 2>/dev/null && return 0
+  ping -c 1 "$1" >/dev/null 2>&1 && return 0
+  return 1
+}
+
+if ! host_resolves "$WP_HOST"; then
+  echo ""
+  echo "WARNING: '$WP_HOST' does not resolve on this host."
+  echo "  Setup will still initialize WordPress, but you won't be able to reach"
+  echo "  http://$WP_DOMAIN in your browser until you map '$WP_HOST' to localhost."
+  echo "  Add this line to your /etc/hosts (see DEVELOPMENT.md,"
+  echo "  \"Add the wp.test host name\"):"
+  echo ""
+  echo "    127.0.0.1 $WP_HOST"
+  echo ""
+fi
+
 rc=`curl \
+  --resolve "$WP_DOMAIN:127.0.0.1" \
   --write-out '%{http_code}' \
   --silent \
   --output /dev/null \
@@ -34,7 +64,9 @@ rc=`curl \
   --data-urlencode "pw_weak=1"`
 
 if [ "$rc" == "200" ]; then
-  echo "SUCCESS initializing WordPress\n\tAdmin dashboard here:\n\thttp://$WP_DOMAIN/wp-admin"
+  echo "SUCCESS initializing WordPress"
+  echo "  Admin dashboard here:"
+  echo "  http://$WP_DOMAIN/wp-admin"
 else
   echo "FAIL with HTTP $rc"
   exit 1

docker-compose.yml

@@ -118,6 +118,10 @@ x-wordpress-defaults: &wordpress_defaults
     - "8765:80"
   extra_hosts:
     - "dockerhost:169.254.254.254"
+    # Makes host.docker.internal resolve to the docker host on Docker Engine for
+    # Linux (20.10+), where it isn't provided automatically as it is on Docker
+    # Desktop for Mac/Windows. Harmless on Docker Desktop.
+    - "host.docker.internal:host-gateway"
   command:
     - apache2-custom.sh
 

docker/add-hosts.sh

@@ -1,16 +1,39 @@
 #!/bin/bash
 
-INTERNAL_IP=$( php -r 'print gethostbyname("host.docker.internal");' )
+# Resolve the docker host's IP address so we can point wp.test at it.
+#
+# host.docker.internal resolves automatically inside containers on Docker
+# Desktop (Mac/Windows), but not on Docker Engine for Linux (e.g. an Ubuntu EC2
+# host) unless the compose file maps it via `extra_hosts: host.docker.internal:
+# host-gateway`. Where it can't be resolved, fall back to the container's
+# default gateway, which is the docker host on a Linux bridge network.
 
-if [ "xhost.docker.internal" == x${INTERNAL_IP} ]; then
-	echo "FAILED resolving host.docker.internal in the docker container"
-	exit 1
+# Parse the default route's gateway from /proc/net/route. The gateway is stored
+# little-endian hex, and this needs no iproute2 (`ip`) dependency.
+default_gateway() {
+  local iface dest gateway rest
+  while read -r iface dest gateway rest; do
+    [ "$dest" = "00000000" ] || continue
+    printf '%d.%d.%d.%d\n' \
+      "0x${gateway:6:2}" "0x${gateway:4:2}" "0x${gateway:2:2}" "0x${gateway:0:2}"
+    return 0
+  done < /proc/net/route
+  return 1
+}
+
+INTERNAL_IP=$( getent hosts host.docker.internal | awk '{ print $1 }' | head -n1 )
+
+if [ -z "$INTERNAL_IP" ]; then
+  INTERNAL_IP=$( default_gateway )
 fi
 
-echo "$INTERNAL_IP wp.test" >> /etc/hosts
+if [ -z "$INTERNAL_IP" ]; then
+  echo "FAILED resolving the docker host IP address for wp.test"
+  exit 1
+fi
 
-if [ "$?" == "0" ]; then
-  echo "SUCCESS adding wp.test to container /etc/hosts"
+if echo "$INTERNAL_IP wp.test" >> /etc/hosts; then
+  echo "SUCCESS adding wp.test ($INTERNAL_IP) to container /etc/hosts"
 else
   echo "FAILED attempting to add wp.test to container /etc/hosts"
   exit 1

docker/apache2-custom.sh

@@ -1,8 +1,57 @@
 #!/bin/bash
 
 # See https://github.com/docker-library/wordpress/issues/205
+#
+# WordPress/Apache needs /var/www/html to be writable by www-data, which we
+# used to guarantee with a blanket recursive chown of the whole tree:
+#
+#     chown -Rf www-data:www-data /var/www/html/
+#
+# The catch: this repo and the integration themes/plugins are bind-mounted into
+# /var/www/html from the host, so a recursive chown walks into those bind mounts
+# and reowns the host's working copy to www-data (uid 33). On the host that
+# trips git's "dubious ownership" guard (CVE-2022-24765) and causes other
+# permission friction.
+#
+# By default (SKIP_PLUGIN_CHOWN unset or any value other than "false") we chown
+# everything under /var/www/html EXCEPT the bind-mounted host paths, so the host
+# working copy keeps its ownership. The excluded paths default to this plugin
+# plus the integration mounts wired up in docker-compose.yml; override with
+# CHOWN_EXCLUDE_PATHS (a colon-separated list) if you add or remove bind mounts
+# there. Set SKIP_PLUGIN_CHOWN=false to restore the original blanket behavior.
 
-chown -Rf www-data:www-data /var/www/html/
+CHOWN_ROOT="/var/www/html"
+
+default_excludes=(
+  "$CHOWN_ROOT/wp-content/plugins"
+  "$CHOWN_ROOT/wp-content/js"
+  "$CHOWN_ROOT/wp-content/themes"
+)
+
+if [ "$SKIP_PLUGIN_CHOWN" != "false" ]; then
+  if [ -n "$CHOWN_EXCLUDE_PATHS" ]; then
+    IFS=':' read -ra excludes <<< "$CHOWN_EXCLUDE_PATHS"
+  else
+    excludes=("${default_excludes[@]}")
+  fi
+
+  # Build a find(1) prune expression: ( -path A -o -path B -o ... )
+  prune=()
+  for path in "${excludes[@]}"; do
+    [ -n "$path" ] || continue
+    [ ${#prune[@]} -eq 0 ] || prune+=( -o )
+    prune+=( -path "$path" )
+  done
+
+  if [ ${#prune[@]} -gt 0 ]; then
+    # Skip (don't descend into) the excluded bind-mount paths, chown the rest.
+    find "$CHOWN_ROOT" \( "${prune[@]}" \) -prune -o -exec chown -f www-data:www-data {} +
+  else
+    chown -Rf www-data:www-data "$CHOWN_ROOT/"
+  fi
+else
+  chown -Rf www-data:www-data "$CHOWN_ROOT/"
+fi
 
 DEFINES=""
 

includes/class-fontawesome.php

@@ -435,11 +435,6 @@ function () {
 
 			$this->maybe_enqueue_admin_assets();
 
-			// Setup JavaScript internationalization if we're on WordPress 5.0+.
-			if ( function_exists( 'wp_set_script_translations' ) ) {
-				wp_set_script_translations( self::ADMIN_RESOURCE_HANDLE, 'font-awesome' );
-			}
-
 			if ( $this->using_kit() ) {
 				if ( $this->skip_enqueue_kit() ) {
 					// Normally, conflict detection is built into a kit.
@@ -1703,6 +1698,14 @@ function ( $hook ) {
 				try {
 					if ( $this->detecting_conflicts() || $hook === $this->screen_id || $should_enable_icon_chooser ) {
 						$this->enqueue_admin_js_assets( $should_enable_icon_chooser );
+
+						// Setup JavaScript internationalization if we're on WordPress 5.0+.
+						// This must happen after the admin script is registered/enqueued
+						// (in enqueue_admin_js_assets), so that the textdomain is
+						// associated with the script object before it is printed.
+						if ( function_exists( 'wp_set_script_translations' ) ) {
+							wp_set_script_translations( self::ADMIN_RESOURCE_HANDLE, 'font-awesome' );
+						}
 					}
 
 					if ( $hook === $this->screen_id ) {

tests/test-enqueue.php

@@ -572,10 +572,129 @@ public function test_conflict_detector_enqueued_when_enabled_webfont() {
 		/**
 		 * We do not test the v4 font face shim inline style has the detection ignore
 		 * attr, since we can't filter the inline style tag.
-		 * 
+		 *
 		 * (Probably we'll have a separate mechanism for ignoring that, if the
 		 * conflict detector reports it.)
 		 */
 	}
+
+	/**
+	 * Regression test for JavaScript translations on the admin settings page.
+	 *
+	 * The admin bundle (font-awesome-official-admin) is not registered until the
+	 * admin_enqueue_scripts hook fires (inside maybe_enqueue_admin_assets ->
+	 * enqueue_admin_js_assets). wp_set_script_translations() must therefore run
+	 * *after* that registration, or WP_Scripts::print_translations() bails early
+	 * (empty textdomain) and never emits the inline setLocaleData script, leaving
+	 * the settings page in English regardless of the site locale.
+	 *
+	 * This drives the real admin_enqueue_scripts flow and asserts that the admin
+	 * script ends up with its textdomain associated, and that the setLocaleData
+	 * inline script is produced.
+	 */
+	public function test_admin_script_translations_are_set() {
+		// Enable conflict detection so the admin JS bundle is enqueued on
+		// admin_enqueue_scripts regardless of the current screen. This lets us
+		// exercise the real enqueue flow without standing up a settings-page
+		// screen_id.
+		$later = time() + ( 10 * 60 );
+		update_option(
+			FontAwesome::CONFLICT_DETECTION_OPTIONS_KEY,
+			array_merge(
+				FontAwesome::DEFAULT_CONFLICT_DETECTION_OPTIONS,
+				array( 'detectConflictsUntil' => $later )
+			)
+		);
+
+		// set_up() pre-registers a stub for the admin handle so unrelated tests
+		// can satisfy the conflict detector's dependency. Deregister it here so
+		// this test reproduces the real runtime, where the admin bundle is not
+		// registered until admin_enqueue_scripts fires. (Otherwise the premature,
+		// buggy wp_set_script_translations() call would find the stub already
+		// registered and the bug would be masked.)
+		wp_deregister_script( FontAwesome::ADMIN_RESOURCE_HANDLE );
+
+		// There is no real .json translation file installed in the test
+		// environment, so short-circuit the file lookup with fake translation
+		// data. print_translations() still bails on an empty textdomain, so this
+		// filter does not paper over the bug being tested.
+		$fake_translations = wp_json_encode(
+			array(
+				'domain'      => 'messages',
+				'locale_data' => array(
+					'messages' => array(
+						''             => array(
+							'domain' => 'messages',
+							'lang'   => 'ja',
+						),
+						'Settings'     => array( '設定' ),
+					),
+				),
+			)
+		);
+
+		add_filter(
+			'pre_load_script_translations',
+			function ( $translations, $file, $handle, $domain ) use ( $fake_translations ) {
+				if ( FontAwesome::ADMIN_RESOURCE_HANDLE === $handle && 'font-awesome' === $domain ) {
+					return $fake_translations;
+				}
+				return $translations;
+			},
+			10,
+			4
+		);
+
+		// Run the plugin's init. This registers the admin_enqueue_scripts
+		// callback. On the buggy code, this is also where wp_set_script_translations()
+		// was (incorrectly) called -- too early, before the admin bundle exists.
+		fa()->init();
+
+		$this->assertFalse(
+			wp_script_is( FontAwesome::ADMIN_RESOURCE_HANDLE, 'registered' ),
+			'Precondition: the admin bundle must not be registered until admin_enqueue_scripts runs.'
+		);
+
+		// A current screen must be set, since WordPress core callbacks on
+		// admin_enqueue_scripts (e.g. wp_auth_check_load) read
+		// get_current_screen()->id.
+		set_current_screen( 'settings_page_font-awesome' );
+
+		// Fire the admin_enqueue_scripts hook. This registers/enqueues the admin
+		// bundle and -- with the fix -- associates the textdomain with it.
+		do_action( 'admin_enqueue_scripts', 'settings_page_font-awesome' );
+
+		$wp_scripts = wp_scripts();
+
+		$this->assertArrayHasKey(
+			FontAwesome::ADMIN_RESOURCE_HANDLE,
+			$wp_scripts->registered,
+			'The admin bundle should be registered after admin_enqueue_scripts.'
+		);
+
+		// Root cause from the bug report: the script object's textdomain property
+		// was never set, so print_translations() returned early.
+		$this->assertEquals(
+			'font-awesome',
+			$wp_scripts->registered[ FontAwesome::ADMIN_RESOURCE_HANDLE ]->textdomain,
+			'The admin script must have its textdomain set to font-awesome.'
+		);
+
+		// With the textdomain set, print_translations() should emit the inline
+		// setLocaleData script -- the <script id="font-awesome-official-admin-js-translations">
+		// element that was reported as absent from the DOM.
+		$translations_output = $wp_scripts->print_translations( FontAwesome::ADMIN_RESOURCE_HANDLE, false );
+
+		$this->assertNotFalse(
+			$translations_output,
+			'print_translations() must not return false; otherwise no setLocaleData script is output.'
+		);
+
+		$this->assertStringContainsString(
+			'wp.i18n.setLocaleData',
+			$translations_output,
+			'The translations output should call wp.i18n.setLocaleData.'
+		);
+	}
 }