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

Check ENV variable first before doing a FS stat in settings.ddev.php #3017

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Check ENV variable first before doing a FS stat in settings.ddev.php #3017

merged 1 commit into from
Jun 1, 2021

Conversation

markhalliwell
Copy link
Contributor

@markhalliwell markhalliwell commented May 20, 2021

Very minor nit, but no need to check whether the file is readable if it's not a DDEV environment.

@gitpod-io
Copy link

gitpod-io bot commented May 20, 2021

Copy link

@plach79 plach79 left a comment

Choose a reason for hiding this comment

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

Makes sense to me

@rfay
Copy link
Member

rfay commented May 20, 2021

Could you please say why this matters?

@markhalliwell
Copy link
Contributor Author

Logically, it just doesn't make a lot of sense to access the physical system when it can first check the environment variable which is already loaded in memory.

From a performance standpoint, I get that this isn't going to matter much. However, this code is injected into settings.php which is something that is still executed in a production environment. While one tiny is_readable call isn't going to do much, it also doesn't hurt avoiding a completely unnecessary stat() on each request when it doesn't have to.

@rfay
Copy link
Member

rfay commented Jun 1, 2021

I guess this is OK. One note though: settings.ddev.php is never intended to exist in a normal production environment. It's gitignored and shouldn't show up there at all.

@rfay rfay changed the title Check ENV variable first before doing a FS stat Check ENV variable first before doing a FS stat in settings.ddev.php Jun 1, 2021
@rfay rfay merged commit 40f9dbb into ddev:master Jun 1, 2021
@markhalliwell
Copy link
Contributor Author

Thanks for the merge 😄 I am aware that settings.ddev.php shouldn't/doesn't get committed. But it would still check for one, even on prod because this was autogenerated into settings.php which is committed.

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.

None yet

3 participants