Passthrough $_ENV variables to artisan commands

[SRWieZ]

The problem

I got an artisan command that sends notifications

NativeAppServiceProvider.php

ChildProcess::artisan(
    cmd: 'app:forge-checker',
    alias: 'forge',
    persistent: true,
);

app:forge-checker artisan command

Notification::title('✅ Deployment finished')
    ->message($commit->watched_website->name . ' has been deployed')
    ->show();

‼️ This notification wasn't showing up because it was missing the NATIVEPHP_ env variables needed to send a request to the Electron part.

---

Current solution

A simple fix is to pass nativephp env variables like this.

ChildProcess::artisan(
    cmd: 'app:forge-checker',
    alias: 'forge',
    env: $_ENV,
    persistent: true,
);

---

Proposed solution

I would like to argue that ChildProcess::artisan() should pass through those variables by default.

I think it's expected that everything we write in the Laravel part should just work.
That's why I didn't pass $_ENV in php() or start() functions by default, only in artisan()

[gwleuverink]

Legend. We were about to debug some issues related to this. @SRWieZ to the rescue right on time 💪🏻

[gwleuverink]

Thanks so much for this PR @SRWieZ. I was experiencing some issues related to this last week concerning Custom Events dispatched from a artisan command child process.

Blocked this evening to get into the weeds but here you came along and found the issue 🥇 Absolutely stellar.

Simon and I had a little back & forth about it. Two takeaways:

1. You're absolutely right to only merge these variables in with the artisan method 👍🏻 The variables we need to dispatch Events, Notifications e.t.c. wouldn't make sense outside of the Laravel context. Good call
2. We could be more selective in what variables we're merging here. The superglobal has a lot in there we do not need

I think after some digging only these are absolutely nessecary:

• NATIVEPHP_RUNNING - So the EventWatcher & LogWatcher are registered
• NATIVEPHP_API_URL - To call the Electron endpoints
• NATIVEPHP_SECRET - To authorize the Electron endpoints

Arguably we'll need these too (though I'm not 100% certain about that, needs checking)

• NATIVEPHP_STORAGE_PATH
• NATIVEPHP_DATABASE_PATH

Would you be willing to add this to your PR? 🙏🏻

[simonhamp]

100% we'll need STORAGE_PATH and DATABASE_PATH too

[SRWieZ]

Thank you for your kind words, @gwleuverink!

Yeah, I took a shortcut with $_ENV.

We can limit it to NATIVEPHP_* variables but I noticed that in php.ts, which is responsible for launching the queue worker, we also send APP_ENV and APP_DEBUG alongside some ini configs.

callPhp is sending some ini configurations for the certificate; I feel like this is important too.

Let's make it even better.

What do you think about creating an endpoint for launching PHP processes (@simonhamp and @gwleuverink ) ?

[SRWieZ]

Oh, there is a function called getDefaultEnvironmentVariables.

I wonder why the scheduler and artisan serve are using this function but not the queue worker.

I don't see any valid reason.

[gwleuverink]

Oh, there is a function called getDefaultEnvironmentVariables.

You're right. It seems like we can get these variables & mix them in the electron-plugin childProcess.ts.

I thought the secret wouldn't be available there, but it is. In the state.ts file. Need to try this out, but passing it explicitly from the Process facade might not even be necessary.

I wonder why the scheduler and artisan serve are using this function but not the queue worker.

Peculiar indeed. In the QueueWorkerCommand I see a couple of environment variables being set, but not the secret. Which would suggest triggering notifications & custom events from the queue do not work at this time

[gwleuverink]

Which would suggest triggering notifications & custom events from the queue do not work at this time

Forget I said this. Just noticed the queue isn't started using the native QueueWorkerCommand, but with Laravel's default queue command.
@simonhamp Unrelated to this PR, but do you think we can delete that class?

[simonhamp]

@gwleuverink tl;dr: on balance, yes, I think we should remove it.

#346 references using the QueueWorkerCommand - so it seems some folks are using it even tho it's not a documented feature.

I don't think it make sense to have this as a separate command that is callable in this way. It tries to set up the context as if the NativePHP application (and thus the Electron environment) is running, but there are no guarantees that it is.

[SRWieZ]

callPhp is sending some ini configurations for the certificate; I feel like this is important too.

Let's make it even better.

What do you think about creating an endpoint for launching PHP processes (@simonhamp and @gwleuverink ) ?

Oh, there is a function called getDefaultEnvironmentVariables.

I wonder why the scheduler and artisan serve are using this function but not the queue worker.

I don't see any valid reason.

@simonhamp, what do you think about my proposal?

That will be summarised by:

• Adding a dedicated endpoint child-process/start/php or something similar
• Making that endpoint use getDefaultEnvironmentVariables and passing through ini settings or at least curl.cainfo and openssl.cafile
• Using the newly created endpoint in ChildProcess::php()

I can do it all in the Laravel part, but I think it makes more sense that it should be handled by the Electron part.

Bonus:

• Tidy up startQueueWorker to use getDefaultEnvironmentVariables

[simonhamp]

👍🏼 Yep, I like it, especially that it's mostly taking place within the Electron plugin... this keeps the Laravel side clean and clear.

It also keeps all of those environment variables that are defined by the environment (in this case, Electron), contained within the environment, rather than relying on them being passed backwards and forwards between Electron and Laravel.

I think it will be slightly more performant and reliable this way.

[gwleuverink]

Shoot I missed your convo here. I was tinkering around & PR'd a fix on the electron repo: NativePHP/electron#129

This approach still forwards those variables to every child process (not artisan only like before).

If you guys think it's worth it we can do that separate endpoint. There are so little variables forwarded that I'm starting to doubt if it's worth a separate endpoint though. What do you think?

[gwleuverink]

I was checking if it worked from the electron side and figured I'd just PR it. Hope I didn't get in the way with something you were working on @SRWieZ. Would love feedback if you have it!

[simonhamp]

Should we close this PR and move the convo over to the electron one?