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

IBX-8506: Disabled display_errors in Cloud #78

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

glye
Copy link
Contributor

@glye glye commented Jul 8, 2024

🎫 Issue IBX-8506

Related PRs:

Description:

The PHP config for Ibexa Cloud should default to not display errors in production. This is normally of no consequence as Symfony controls error handling, but in exceptional situations Symfony may fail and PHP's error handling take over. So to be on the safe side the default should be to not display errors.

For QA:

See Doc note below. And note that you will have to crash Symfony to reproduce the change.

Documentation:

See related doc PR (merged). Note also that these config files do not detect prod/dev mode, so display_errors will now by default be disabled regardless of mode, this should be noted in release notes.

Note

  • Test the solution manually
  • Provide automated test coverage
  • Confirm that target branch is set correctly

@glye glye marked this pull request as ready for review July 8, 2024 14:29
@glye glye added Ready for review Doc needed The changes require some documentation labels Jul 8, 2024
@glye glye requested a review from a team July 8, 2024 14:31
@mnocon
Copy link
Contributor

mnocon commented Jul 9, 2024

Hi! Please note that it expands on what's in #77 - I believe only one of these PRs should be merged (and this one seems to add more "sane defaults"? 🤔 )

@glye
Copy link
Contributor Author

glye commented Jul 9, 2024

Oh dang, I didn't know. WDYT @mateuszbieniek?

PHP says "It's strongly recommended to keep display_startup_errors off, except for debugging." And I think it's good to have a comment about why we're setting them.

@mateuszbieniek
Copy link
Contributor

mateuszbieniek commented Jul 9, 2024

I think you are right and I'm OK with dropping my PR in favor of this one :) (also checked, and yes, in p.sh envs this is also set to "on" by default)

@glye
Copy link
Contributor Author

glye commented Jul 9, 2024

Thanks, good to know, that's not a great default on their side.

@glye glye merged commit 46e7d79 into 1.0 Jul 30, 2024
8 checks passed
@glye glye deleted the ibx8506_disable_display_errors_cloud branch July 30, 2024 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc needed The changes require some documentation Ready for QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants