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

feat: rename DefaultOs to Linux #488

Merged
merged 1 commit into from Aug 6, 2023
Merged

feat: rename DefaultOs to Linux #488

merged 1 commit into from Aug 6, 2023

Conversation

kesselb
Copy link
Collaborator

@kesselb kesselb commented Aug 1, 2023

DefaultOs was actually the implementation for Linux.

@MichaIng
Copy link
Member

MichaIng commented Aug 2, 2023

So depending on the detected OS, the app tries to gather system info via common commands/APIs. And the DefaultOs is basically a dummy where no external commands/APIs are used?

Definitely better than before with Linux actually named Linux and a universally compatible fallback/default OS, which additionally can be used when external commands/APis are explicitly not wanted or available. But I find the name "default" not very intuitive code-wise and for the CLI/setting.

What about calling it DummyOs or SkeletonOs or something which better expresses what it actually is? Not sure if there are cases where PHP_OS can be wrong, but for this, it could be useful to have the setting changed to e.g. enforce_os_type which then takes linux, freebsd or dummy to maintain the same effect force_default_os currently has. Otherwise, probably rename it to disable_system_calls or something like that, which better explains what it is for without the need to read the README?

@joshtrichards
Copy link
Member

joshtrichards commented Aug 2, 2023

  • Moving the Linux specific stuff out of the DefaultOs class to a dedicated Linux probably makes sense for clarity since internals are so OS specific
  • Adding a flag to enable the user to say they're in a restricted OS or want to limit the OS info exposed seems a simple way to address the ongoing limited filesystem access and/or exec access issues which we're unlikely to 100% ever overcome just do to the nature of the bits we need to gather (and might also help reduce some of the work needed to be made by forkers that I've seen where hosters basically tear our a lot of the OS stuff)

Suggestion:

  • Maybe rename the DefaultOs class - which isn't really default in that case anymore - to something else. Ideas: BaseOs, BasicOs, or BarebonesOs, BareOs or MinimalOs? And then change the override flag name as appropriate to match.

@kesselb
Copy link
Collaborator Author

kesselb commented Aug 2, 2023

Thank you very much for your feedback 🙌

I couldn't come up with a better name, so I stayed with DefaultOs 🤣

Not sure if there are cases where PHP_OS can be wrong

Afaik PHP_OS is set on compilation: https://github.com/php/php-src/blob/6f6fedcb46a27cd3530f0babc9b03ce4598f9eab/configure.ac#L1498-L1499

(and might also help reduce some of the work needed to be made by forkers that I've seen where hosters basically tear our a lot of the OS stuff)

Good to know, I wasn't aware.

@MichaIng
Copy link
Member

MichaIng commented Aug 2, 2023

Afaik PHP_OS is set on compilation

Indeed, no point then to allow enforcing anything else than the dummy.

Two concurrently written feedback comments with the same point(s) about the naming 💯. "dummy", "skeleton", "base", "basic", "bare", "minimal" ... "fallback"? If it acted like a template, "base" would fit best. But here it is an independent class which just implements the same interface, so "dummy" or "minimal" probably fit better.

The setting could then be called force_dummy_os or force_minimal_os, but it does not necessarily need to contain the class name? I like the word "restricted" in this context, so restrict_external_calls or restricted_mode or something like that would be another idea.

@kesselb kesselb force-pushed the default-os branch 2 times, most recently from 9b06183 to 080fffe Compare August 4, 2023 16:04
@kesselb
Copy link
Collaborator Author

kesselb commented Aug 4, 2023

Thanks for your valuable input 👍

Copy link
Member

@MichaIng MichaIng left a comment

Choose a reason for hiding this comment

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

IMO, for admins as well as for developers it is nicely understandable now.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@kesselb kesselb merged commit dcb09a8 into master Aug 6, 2023
25 checks passed
@delete-merged-branch delete-merged-branch bot deleted the default-os branch August 6, 2023 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants