MarcusBarnes / mik

The Move to Islandora Kit is an extensible PHP command-line tool for converting source content and metadata into packages suitable for importing into Islandora (or other digital repository and preservations systems).
GNU General Public License v3.0
34 stars 11 forks source link

SYSTEM required in config.ini always #412

Closed whikloj closed 7 years ago

whikloj commented 7 years ago

You must have a [SYSTEM] section in your config.ini due to https://github.com/MarcusBarnes/mik/blob/master/mik#L83

But if you have a timezone set, this section can be empty. ie. mine is

[SYSTEM]
; what ever

Probably nicer to see if you don't have it set in PHP first then try to load the SYSTEM section.

mjordan commented 7 years ago

The timezone setting was the first but is not the only possible setting in the [SYSTEM] section, there's also https://github.com/MarcusBarnes/mik/wiki/Cookbook:-Running-MIK-on-Mac-OS-X. It would be good to come up with a way of not showing the PHP warning if the section is not present.

mjordan commented 7 years ago

Running --checkconfig already does some foo here: https://github.com/MarcusBarnes/mik/blob/master/src/config/Config.php#L34. Maybe we could add the timezone check here too?

whikloj commented 7 years ago

@mjordan is there another reason to re-parse the config there (line 83)?

Could we do something like:

# $system_settings = parse_ini_file($configPath, true)['SYSTEM']; # DELETE THIS
if (ini_get('date.timezone') == null || 
   (isset($settings['SYSTEM']) && isset($settings['SYSTEM']['date_default_timezone']))
) {
whikloj commented 7 years ago

Full block that seems to work for me

# $system_settings = parse_ini_file($configPath, true)['SYSTEM'];
if (ini_get('date.timezone') == null || (
        isset($settings['SYSTEM']) && isset($settings['SYSTEM']['date_default_timezone']))
) {
    if(isset($settings['SYSTEM']) && isset($settings['SYSTEM']['date_default_timezone'])){
        $date_timezone = $settings['SYSTEM']['date_default_timezone'];
        date_default_timezone_set($date_timezone);
    } else {
        // Use a default time-zone.
        date_default_timezone_set('America/Vancouver');
    }
}
mjordan commented 7 years ago

@whikloj nice. It would be good to break out this check, and any others we do now (like the verify_ca setting) and in the future into a utility function we could call from within mik and anywhere else it might be needed like post-write hook scripts. We've already got a couple "utilities" in src/utilities. @MarcusBarnes any thoughts? I agree with @whikloj that the [SYSTEM] section shouldn't need to be in the .ini unless it's needed.

whikloj commented 7 years ago

The only thing that this misses, is if you haven't set the date_default_timezone you get Vancouver by default. In past you get an Exception, but I think you could probably stick the Exception in instead of the date_default_timezone_set('America/Vancouver');

MarcusBarnes commented 7 years ago

Addressed in pull-request https://github.com/MarcusBarnes/mik/pull/416 (merged with commit https://github.com/MarcusBarnes/mik/commit/0bd2a7786316e11ff992fecafdc401bf9beccef0). Thank you @whikloj.