Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BC break in 3.6.3 with sorting paths #45

Closed
snapshotpl opened this issue Dec 28, 2021 · 18 comments · Fixed by #46
Closed

BC break in 3.6.3 with sorting paths #45

snapshotpl opened this issue Dec 28, 2021 · 18 comments · Fixed by #46
Labels
BC Break Bug Something isn't working
Milestone

Comments

@snapshotpl
Copy link
Member

BC Break Report

Q A
Version 3.6.3

Summary

#43 with fix for #15 introduces BC break. #17 explained it already.

Previous behavior

File paths didn't sort.

Current behavior

File paths are sort now.

@snapshotpl
Copy link
Member Author

I have use Glob::glob in laminas-modulemanager context in config_glob_paths

@Ocramius
Copy link
Member

Should we just revert?

@Ocramius Ocramius added the Bug Something isn't working label Dec 28, 2021
@snapshotpl
Copy link
Member Author

Not sure.

For me call fallbackGlob with $forceFallback and skip systemGlob should help.

https://github.com/laminas/laminas-stdlib/blob/3.7.x/src/Glob.php#L55

@driehle
Copy link
Contributor

driehle commented Dec 28, 2021

I can confirm this bug. Updating from laminas-stdlib 3.6.2 to 3.6.3 breaks my application. I am having a configuration file named ldap.global.php which seems not to be loaded anymore and then a service breaks, throwing an exception for missing its configuration.

Thanks, snapshotpl, for figuring out the cause!

By the way, I am using pretty much the default Laminas MVC skeleton with the following configuration in application.config.php:

        'config_glob_paths' => [
            realpath(__DIR__) . '/autoload/{{,*.}global,{,*.}local}.php',
        ],

... and the following in development.config.php(.dist):

        'config_glob_paths' => [
            realpath(__DIR__) . '/autoload/{,*.}{global,local}-development.php',
        ],

This is how the merged configuration looks like:

grafik

So far everything fine, I think, but ldap.global.php is only loaded with laminas-stdlib 3.6.2, not with 3.6.3.

@snapshotpl
Copy link
Member Author

Looks like this will break most laminas app, so need to revert ASAP. @Ocramius ? @weierophinney ?

@Ocramius
Copy link
Member

Send a revert patch 👍

@naur1970
Copy link

It seems to be negation sign is forgotten here https://github.com/laminas/laminas-stdlib/blob/3.7.x/src/Glob.php#L112
if (! $flags & self::GLOB_BRACE) { was before
if (self::flagsIsEqualTo($flags, self::GLOB_BRACE)) { is now

@driehle
Copy link
Contributor

driehle commented Dec 29, 2021

I can confirm that adding the missing negation sign fixes the broken loading of configuration files in my MVC application.

@Ocramius
Copy link
Member

@driehle should I wait for your work on the patch, or revert?

@Ocramius Ocramius linked a pull request Dec 29, 2021 that will close this issue
@Ocramius Ocramius added this to the 3.6.4 milestone Dec 29, 2021
@driehle
Copy link
Contributor

driehle commented Dec 29, 2021

@Ocramius I think a revert is not needed. Simply fixing the negation sign should be fine. What I am currently trying to to is setting up a CI job that runs with Alpine Linux, so that we have a failing test case.

I think that the current issue only appears on Alpine Linux, which doesn't provide GLOB_BRACE. At least my app runs on Alpine Linux. Maybe @snapshotpl and @naur1970 can confirm that for him the issue appears on Alpine Linux as well? Anyways, currently CI runs on Ubuntu (latest) only, which provides GLOB_BRACE. Hence, the fallback is never actually covered in CI which is likely why this error has not been discovered prior to merging.

@naur1970
Copy link

Confirm! In my case the ploblem has manifested on Alpine Linux.

driehle added a commit to driehle/laminas-stdlib that referenced this issue Dec 29, 2021
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
driehle added a commit to driehle/laminas-stdlib that referenced this issue Dec 29, 2021
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
driehle added a commit to driehle/laminas-stdlib that referenced this issue Dec 29, 2021
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
@driehle
Copy link
Contributor

driehle commented Dec 29, 2021

@Ocramius I think I got it all solved, see #46 for a patch.

@snapshotpl
Copy link
Member Author

Yes, it appears on Alpine. Thanks for patch!

@snapshotpl
Copy link
Member Author

FIxed in #46

@weierophinney
Copy link
Member

Ah, that's interesting! Previously, the only OS that didn't provide GLOB_BRACE was IBM i - and we had workarounds for that (it's why we even had the functionality in here!).

Will look at the patch later today - thanks for doing the legwork!

@driehle
Copy link
Contributor

driehle commented Dec 29, 2021

Thanks to @naur1970, who found the actual cause!

@Ocramius
Copy link
Member

Thanks for getting it released so quickly, y'all: stellar work! 💪

@nynka
Copy link

nynka commented Dec 29, 2021

Good job guys! I was struggling with this for a while today. Thanks for fixing so quickly! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants