Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow to use custom runtime #57

Merged
merged 5 commits into from
Aug 27, 2024
Merged

allow to use custom runtime #57

merged 5 commits into from
Aug 27, 2024

Conversation

allan-simon
Copy link
Contributor

close #56

@allan-simon
Copy link
Contributor Author

now it should be good @mnapoli

@DaMitchell
Copy link

Is there anything stopping this from being added?

@mnapoli
Copy link
Member

mnapoli commented May 13, 2022

@DaMitchell what would be your use case?

@DaMitchell
Copy link

@mnapoli We have a set of parameters we store in Parameter Store that we want to pull down and make available to the application, and we are testing that currently by extending the Symfony runtime wrapping the runner and then loading them in via DotEnv.

Something like what @allan-simon has included in #56.

@mnapoli
Copy link
Member

mnapoli commented May 15, 2022

Got it. Is it something that could be implemented in Kernel::boot() or a similar method?

Is there any reason to implement it in a custom runtime? (I'm trying to be exhaustive and figure out if this is really something worth the maintenance effort)

@allan-simon
Copy link
Contributor Author

in my case it was because the env and debug attribute used to construct the kernel were overwritten by the dotenv value , and so Kernel::boot is already too late

@t-richard
Copy link
Member

@allan-simon maybe it didn't worded in boot because of something similar to what I found in this PR #37

That said, adding support for any runtime is out of the scope of this bridge for now.

I fear that if we support that, we end up with people wanting to use runtimes for roadrunner, reactphp or swoole in Lambda for example and that may not work as expected in a Lambda environment.

That being said, if that's a real blocker, we could allow to specify a runtime but only if it's an instance of SymfonyRuntime.

That way we support use cases like yours but constrain it to something we know works and that we can maintain.

@allan-simon
Copy link
Contributor Author

@t-richard I've checked, and I think here it was not the issue, but if I remember my investigation (i'ts been a while so I may recall incorrectly) it was really a "by design" , as the Kernel constructor already expect to receive env variables , which mean they need to be ready by the time you instantiate it.

I understand that opening to any kind of custom runtime may widen too much the scope and make it unbearable to support in term of human time, so your proposition of limiting it to sub-class of SymfonyRuntime sounds like a good compromise to me.

@DaMitchell
Copy link

@mnapoli I understand trying to be exhaustive with a requirement like this. We have and internal framework we use across multiple projects so I was just looking for a method that would require the least amount of project level change and moving it into the runtime "model" seemed to fit that.

I will take a look at doing something in Kernel::boot() as an alternative path if this isn't a change that makes it in.

Limiting it to an instance of the SymfonyRuntime would also support our case.

Copy link
Collaborator

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate this.

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, to piggy-back on the comments above let's merge this with a check that the runtime extends SymfonyRuntime

@allan-simon
Copy link
Contributor Author

sure no problem

@allan-simon
Copy link
Contributor Author

@mnapoli done, I've tested locally by monkeypatching my vendor

@Nyholm
Copy link
Collaborator

Nyholm commented Aug 27, 2024

I've rebased the PR and fixed the CI issues

@mnapoli mnapoli merged commit cce0757 into brefphp:master Aug 27, 2024
7 checks passed
@Nyholm
Copy link
Collaborator

Nyholm commented Aug 27, 2024

Thank you for the PR and thank you for merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

How to use with a custom runtime (that also extends SymfonyRuntime)
5 participants