"PR #38 Add missing module checks" introduces breaking change

[pamtbaau]

PR #38 has added a check for the intl PHP module. However, this is creating a breaking change.

// Check for PHP intl library
$required = Grav::instance()['config']->get('system.intl_enabled');
$this->check_php_module('intl', $required, 'intl (Internationalization Functions)');

It checks for variable system.intl_enabled, which is by default true, which means each and every installation now requires module intl.

According the docs, module intl is not a requirement.

On Discourse, users have reported issues about a missing intl module.

[Rotzbua]

It is not a breaking change. Your config is wrong, because if intl_enabled: true then the intl extension is required.

intl_enabled: | Special logic for PHP International Extension (mod_intl)

See doc: https://learn.getgrav.org/17/basics/grav-configuration

[pamtbaau]

Your config is wrong

Sorry, but that config comes default by Grav. Variable intl_enabled is true by default in system/config/system.yaml: https://github.com/getgrav/grav/blob/fa56984dc3317e7f3d7946b7f7231d4ea4d36080/system/config/system.yaml#L12

This setting has been added already in 2017: getgrav/grav@83bfc1f

The config hasn't changed, but the module checks has...

[Rotzbua]

Funny thought: the default configuration may not have been flawless back in 2017?

[pamtbaau]

At first sight, I don't think there is anything wrong with the config. But I'm not sure if I can think of a use case at the moment though.

It never created any issue because Grav only uses it in two places.

In both cases, the setting doesn't create an issue. The setting is only used to enable(default)/disable intl aware sorting when module intl is available.

if (extension_loaded('intl') && $this->grav['config']->get('system.intl_enabled')) {

I don't see a reason why people would disable intl sorting, but that's another discussion...

My conclusion: Checking for the existance of the intl module isn't needed. It is optional.

[rhukster]

as far as i'm concerned intl is optional and should not be checked as such.

[pamtbaau]

A patch reversing the intl module check should then be created I guess?

[rhukster]

Would be great 😉

[Rotzbua]

In both cases, the setting doesn't create an issue. The setting is only used to enable(default)/disable intl aware sorting when module intl is available.

if (extension_loaded('intl') && $this->grav['config']->get('system.intl_enabled')) {

Wrong interpretation. The execution of grav does not fail, if intl module is not available. The missing functionally does does cause issues.

My conclusion: Checking for the existance of the intl module isn't needed. It is optional.

If intl_enabled: false => intl not needed
If intl_enabled: true => intl is required for functionally but grav core does not crash if it is not available.

The current problem is, that you can set intl_enabled: true but it does not work if intl is not available. There is no warning or log from grav core!

[pamtbaau]

Wrong interpretation.

That depends on the intention of the developer and is a discussion you should have with the developer himself.

In the meantime, the extra check is not backwards compatible and throws an error. The default value has always been true, it has never thrown an error, and now it does... That should be fixed first because users have reported their websites throw errors.

[pamtbaau]

@Rotzbua, Any chance you will create a patch? See also this issue #47 in which Andy calls it a bug.