magicbug / Cloudlog

Web based amateur radio logging application built using PHP & MySQL supports general station logging tasks from HF to Microwave with supporting applications to support CAT control.
http://www.cloudlog.co.uk
MIT License
461 stars 190 forks source link

Question: Development mode enabled by default? #2696

Closed bgritzmacher closed 11 months ago

bgritzmacher commented 12 months ago

Is there a particular reason developer mode is hardcoded to be enabled? The information provided by it is not particularly useful to end-users unless you are actively debugging the application.

Best practices would be to use the CodeIgniter default that has been commented out - which is to set server variables to control its status, while assuming production mode by default. It is then up to the developer to make sure that their environment is setup correctly to do development tasks (even if it is just hardcode it for themselves.)

This is more annoying in Docker which requires one of numerous possible steps to disable:

And then this has to happen any time you down and up the container, as the changes get lost from the writable layer (unless you build a local image with the change in it.)

All for something that should be default, and able to be controlled at docker runtime, without modifying files within the container.

Everything I've seen within the code seems to indicate that this is an intentional choice, but I do not understand why.

iu2frl commented 11 months ago

I see the line you are mentioning, it is:

#define('ENVIRONMENT', isset($_SERVER['CI_ENV']) ? $_SERVER['CI_ENV'] : 'development');
define('ENVIRONMENT', 'development');
bgritzmacher commented 11 months ago

Correct. The default way CodeIgniter sets the ENVIRONMENT variable via the $_SERVER['CI_ENV'] variable was commented out and just hardcoded to always be "development." I could argue that CodeIgniter's defaulting to "development" when CI_ENV isn't set isn't preferable either, but this isn't CodeIgniter's support 😉

A further suggestion to improve on your pull request and make it more complete for Docker support would be to add support for setting the CI_ENV value at runtime (either in docker compose in an environment section or by using docker with the -e switch.) This could then get set using SetEnv in a config file that gets loaded by apache in the /etc/apache2/conf-enabled folder.

As an additional note for supporting users with going to using the production setting by default - this does disable PHP errors from getting displayed out to the user's browser, and will result in a "white screen of death" on critical failures, where error exception handling does not exist. Ideally, suppressed errors should still get logged somehow for developer review, but I'm not familiar with CodeIgniter to know if it does this by default (cursory glance through the config file shows $config['log_threshold'] gets set to 0 by default, which means no logging is written - I'm changing this on my instance to see if it has the effect I think it should have.)

So, this could have the negative consequence of causing ambiguous support requests along the lines of "I did X, and just got a white screen." resulting in having to instruct them how to switch to developer mode and then doing that process the same way again to reproduce the issue and get the errors from PHP. At least, until proper error logging and handling gets implemented to give nice errors to users 😉 (at least, I would make that a goal as a result of this.)

iu2frl commented 11 months ago

I have to admit the development mode saved my cheeks multiple times when doing upgrade procedures (wrong database tables and so on)

bgritzmacher commented 11 months ago

It is a fine line to providing error messages to users that are useful and actionable, while still being helpful to maintainers and those that provide support.

Catching and gracefully handling fatal errors would be a significant undertaking that would take time, I'm sure. Maybe I'm just used to the framework I use having graceful handling of errors built-in with logging enabled by default 🤷‍♂️

Maybe a good middle ground for the time being is to leave it in development mode for the time being, but make showing the profiler user configurable? I'm not sure what development mode controls within CodeIgniter and the pros and cons of leaving it in that mode, but I do know in other frameworks, similar settings control things like cache expiration timings - making them shorter, or clearing them on errors, in addition to having increased error details.

Comparing the CodeIgniter 3 repo to this one, it does look like the PHP error template had the entire section for showing a backtrace has been removed entirely, which would normally be one of the biggest concerns with leaving it in development mode.

magicbug commented 11 months ago

Due to a range of decisions, this will stay the default.

Currently, I'm not keen to change this as it will break 100% of installs requiring them to run Git commands, might be prepared todo it further down the road but not yet.

nosbig commented 4 months ago

@magicbug : I have a potential workaround for the folks who want to have the auto-updates via git and have the system in "production" mode during operation...

In your step 1 of the post-install steps, you provide a short script to perform a git pull and update some permissions. You could use 'sed' to perform in-place replacements of the "ENVIRONMENT" variable inside of index.php.

Here's my version of the update script, note the 3rd and last lines. This performs an in-place replacement for the "development" and "production" keywords such that when a git pull is performed, it can cleanly pull the repo. Otherwise, the script will continuously fail because local changes aren't committed or stashed before pulling the latest commits.

`#!/bin/bash cd /var/www/cloudlog sed -i "s/define('ENVIRONMENT', 'production')/define('ENVIRONMENT', 'development')/" index.php git pull

chown -R www-data:www-data /var/www/cloudlog/application/config/ chown -R www-data:www-data /var/www/cloudlog/backup/ chown -R www-data:www-data /var/www/cloudlog/updates/ chown -R www-data:www-data /var/www/cloudlog/uploads/ chown -R www-data:www-data /var/www/cloudlog/images/eqsl_card_images/

sed -i "s/define('ENVIRONMENT', 'development')/define('ENVIRONMENT', 'production')/" index.php `