[ Intl ] disable Intl functions in bootWordpress by default

[mho22]

Motivation for the change, related issues

The presence of the Intl extension is detected by checking if functions exist in web mode, even when withICU is set to false or not set at all.

Since withICU is set to false by default, the related functions should be disabled.

This is a first and temporary attempt, as intl is currently static. The best approach would be to generate a dynamic intl extension and load it at runtime, if necessary.

Implementation details

Alongside withNetworking or networking, add withICU and icu in types and function parameters. Set withICU to false by default and load a comprehensive list of intl functions to disable if withICU is set to false.

To improve readability and maintainability, the list of disabled functions is separated into a dedicated file.

Testing Instructions (or ideally a Blueprint)

Tests are provided in packages/playground/website/playwright/e2e/blueprints.spec.ts

[mho22]

Two files were generated while I was testing my code :

packages/playground/blueprints/public/blueprint-schema-validator.js
packages/playground/blueprints/public/blueprint-schema.json

Should I restore them ?

This part takes the current disable_functions string entry, converts it in an array, concatenates it with the dedicated intlDisabledFunctions, removes any empty strings and then converts it back to a string.

phpIniEntries['disable_functions'] = (
                phpIniEntries['disable_functions'] ?? ''
	)
	.split(',')
	.concat(intlDisabledFunctions)
	.filter((n) => n)
	.join(',');

Do you have any suggestions on how and in which file I could test this new feature?

[adamziel]

Two files were generated while I was testing my code :

That's good, they reflect the current Blueprint JSON schema based on TypeScript types.

[adamziel]

Maybe intl instead? The name relates to internationalization and a PHP extension, whereas icu isn't that well known abbreviation.

[adamziel]

Do you have any suggestions on how and in which file I could test this new feature?

packages/playground/blueprints/src/lib/compile.spec.ts may be a good candidate for adding a few more tests. That would test it with the Node.js Playground version. I don't imagine anything would be different in the browser, but if you'd rather test it with Chrome/Safari/Firefox, there's some Playwright tests in packages/playground/website/playwright/e2e/blueprints.spec.ts.

[mho22]

I managed to get the tests working in Firefox and WebKit:

  ✓  44 [webkit] › blueprints.spec.ts:199:5 › Intl functions should be disabled by default (9.6s)
  ✓  46 [webkit] › blueprints.spec.ts:224:5 › Intl functions should work when intl is enabled (12.6s)

Even though WebKit initially failed.

Chromium failed with Error: Timed out 60000ms waiting for expect(locator).not.toBeEmpty() in my new test Intl functions should work when intl is enabled. I will rerun the tests to see if the Chromium test passes.

[mho22]

Rerunning the tests return an artifact not found error :

Run actions/download-artifact@v4
Downloading single artifact
Error: Unable to download artifact(s): Artifact not found for name: playwright-dist
        Please ensure that your artifact is not expired and the artifact was uploaded using a compatible version of toolkit/upload-artifact.
        For more information, visit the GitHub Artifacts FAQ: https://github.com/actions/toolkit/blob/main/packages/artifact/docs/faq.md

That's odd.

Oh no it means, I have to rerun the entire test suite I suppose ?

[adamziel]

Oh no it means, I have to rerun the entire test suite I suppose ?

No idea! Potentially yes :(

[mho22]

It seems to pass for Firefox and WebKit, but without an explanation, it's unclear why it doesn't pass for Chromium.

[adamziel]

@mho22 we're storing playwright traces with test run reports, would that help? See the artifacts at:

https://github.com/WordPress/wordpress-playground/actions/runs/15556615881?pr=2247

Alternatively, you might be able to run playwright locally and see where it fails. I assume manually following the steps just works since the test passes in the other two browsers?

[mho22]

I ran the isolated tests locally by running this command :

node_modules/.bin/playwright test --config=packages/playground/website/playwright/playwright.config.ts --project=chromium

And I got these results :

Running 2 tests using 1 worker

  ✓  1 [chromium] › intl.spec.ts:4:5 › Intl functions should be disabled by default (3.3s)
  ✓  2 [chromium] › intl.spec.ts:29:5 › Intl functions should work when intl is enabled (3.1s)

  2 passed (7.2s)

To open last HTML report run:

  npx playwright show-report packages/playground/website/playwright-report

Let's see if Github actions returns the same results.

[mho22]

This is a complete mystery. I have no clue why chromium does not find a body while webkit and firefox do and while locally on my machine they all have that body, even chromium.

This could possibly be a problem within ci.yml but where ?
During the test-e2e-playwright-prepare ?

[adamziel]

I wonder if it's something specific to GitHub CI setup. Definitely weird!

[mho22]

I commented the lines related to the ICU data file fetch in with-icu-data and the tests passed the not.toBeEmpty step.

[mho22]

These lines are responsible of the empty body :

const filePath = (await import('../../public/shared/icudt74l.js')).dataFilename;

const ICUData = await (await fetch(filePath)).arrayBuffer();

@bgrgicak @adamziel @brandonpayton Has this issue occurred in the past during CI tests? Have we observed any unusual behavior related to file imports or fetching?

Edit :

I am testing this approach

const fileName = 'icudt74l.dat';
/* @vite-ignore */
const filePath = (
	await import(new URL(`./shared/icudt74l.js`, import.meta.url).pathname)
).dataFilename;
const ICUData = await (await fetch(filePath)).arrayBuffer();

[adamziel]

I wonder if we're just running out of memory again and the tab is crashing 😢 I suppose we could disable this test in Chromium if we still have Firefox as a canary

[adamziel]

@mho22 We'll also need to disable Intl classes to address cases like this in WooCommerce – right now it generates a bunch of error logs:

https://github.com/woocommerce/woocommerce/blob/3c4bb10110a8340324c2589053133e62340ed4fd/plugins/woocommerce/includes/wc-core-functions.php#L1953

[bgrgicak]

I'm not entirely sure about this, so could you please share your use case with @playground/client so I can look into it further?

The use case I'm having issue with is on a private repo, so I need to create an isolated example for you. It might take me a couple of days. I'm currently focused on something else.

[mho22]

@adamziel Disabling a "built-in" class is a bit more challenging since I couldn't find a direct way to do it. I believe the best approach would be to dynamically load the intl extension. I considered wrapping class_exists, but I think that might lead us in a problematic direction. I should focus on dynamically loading intl soon. Or am I missing something?

@bgrgicak No worries at all.

[adamziel]

@mho22 how about the disable_classes php.ini directive?

[mho22]

@adamziel Unfortunately, disable_classes won't make the classes disappear, only prevent them to be instantiated.

I tested some code to verify that :

import { PHP, setPhpIniEntries } from '@php-wasm/universal';
import { loadNodeRuntime } from '@php-wasm/node';


const script = `<?php

var_dump( class_exists( 'Collator' ) );

$collator = new Collator('en');
`;


const php = new PHP( await loadNodeRuntime( '8.4' ) );

await setPhpIniEntries( php, { disable_classes : 'Collator' } );

const result = await php.run( { code : script } );

console.log( result.text );

This will result in

bool(true)
<br />
<b>Warning</b>:  Collator() has been disabled for security reasons in <b>/internal/eval.php</b> on line <b>5</b><br />

And as you can see, Collator still exists.

[adamziel]

Ah, snap. Gotcha! Let's wait for the dynamic extensions, then.

[bgrgicak]

The use case I'm having issue with is on a private repo, so I need to create an isolated example for you. It might take me a couple of days. I'm currently focused on something else.

@mho22 I created a isolated test where I load the latest version of the Playground Client and build the project with Vite.
It works without any issues, so the .dat file error must be specific to my other project.

[mho22]

@bgrgicak Thanks for the update. Let me know if I can help you.

[bgrgicak]

@bgrgicak Thanks for the update. Let me know if I can help you.

Thanks, I just update Playground in the private repo and it works without any changes to vite.config.js.

[bgrgicak]

@mho22 I was finally able to recreate it. This repo will trigger the .dat error.

I had to install the Blueprints package, which depends on @php-wasm/web to recreate the issue.

[mho22]

@bgrgicak I'm sorry for the inconvenience. I currently can't reproduce the error locally when I run node_modules/.bin/nx playground-blueprints:build and replace this in the package.json file of your project :

- "@wp-playground/blueprints": "^1.1.3",
+ "@wp-playground/blueprints": "file://../wordpress-playground/dist/packages/playground/blueprints",

It works on my machine. I don't know, I am still investigating. I probably should add ignoreDataImports as a plugin in blueprints/vite.config.ts file but why can't I reproduce the error without it locally then?

[adamziel]

The published packages are different than the dist directory, eg they may have less files or additional package json entries

[bgrgicak]

You can use npm run local-package-repository to test using packages built from your local environment.
It's a bit slow, so I usually just make changes inside Node modules and then test a fix using the local package repository.

[mho22]

@bgrgicak Thank you! Let's try that and correct the issue.

[mho22]

This is the error encountered by @bgrgicak :

No loader is configured for ".dat" files: node_modules/@php-wasm/web/shared/icudt74l.dat

    node_modules/@php-wasm/web/shared/icudt74l.js:1:25:
      1 │ import dataFilename from './icudt74l.dat';
        ╵                          ~~~~~~~~~~~~~~~~

/Users/mho/Work/Projects/Development/Web/Professional/intl/template/node_modules/esbuild/lib/main.js:1463
  let error = new Error(text);
              ^

Error: Build failed with 1 error:
node_modules/@php-wasm/web/shared/icudt74l.js:1:25: ERROR: No loader is configured for ".dat" files: node_modules/@php-wasm/web/shared/icudt74l.dat
    at failureErrorWithLog (/Users/mho/Work/Projects/Development/Web/Professional/intl/template/node_modules/esbuild/lib/main.js:1463:15)

I struggled to find the origin of it. But I finally found it and it seems esbuild will trigger that import and return that loader error independently of vite when running npm run dev.

And I couldn't find a way to customize the loader for dat files for esbuild inside wordpress-playground since it is not related to the wordpress-playground but related to the user's personal project.

So inevitably, I had to avoid the use of "import 'icudt74l.dat' anywhere in the code since esbuild doesn't support dat files by default.

This led me to that solution :

const fileName = 'icudt74l.dat';
- const filePath = (await import('../../public/shared/icudt74l.js')).dataFilename;
+ const filePath = new URL('../../@php-wasm/web/shared/icudt74l.dat', import.meta.url).toString();
const ICUData = await (await fetch(filePath)).arrayBuffer();

This means we only get the correct url of the file inside node_modules and fetches it regardless of any import.
I think it works. But honestly, I don't like the use of ../../@php-wasm and I don't know if I can use new URL().

I previously used the same technique as for the .wasm files but unlike .wasm files that can be loaded by esbuild, dat files don't.

[adamziel]

I keep wondering if we could get it to work somehow, either with an esbuild plugin or by migrating to rollup. Vite uses esbuild and rollup behind the scenes after all. AFAIR the only reason we don't use vite for this was some nuance of the library build mode – I think it included some static files in JS inline as base64 or something to that effect

[adamziel]

@mho22 would this plugin help? It seems to be processing dynamic import("file.dat?url") in the way we need:

https://github.com/adamziel/esbuild-url-dynamic-import

[mho22]

I am not sure if you're talking about building it in php-wasm-node [ which uses ESbuild ] or php-wasm-web [ which uses Vite ]. But the current issue comes from php-wasm-web here and we use Vite to build it :

php-wasm/web/vite.config.ts :

build: {
	lib: {
		// Could also be a dictionary or array of multiple entry points.
		entry: 'src/index.ts',
		name: 'php-wasm-web',
		fileName: 'index',
		formats: ['es', 'cjs'],
	},
	sourcemap: true,
	rollupOptions: {
		// Don't bundle the PHP loaders in the final build. See
		// the preserve-php-loaders-imports plugin above.
		external: [
			/php_\d_\d.js$/,
			/icudt74l.js$/,
			...getExternalModules(),
		],
	},
},

You probably suggest to replace the Vite building with an Esbuild building to have more control on that import. And you are right, it builds it and resolves it with :

icudata_default = new URL("icudata-1c8c6df5.dat", import.meta.url).href;

No more 'import', which will solve the issue! I should try to make this work using Vite.

[adamziel]

You probably suggest to replace the Vite building with an Esbuild building to have more control on that import.

Actually I got confused with which tool do we use for what. So many tools! I wish we could just use esbuild, but it doesn't do well with commonjs builds which is why Vite also ships rollup. And, after thinking about this more, I guess we do need an import in the built file because that is a library to be imported in other packages and needs a strong, statically analyzable tie between the script and the data file, which the import provides – the downstream build tools can be typically configured to treat the imported .dat file as an asset.

[adamziel]

Okay, I'm completely lost in what the problem is. Is it with build? Is it with running from source? Is it in a downstream build?

[mho22]

Sorry for the delay. I had to make sure I tested everything before replying. The problem comes from a downstream build. Currently, the icudt74l.dat file is fetched as follows :

php-wasm/web/src/lib/with-icu-data.ts

const filePath = (await import('../../public/shared/icudt74l.js')).dataFilename;
const ICUData = await (await fetch(filePath)).arrayBuffer();

it imports the icudt74l.js file like it's done for wasm files :

php-wasm/web/public/shared/icudt74l.js

import dataFilename from './icudt74l.dat';
export { dataFilename };

it exports the path of the file in @fs. And because we have rollupOptions.externals to not bundle the icudt74l.js file, the .dat file is not bundled.

We also have the vite plugin :

{
name: 'preserve-data-loaders-imports',

resolveDynamicImport(specifier): string | void {
	if (
		command === 'build' &&
		typeof specifier === 'string' &&
		specifier.match(/icudt74l\.js$/)
	) {
		/**
			* The ../ is weird but necessary to make the final build say
			* import("./shared/icudt74l.js")
			* and not
			* import("shared/icudt74l.js")
			*
			* The slice(-2) will ensure the 'public/`
			* portion is removed.
			*/
		return '../' + specifier.split('/').slice(-2).join('/');
	}
    },
}, 

That will modify the dynamic import path to match the built correct path, while the unbuilt path is already correct.

So the source is ok, because intl tests pass. And the build is ok since it doesn't fail to build.

But.

When I make a simple side project with @php-wasm/web :

package.json

{
    "type" : "module",
    "dependencies" : {
        "@php-wasm/universal" : "^1.1.4",
        "@php-wasm/web" : "^1.1.4",
        "vite" : "^6.2.5"
    },
    "scripts" : {
        "dev" : "vite"
    }
}

index.html

<!DOCTYPE html>
<html>
    <head>
        <script type="module" src="web.js"></script>
    </head>
    <body>
        <div id="app" />
    </body>
</html>

web.js

import { PHP } from '@php-wasm/universal';
import { loadWebRuntime } from '@php-wasm/web';

new PHP( await loadWebRuntime( '8.4' ) );

And run npm install && npm run dev it returns this :

✘ [ERROR] No loader is configured for ".dat" files: node_modules/@php-wasm/web/shared/icudt74l.dat

    node_modules/@php-wasm/web/shared/icudt74l.js:1:25:
      1 │ import dataFilename from './icudt74l.dat';

And this is, to my understanding, because, by default, Vite (via esbuild) tries to statically analyze and bundle all imports, even in node_modules. So when it sees:

import dataFilename from './icudt74l.dat';

and has no loader for .dat files, it throws. It doesn’t "ignore" unknown files — it expects every import to be resolvable at build time unless told otherwise.

I found solutions by not using the import method, but as you said, it is mandatory. So I made different tests, by building php-wasm-web after modifying the import method.

1. If I import( '../../public/shared/icudt74l.dat' ) directly -> No surprises : ERROR No loader is configured for ".dat" files: node_modules/@php-wasm/web/shared/icudt74l.dat

2. If I import( '../../public/shared/icudt74l.dat?url' ) -> it creates two new files during build :

dist/packages/php-wasm/web/icudt74l-CyBu59Ss.js  41,043.99 kB
dist/packages/php-wasm/web/icudt74l-CgrFAaac.cjs  41,044.06 kB

3. If I import( '../../public/{fileName}' ) -> ERROR No loader is configured for ".dat" files: node_modules/@php-wasm/web/shared/icudt74l.dat

And since we use Vite to build php-wasm-web, we don't have direct access to esbuild. The use of the import method will probably always trigger the No loader error.

[adamziel]

Good investigation @mho22, thank you! Let's move this to a separate issue for posterity, I feel we'll keep bumping into this and we could use a single source of context. This may also be a good opportunity to create a canonical solution for isomorphic loading of static assets across the board.

[adamziel]

Also, I know we're spending a lot of time here and I just want to acknowledge this is actually not an easy problem. Here's the list of scenarios we need to support:

• Running the unbuilt code directly via Node.js with a custom loader
• Running the unbuilt code directly via Bun (can be skipped for now since Bun is unsupported until we figure out the fs-ext situation)
• Building php-wasm/node as a reusable commonjs module and an esmodule for Node
• Building php-wasm/web as a reusable esmodule for the web
• Building the wp-playground/remote app that consumes the php-wasm/web package (built or unbuilt? I'm actually not sure)
  • Dev server (esbuild via Vite)
  • Production build (rollup via Vite)
• Building a downstream web or node app that imports php-wasm/web or php-wasm/node

Let's acknowledge all of them in the new issue and consider them in the discussion.

[adamziel]

#2299

[mho22]

Thanks! I'll fill in the issue with a summarized block of our comments.