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

v0.6.0 - composer.json extra including alias unable to override #88

Closed
bkuhl opened this Issue Jan 8, 2018 · 5 comments

Comments

3 participants
@bkuhl

bkuhl commented Jan 8, 2018

v.0.6.0 introduced this change into the composer.json:

            "extra": {
                "laravel": {
                    "aliases": {
                        "Setting": "anlutro\\LaravelSettings\\Facade"
                    },
                    "providers": [
                        "anlutro\\LaravelSettings\\ServiceProvider"
                    ]
                }
            },

This is great for the providers, but the alias is a breaking change and we're unable to override it (that I can find anyways).

I have my facade alias configured as:

'Setting'              => \App\Support\Facades\SettingsManager::class,

I then extend this package's SettingsManager. When upgrading to 0.6.0 I see this error:

Call to undefined method anlutro\LaravelSettings\DatabaseSettingStore::firstYearDiscount()

This is happening because it's using the composer's alias rather than mine. I think registering the facade should be left optional and removed from composer.json.

Reverting to 0.5.2 resolved the issue.

@anlutro

This comment has been minimized.

Show comment
Hide comment
@anlutro

anlutro Jan 8, 2018

Owner

Hey, technically this is indeed a breaking change but that's what major versions are for. However, I think you're doing something wrong here. Why are you using your own facade? I think the more appropriate way of extending a third-party package is by re-binding the underlying class:

$app->bindShared('anlutro\Settings\SettingsManager', function($app) {
    return new MySettingsManager($app);
});

This page has some more information. You can also try Settings::extend if you want to create a custom driver, that's even cleaner, but might also be a little bit more work.

Regardless - ultimately it might have been a mistake to set a default facade alias. I really can't say, as I don't work with Laravel anymore, but let me know if you're unable to work around this issue.

Owner

anlutro commented Jan 8, 2018

Hey, technically this is indeed a breaking change but that's what major versions are for. However, I think you're doing something wrong here. Why are you using your own facade? I think the more appropriate way of extending a third-party package is by re-binding the underlying class:

$app->bindShared('anlutro\Settings\SettingsManager', function($app) {
    return new MySettingsManager($app);
});

This page has some more information. You can also try Settings::extend if you want to create a custom driver, that's even cleaner, but might also be a little bit more work.

Regardless - ultimately it might have been a mistake to set a default facade alias. I really can't say, as I don't work with Laravel anymore, but let me know if you're unable to work around this issue.

@bkuhl

This comment has been minimized.

Show comment
Hide comment
@bkuhl

bkuhl Jan 8, 2018

I'm extending so that I can use strongly typed parameters and return values.

It's not possible to overwrite that setting when it's in the composer file, so there's no workaround that I could find other than reverting to the previous version, which works sufficiently.

bkuhl commented Jan 8, 2018

I'm extending so that I can use strongly typed parameters and return values.

It's not possible to overwrite that setting when it's in the composer file, so there's no workaround that I could find other than reverting to the previous version, which works sufficiently.

@anlutro

This comment has been minimized.

Show comment
Hide comment
@anlutro

anlutro Jan 8, 2018

Owner

Ah, and the strong typing doesn't work when extending the manager/driver because it's the facade you're calling, not the "real" class? I remember there used to be code-generators that generated autocomplete stubs for your facades for use in IDEs like PHPStorm, maybe that can help you out. Honestly, reasons like these are reasons I moved away from using facades at all, and just injected the dependencies directly into my classes' constructors.

I'm tempted to say that the design of Laravel's facades are to blame here, as well as the fact that composer-registered aliases take precedence over user-defined ones. I'm afraid that if I were to remove the default alias now I would probably break the package for more people than I'm helping, but for the next major version (whenever that is) I'm willing to reconsider.

Owner

anlutro commented Jan 8, 2018

Ah, and the strong typing doesn't work when extending the manager/driver because it's the facade you're calling, not the "real" class? I remember there used to be code-generators that generated autocomplete stubs for your facades for use in IDEs like PHPStorm, maybe that can help you out. Honestly, reasons like these are reasons I moved away from using facades at all, and just injected the dependencies directly into my classes' constructors.

I'm tempted to say that the design of Laravel's facades are to blame here, as well as the fact that composer-registered aliases take precedence over user-defined ones. I'm afraid that if I were to remove the default alias now I would probably break the package for more people than I'm helping, but for the next major version (whenever that is) I'm willing to reconsider.

@bweston92

This comment has been minimized.

Show comment
Hide comment
@bweston92

bweston92 Jan 8, 2018

Collaborator

Hi @bkuhl sorry for the inconvenience, I've managed to find a solution for you though.

Add the following to your applications composer.json file.

"extra": {
    "laravel": {
        "dont-discover": [
            "anlutro/l4-settings"
        ]
    }
},

Corresponding Laravel code: https://github.com/laravel/framework/blob/5.5/src/Illuminate/Foundation/PackageManifest.php#L144

Collaborator

bweston92 commented Jan 8, 2018

Hi @bkuhl sorry for the inconvenience, I've managed to find a solution for you though.

Add the following to your applications composer.json file.

"extra": {
    "laravel": {
        "dont-discover": [
            "anlutro/l4-settings"
        ]
    }
},

Corresponding Laravel code: https://github.com/laravel/framework/blob/5.5/src/Illuminate/Foundation/PackageManifest.php#L144

@bweston92 bweston92 closed this Jan 8, 2018

@bweston92

This comment has been minimized.

Show comment
Hide comment
@bweston92

bweston92 Jan 10, 2018

Collaborator

@bkuhl can you confirm the proposed solution worked? If so please create a PR to the README.

Collaborator

bweston92 commented Jan 10, 2018

@bkuhl can you confirm the proposed solution worked? If so please create a PR to the README.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment