Make WordPress Core

Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#64925 closed defect (bug) (fixed)

Build/Test Tools: PHP is required locally to build with the new Gutenberg build process

Reported by: gaisma22's profile gaisma22 Owned by: desrosj's profile desrosj
Milestone: 7.0 Priority: normal
Severity: normal Version: 7.0
Component: Build/Test Tools Keywords: has-patch
Focuses: Cc:

Description

When running npm run build:dev or npm run dev on a fresh clone of
wordpress-develop, the build fails silently if PHP is not installed
locally on the developer's machine.

The Gutenberg build process reads .min.asset.php files to generate
script-loader-packages.php. Without local PHP, this step throws
"Unknown error while reading PHP source file" for every asset file
and generates empty package files, causing the block editor to fail
to load in the local Docker environment.

Steps to reproduce

  1. Clone wordpress-develop on a machine without PHP installed locally
  2. Run npm install
  3. Run npm run build:dev
  4. Run npm run env:start and npm run env:install
  5. Navigate to wp-admin/post-new.php

Expected result

The block editor loads correctly.

Actual result

The block editor fails to load with PHP notices about missing
dependencies (wp-a11y, wp-i18n, wp-blocks). The root cause is that
script-loader-packages.php is generated with 0 packages because
local PHP is not available to parse the .min.asset.php files.

Change History (19)

#1 follow-up: @dmsnell
3 months ago

See #64393

@gaisma22 given that WordPress is a PHP project, do you find it unreasonable to require PHP? There’s likely more in the build change that will require PHP to replace some unreliable RegExp functions that were meant to parse PHP.

I’m asking because I am a bit surprised at the request; I would think that PHP is a foundational requirement for just about any Core development.

#2 @SirLouen
3 months ago

  • Keywords needs-patch added
  • Version set to trunk

Thanks @gaisma22 for reporting (my WP Credits mentee).

I've been doing a bit of research, and it appears that the problem started with [61873]/#64393

By default, the wordPress-develop, going through Docker, should not need PHP by default as it fully runs under the Docker premises.

With this change, it's forcing PHP in the local machine for building and for the first time.

cc @desrosj

Edit: btw @dmsnell is faster than billy the kid answering

Last edited 3 months ago by SirLouen (previous) (diff)

#3 in reply to: ↑ 1 @SirLouen
3 months ago

Replying to dmsnell:

There’s likely more in the build change that will require PHP to replace some unreliable RegExp functions that were meant to parse PHP.

Not much code required, but forcing PHP for first time on the host is a big deal.

FWIW, not too far away, it was added a npm script ([60803]) to avoid need composer locally also.

#4 follow-up: @dmsnell
3 months ago

Thanks @SirLouen for noting Docker — I have never used that for Core development so that’s probably why I didn’t think about it.

Would the build not run inside Docker? What about the new process requires PHP locally vs inside the container? Is it because the build change pushed code into post-install hooks?

forcing PHP for first time on the host is a big deal

Not sure I follow. I think the first time was over twenty years ago 🙃


Trying to consider whether this is a worth closing and moving into #64393 or keep separate.

#5 in reply to: ↑ 4 @SirLouen
3 months ago

Replying to dmsnell:

Not sure I follow. I think the first time was over twenty years ago 🙃

I mean, this is the wordpress-develop Docker workflow

Would the build not run inside Docker?

Basically you don't really need anything but NPM/Node according to the building instructions (not even, Docker itself, according to the last recent update, any container environment should do the trick). The build itself runs outside Docker.

It's important to keep this safe for new contributors, or updating that PHP is going to be required from now on.

Here we have two options:

  • Implement the regex logic (a small tokenizer, not a great deal; there are even some JS libs that also do this).
  • Update the Getting Started guide to add the PHP requirement explicitly.

cc @johnbillion

Last edited 3 months ago by SirLouen (previous) (diff)

#6 @SirLouen
3 months ago

  • Keywords 2nd-opinion added

This ticket was mentioned in Slack in #core-editor by sirlouen. View the logs.


3 months ago

#8 follow-up: @desrosj
3 months ago

Thanks for the report @gaisma22!

I’d like to avoid adding a PHP requirement for the time being. I think it’s something to consider in the future, but given the amount of build script changes this release, I’d prefer to get things stable before adding a new requirement.

I’m curious what your setup is, though. You’re the first person to report this and the change has been in place for about 2 weeks.

#9 in reply to: ↑ 8 @gaisma22
3 months ago

Replying to desrosj:

I’m curious what your setup is, though. You’re the first person to report this and the change has been in place for about 2 weeks.

I'm running Ubuntu 24.04 LTS with Node.js v20.20.1 and Docker 29.0.3. I did a fresh clone of wordpress-develop and followed the standard setup steps without installing PHP locally, since the Docker workflow never required it before.

The block editor failed to load with PHP notices about missing dependencies. @SirLouen (my WP Credits mentor) spotted the issue - he noticed a line in the build script calling PHP and asked me to run php -v, which confirmed PHP was not installed.

