Do not force error display

[Berdir]

Describe the bug

The enforced error hide/show is currently not working on projects that don't use master because onProduction() actually just hardcodes master and doesn't check the environment type.

Even with that fixed, I think it's not a good idea to have that. Usually you don't want errors to show either when clients are testing and there might even be a case where you need to enable it on production.

I'd suggest to either remove the whole section or just keep them as commented out examples or so.

Include some logs

No logs

Reproducing

Have a project with a main branch, do something that triggers a php notice/error.

Expected behavior

No error is shown on production.

Your environment

not relevant

Screenshots

No response

Additional context

No response

[flovntp]

sorry, will need to rework on it, i went in the wrong direction.

[gilzow]

Display errors should already be disabled via the php.ini file included in the template: https://github.com/platformsh-templates/drupal10/blob/master/php.ini#L2

@Berdir can you share a link to your respitory?

[Berdir]

I'm talking about https://github.com/platformsh-templates/drupal10/blob/71632ea554699a4475df654943c0f0825ee45281/web/sites/default/settings.platformsh.php#L31C7-L31C7

[gilzow]

Ok, now I see you're talking about Drupal's display errors setting, not PHP.

The config reader is only hardcoded for the master branch if the project is running on our Dedicated service instance; everything else retrieves the value in the environmental variable PLATFORM_BRANCH which will set to the branch name of the environment. It's hardcoded to master for our dedicated offering because in that service only the master branch can be production.

Please see this line in the config reader: https://github.com/platformsh/config-reader-php/blob/09982f28b0831401dc62a34fa3edceda76efaf19/src/Config.php#L440

[Berdir]

Both master and production are hardcoded, depending on dedicated or not. And according to open issues in the reader project, it's both broken on dedicated gen 3 (platformsh/config-reader-php#20) and non-master production branch names (platformsh/config-reader-php#21). Both of those should be fixed there.

But that's not even my point. I'm saying this shouldn't be hardcoded by default, neither force showing nor force hiding them. It could be an example,. but it shouldn't be on by default. It very much depends on your workflows and use cases, you likely users at least on staging but also development branches testing stuff and they'll just be confused by things like php notices/warnings, especially on verbose, which typically shows a huge wall of a backtrace call.

[gilzow]

Both master and production are hardcoded, depending on dedicated or not. And according to open issues in the reader project, it's both broken on dedicated gen 3 (platformsh/config-reader-php#20) and non-master production branch names (platformsh/config-reader-php#21). Both of those should be fixed there.

I stand corrected. I had assumed the config-reader had been updated to utilize the PLATFORM_ENVIRONMENT_TYPE environmental variable to determine onProduction() but upon further inspection, it has not. A PR is underway to correct that issue.

I'm saying this shouldn't be hardcoded by default, neither force showing nor force hiding them.

That decision predates me, though the logic behind it seems reasonable since best practice is to hide debugging information in production, but display it non-production environments. I will admit though hiding it for all environments that are on dedicated instances is odd. I will have to ask around to see if there's information on why that decision was made.

Regardless, once you create a project from this template they are disconnected so you are more than free to edit that file to however you see fit in your specific project.