laminas / laminas-modulemanager

Modular application system for laminas-mvc applications
https://docs.laminas.dev/laminas-modulemanager/
BSD 3-Clause "New" or "Revised" License
30 stars 18 forks source link

Merge order of config files on FreeBSD-based systems is invalid #20

Closed boesing closed 2 years ago

boesing commented 3 years ago

Bug Report

Q A
Version(s) 2.10.1

Summary

Relates to https://github.com/zendframework/zend-config-aggregator/pull/14

Current behavior

Glob path: autoload/{,*.}{global,local}.php Directory structure:

autoload/a.local.php autoload/global.php autoload/local.php

Merge order on non-FreeBSD-based systems:

  1. autoload/global.php
  2. autoload/a.local.php
  3. autoload/local.php

Merge order on FreeBSD-based systems:

  1. autoload/a.local.php
  2. autoload/global.php
  3. autoload/local.php

How to reproduce

See above.

Expected behavior

Merge should be the same, regardless on what system is being used.

In the mentioned issue https://github.com/zendframework/zend-config-aggregator/pull/14, the fix was to force the fallback mode of laminas-stdlib Glob::glob. I've verified this locally and can confirm that it fixes the order of the files.

boesing commented 3 years ago

Temporary fix is changing the application.config.php to something like this:


return [
    // all the other params
    'module_listener_options' => [
        'config_glob_paths' => [
            sprintf('%s/autoload/{,*.}global.php', __DIR__),
            sprintf('%s/autoload/{,*.}local.php', __DIR__),
        ],
    ],
];
Xerkus commented 3 years ago

Expecting files to be returned in certain order based on glob format was an incorrect assumption based on implementation detail.

Depending on undefined behavior is problematic and the only correct way to handle this is to amend usage in our skeleton applications and anywhere in documentation. Correct usage would be to provide multiple separate glob patterns (or multiple PhpFileProvider for config-aggregator) in order of merge priority.

boesing commented 3 years ago

yup, I see this point. But at least we should try to be consistent with config-aggregator here, dont you think so?