Installing php-cli resolved it immediately.

This ticket was mentioned in PR #11335 on WordPress/wordpress-develop by @desrosj.


3 months ago
#10

  • Keywords has-patch added; needs-patch removed

This removes the php command in the build script introduced in r61873.

Trac ticket: Core-64925

## Use of AI Tools

AI assistance: Yes
Tool(s): Claude Code
Model(s): Sonnet 4.6
Used for: Used to create the first draft of this PR.

#11 @desrosj
3 months ago

  • Keywords 2nd-opinion removed
  • Milestone changed from Awaiting Review to 7.0

I've attached a PR that removes the requirement of running a php command.

Please have a look and let me know if it works well.

It's worth noting that my hope is for this to be a temporary solution. While looking into this, it seems to me that the ideal solution is for the registry.php file to be updated to include this information and be usable in both wordpress-develop and gutenberg, or some other solution that would avoid having to parse this file in such a way. But that's something to explore after 7.0.

#12 @sabernhardt
3 months ago

@r1k0 mentioned the 'Unknown error while reading PHP source file' error messages in a Slack thread.

I still have the errors in my SVN installation on Windows (using WAMP) today, resulting in missing dependencies such as wp-a11y and wp-i18n on most admin pages and an unusable editor. PR 11335 resolves the build errors, and the script dependencies are available again. The post and site editors load their scripts again too (without PHP or console errors).

@dmsnell commented on PR #11335:


3 months ago
#13

I think when I added the original code I saw that the PHP files included an if ( ! defined( 'ABSPATH' ) ) { die(); } check. it’s unclear from the code what the purpose of fromString does, and how it parses the PHP code.

with require file_get_contents( 'php://stdin' ); we at least know that we expect the included PHP to return a value, and that value will be serialized.

I had to perform a couple web searches to find the php-array-reader and then to find the php-parser node module it depends on to begin to get a faint picture of what it’s doing and how it’s written.

---

I’m still perplexed by how this is a problem, and what makes this particular build step distinct from the other basic requirements of PHP. is running all of this in npm ci via a post-install hook the actual problem? because it seems like this should be doable within the Docker image, if that’s what someone is doing, or plainly via php if running outside of Docker.

While I don’t want to delay any changes that are necessary, I’m just wanting to double-check our values when introducing more npm dependencies to booting the repo, running more JS code to recreate a PHP parser when one is available to us already, and where we know we need php anyway.

#14 @manhar
3 months ago

I was able to reproduce this issue.

It seems like PHP is still needed during the build process when using npm ci.

This is not what I expected before.

Can we please clarify if PHP is supposed to be a dependency or if there is another way to handle it?

I would like to know if this is, on purpose or if it can be changed.

@desrosj commented on PR #11335:


3 months ago
#15

I’m still perplexed by how this is a problem, and what makes this particular build step distinct from the other basic requirements of PHP. is running all of this in npm ci via a post-install hook the actual problem? because it seems like this should be doable within the Docker image, if that’s what someone is doing, or plainly via php if running outside of Docker.

While I don’t want to delay any changes that are necessary, I’m just wanting to double-check our values when introducing more npm dependencies to booting the repo, running more JS code to recreate a PHP parser when one is available to us already, and where we know we need php anyway.

While it's true that WordPress cannot be run without PHP, the build script has never required PHP to be present to _prepare_ wordpress-develop for use. The only requirement to build wordpress-develop was Node.js/npm.

While it's true that it could be run within the Docker image, many contributors do not use the local Docker environment at all, and requiring someone to pull large containers (the latest image for just wordpress/php is nearly 350MB) just to build the code is not ideal. Running commands like this within the containers also tends to be much slower.

This ticket was mentioned in Slack in #core by desrosj. View the logs.


3 months ago

#17 @desrosj
3 months ago

  • Owner set to desrosj
  • Resolution set to fixed
  • Status changed from new to closed

In 62157:

Build/Test Tools: Remove PHP requirement for the build script.

In [61873], the build script started failing in some environemnts due to logic that added a requirement for php-cli.

While WordPress itself cannot be run without PHP, the build script has never required PHP to be present to prepare wordpress-develop for use. This adjusts the relevant code to make use of the php-array-reader package instead.

Reviewed by peterwilsoncc.

Props dmsnell, peterwilsoncc, gaisma22, SirLouen, sabernhardt, manhar.
Fixes #64925. See #64393.

This ticket was mentioned in PR #11367 on WordPress/wordpress-develop by @desrosj.


3 months ago
#18

This removes an accidental change in r62157.

Trac ticket: Core-64925.

## Use of AI Tools

#19 @desrosj
3 months ago

In 62158:

Build/Test Tools: Remove unintentional change in [62157].

This removes a change that was unintentionally included in the previous commit.

Reviewed by peterwilsoncc.

Unprops desrosj.
Props peterwilsoncc.
Fixes #64925. See #64393.

Note: See TracTickets for help on using tickets.