Fix custom PHP binary deleted when binary path is the app root by simonhamp · Pull Request #117 · NativePHP/desktop

simonhamp · 2026-05-29T06:38:31Z

Summary

Fixes #115.

When NATIVEPHP_PHP_BINARY_PATH is set to a value that points at the project root (e.g. ./), native:build deletes the entire built app right before electron-builder packages it.

Root cause

In PrunesVendorDirectory::pruneVendorDirectory(), the configured binary package directory was interpolated naively into "app/{$binaryPackageDirectory}". When the value normalizes to a no-op (./, ., /, or a trailing /.), Path::join('/build', 'app/./') collapses to /build/app, so the prune step removes the whole app directory. The previous ! empty() guard did not catch this because strings like ./ are non-empty.

native:run was unaffected because it does not run the prune step.

Fix

Trim the configured path of surrounding dots, slashes, and whitespace, then skip removal entirely when nothing meaningful remains:

$binaryPackageDirectory = trim($this->binaryPackageDirectory(), "./\\ \t\n\r\0\x0B");
if ($binaryPackageDirectory !== '' && $filesystem->exists($this->buildPath("app/{$binaryPackageDirectory}"))) {
    $filesystem->remove($this->buildPath("app/{$binaryPackageDirectory}"));
}

Tests

Added tests/Build/PruneVendorDirectoryTest.php:

A normal custom binary directory (php-bin/) is still pruned.
A path that normalizes to the app root (./) no longer wipes the built app (regression test for Custom PHP binary gets deleted if in root of project #115).

Verified the regression test fails against the old code and passes with the fix. Full suite (114 tests) passes and Pint is clean.

🤖 Generated with Claude Code

Trim the configured binary package path before pruning so values that normalize to a no-op (./, ., /, trailing /.) no longer collapse to the app root and delete the entire built app. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

simonhamp · 2026-05-29T07:00:22Z

@bmckay959 would you mind trying this fix out when you get a chance?

bmckay959 · 2026-05-29T21:45:29Z

+| Tests
+|--------------------------------------------------------------------------
+*/
+it('removes the custom php binary package directory', function () use ($buildPath, $command) {


I'm a bit confused by this test....why do we want the custom binary removed? Feels like this test is introducing the bug the opposite direction? Basically, we don't want to delete the directory where the NATIVEPHP_PHP_BINARY points....or am I missing something?

Understandable confusion. The binary is moved to a location in the app bundle itself during build. So leaving it in the source directory would include it twice 👍🏻

PeteBishwhip · 2026-06-01T11:50:50Z

LGTM. Happy to merge pending passing CI.

PeteBishwhip · 2026-06-01T12:05:43Z

+
+    expect("$buildPath/app/php-bin")->not->toBeDirectory();
+    expect("$buildPath/app/index.php")->toBeFile();
+})->after(fn () => putenv('NATIVEPHP_PHP_BINARY_PATH'));


Looks like we're not currently able to support the ->after() method here.

bmckay959 · 2026-06-01T13:01:18Z

+    $command->pruneVendorDirectory();
+
+    expect("$buildPath/app/index.php")->toBeFile();
+    expect("$buildPath/app/bin/win/x64/php.exe")->toBeFile();


Based on @gwleuverink comment from above, do we actually want the /app/bin/win/x64/php.exe to be removed? So basically add the expect("$buildPath/app/bin")->not->toBeDirectory()?

I can confirm this solved the bug, but I believe this is now including ALL the PHP versions with the app this way(So my build goes from 127MB to 212MB). So, if the binary is getting copied to a 'special' place inside the electron app(?), then can we not have the /bin directory also removed?

Good point. I may have misinterpreted the test. I'll have a another look at it to confirm

gwleuverink · 2026-06-02T22:05:09Z

Dug into this a bit more. The PR fixes the crash, but Ben's spotted a second thing worth handling before we merge: the binaries are still ending up in the bundle, which is the 127 to 212MB jump he measured.

What's happening is php.js already extracts the one binary we need into build/php during the build, so the php-*.zip files left in the source tree are redundant copies. Right now the prune skips them when the path is ./, because we're removing the whole binary directory and there's no way to do that at the app root without taking the app with it. So the test ends up asserting they survive, when really we'd want them gone.

The wrinkle on my side is that we don't own bin/ outright, only {os}/{arch}/php-*.zip inside it, so I'd rather not remove the whole directory in case someone keeps other things there. Targeting just our archives covers both the size and that:

$binDir = $this->buildPath('app/'.$this->binaryPackageDirectory().'bin');
$filesystem->remove(glob("{$binDir}/*/*/php-*.zip"));

For ./ that resolves to app/bin/*/*/php-*.zip, which never collapses to the app root, so the trim/guard isn't needed anymore either. Tested it against a bin/ with our zips plus some unrelated files and only the zips go.

@simonhamp happy to push this onto the branch and tweak the test to match if you're good with it. Or we iterate the os/arch matrix instead of the glob if you'd prefer, though php-*.zip feels specific enough.

bmckay959 · 2026-06-02T22:35:05Z

Thanks for digging in more...as I mentioned in my initial issue, I'm still pretty new on the native php side of things, so I haven't fully wrapped my arms around the build process/order/when things get injected.

This feels like the right approach to me.

gwleuverink · 2026-06-02T22:48:01Z

Wouldn't have spotted it without your keen eye! @bmckay959 🙏🏻

Here's my suggested fix for review #121

This targets only the archives we manage, at their fixed bin/{os}/{arch}/php-*.zip location, never the containing directory. so if you happen to keep other files in ./bin those won't be wiped.

gwleuverink · 2026-06-03T07:28:26Z

The failing tests seem unrelated to this PR. Looks like the PendingRequest contract has broken backwards compat in a minor release. (Only happens with L13 - prefer-stable)

Laravel 13.13.0 tightened the HTTP client's header casting to throw on non-scalar values. The X-NativePHP-Secret header is set from config('nativephp-internal.secret'), which is null when NATIVEPHP_SECRET is unset, so every request blew up with an InvalidArgumentException. Casting to a string keeps null as an empty string, which the client accepts. This is what older Laravel versions did implicitly.

SRWieZ · 2026-06-05T07:03:25Z

Does it still work with absolute directory path ?

gwleuverink · 2026-06-09T08:49:15Z

Good point, relative paths work great today. Absolute paths are the snag: the path gets joined against the project root, so an absolute value doubles the base and php.js can't find the binary.

The docs example actually shows an absolute path, so at minimum that's a docs bug.

Two ways we could go: keep it relative-only and fix the docs, or also add support for absolute paths. I'm happy either way.

Btw, this was the case before this PR. How do we want to handle this?

SRWieZ · 2026-06-09T09:18:07Z

I always used absolute paths for my projects (per the doc recommendation), so I consider this a breaking change personally 😅

gwleuverink · 2026-06-09T09:23:48Z

Does the path point inside the project or outside? I just tried it in a new project and it did not work for me 🤔

Nothing here changes how the binary path is resolved, only change in this PR is how they are pruned (entire bin directory vs targeted binary inside bin) so this PR should not introduce anything breaking.

Anything I assessed in the path handling above was there before this change

Edit: give me a moment to verify again. just to be sure i'm not testing a stale branch or something

SRWieZ · 2026-06-09T09:33:49Z

inside, as a composer dependency, but that should work outside too, I believe

gwleuverink · 2026-06-09T09:46:56Z

can you confirm those projects are running v2?

SRWieZ · 2026-06-09T10:03:21Z

I checked, the project I had in mind is not on NativePHP Desktop v2 yet — it’s still using the old nativephp/electron package, locked at 0.8.7.

One of my other projects does use v2 tailscale-notifier. But I don't recall building with custom binaries recently.

simonhamp and others added 2 commits May 29, 2026 15:38

Fix styling

783b28e

simonhamp marked this pull request as ready for review May 29, 2026 07:00

gwleuverink approved these changes May 29, 2026

View reviewed changes

bmckay959 reviewed May 29, 2026

View reviewed changes

PeteBishwhip approved these changes Jun 1, 2026

View reviewed changes

Merge branch 'main' into fix-custom-php-binary-prune

1508380

PeteBishwhip requested changes Jun 1, 2026

View reviewed changes

bmckay959 reviewed Jun 1, 2026

View reviewed changes

gwleuverink mentioned this pull request Jun 2, 2026

prune only the php binary archives, not the whole bin dir #121

Merged

prune only the php binary archives, not the whole bin dir (#121)

bdc6011

gwleuverink added 2 commits June 3, 2026 09:40

Merge branch 'main' into fix-custom-php-binary-prune

cbc7877

gwleuverink requested review from PeteBishwhip and SRWieZ June 4, 2026 20:31

Uh oh!

Conversation

simonhamp commented May 29, 2026

Summary

Root cause

Fix

Tests

Uh oh!

simonhamp commented May 29, 2026

Uh oh!

bmckay959 May 29, 2026

Choose a reason for hiding this comment

Uh oh!

gwleuverink May 29, 2026

Choose a reason for hiding this comment

Uh oh!

PeteBishwhip commented Jun 1, 2026

Uh oh!

PeteBishwhip Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

bmckay959 Jun 1, 2026

Choose a reason for hiding this comment

Uh oh!

gwleuverink Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

gwleuverink commented Jun 2, 2026

Uh oh!

bmckay959 commented Jun 2, 2026

Uh oh!

gwleuverink commented Jun 2, 2026

Uh oh!

gwleuverink commented Jun 3, 2026

Uh oh!

SRWieZ commented Jun 5, 2026

Uh oh!

gwleuverink commented Jun 9, 2026

Uh oh!

SRWieZ commented Jun 9, 2026

Uh oh!

gwleuverink commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SRWieZ commented Jun 9, 2026

Uh oh!

gwleuverink commented Jun 9, 2026

Uh oh!

SRWieZ commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gwleuverink commented Jun 9, 2026 •

edited

Loading

SRWieZ commented Jun 9, 2026 •

edited

Loading