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

Turn on error display for local server #177

Merged
merged 3 commits into from
Jul 6, 2020
Merged

Conversation

pdewouters
Copy link

Screenshot 2020-07-06 at 11 13 45

Copy link

@hm-linter hm-linter bot left a comment

Choose a reason for hiding this comment

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

Linting failed (1 error).

1 notice occurred in your codebase, but none on files/lines included in this PR.

@pdewouters pdewouters requested a review from joehoyle July 6, 2020 10:21
@joehoyle
Copy link
Member

joehoyle commented Jul 6, 2020

It looks like this will mean display_errors will no longer be on for non-cli (as that line of code got removed?)

Could we just set this for cli & fpm by setting the value in https://github.com/humanmade/altis-local-server/blob/master/docker/php.ini?

@pdewouters
Copy link
Author

@joehoyle are the acceptance criteria here outdated then? #123

@joehoyle
Copy link
Member

joehoyle commented Jul 6, 2020

Ah thanks for linking @pdewouters -- could you link the original issues in subsequent PRs too to give extra context.

It looks like that acceptance criteria actually came from @roborourke! He edited my comment :D

It does actually looked like we want to rmove display_errors from the web context per @rmccue 's response:

FWIW, it's already turned on in the local-server bootstrap file, but we should actually remove this so that errors aren't displayed in a HTML context:

I guess that's fine, though I'm not entirely sure why we don't want to show errors in the web context! In which case anyway, judging by the notes on #123 then this PR is good to go.

@joehoyle joehoyle merged commit e18a8de into master Jul 6, 2020
@joehoyle joehoyle deleted the 123-display-errors branch July 6, 2020 14:32
@rmccue
Copy link
Member

rmccue commented Jul 6, 2020

I think this should be set in the .ini rather than in wp-config FWIW

@joehoyle
Copy link
Member

joehoyle commented Jul 6, 2020

@rmccue that will affect both web and cli though, which you comment specifically recommends against I think?

@rmccue
Copy link
Member

rmccue commented Jul 6, 2020

Yeah; I think local-server might need to distinguish the two SAPIs and separate the configuration, the same way that e.g. Ubuntu PHP does.

@roborourke roborourke mentioned this pull request Jul 7, 2020
2 tasks
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.

3 participants