Skip to content
This repository has been archived by the owner on Jul 28, 2022. It is now read-only.

Disable admin by default #324

Merged
merged 2 commits into from Mar 8, 2019
Merged

Conversation

covex-nn
Copy link
Contributor

@covex-nn covex-nn commented Apr 20, 2018

I am targeting this branch, because this is BC break

Changelog

### Changed
- Disable admin configuration by default

Subject

I believe, that disabling admin in configuration is more correct, because it can be enabled only when sonata.notification.backend.doctrine was enabled.

A default configuration, i.e. recipe for sonata-project/notification-bundle could be like this:

sonata_notification:
    consumers:
        register_default: false
    admin:
        enabled: false

Also, default configuration could contain commented configuration for backend: sonata.notification.backend.rabbitmq. Anyway, i have already created PR with recipe for this bundle in febuary symfony/recipes-contrib#302

Additional configuration, i.e. a recipe for a new package sonata-project/notification-orm-pack could be like this (i created a gist with entity class):

sonata_notification:
    backend: sonata.notification.backend.doctrine
    backends:
        doctrine: ~
    class:
        message: App\Entity\SonataNotificationMessage
    admin:
        enabled: true

So, additional configuration will override default.

Copy link
Contributor

@kunicmarko20 kunicmarko20 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a BC break? You can't do it on 3.x

@kunicmarko20
Copy link
Contributor

cc @sonata-project/contributors

@kunicmarko20
Copy link
Contributor

I would rather add deprecation now, change it on master and for now add it to recipe

@covex-nn
Copy link
Contributor Author

covex-nn commented Apr 20, 2018

No! It doesn't matter, i will update right now

@covex-nn covex-nn changed the base branch from 3.x to master April 20, 2018 16:18
@covex-nn
Copy link
Contributor Author

I changed base branch of this PR

@kunicmarko20
Copy link
Contributor

Please add upgrade note. Should we deprecate this on stable @sonata-project/contributors ?

@greg0ire
Copy link
Contributor

What should probably be deprecated on stable would be not specifying this boolean (and expecting it to default to true)

@covex-nn
Copy link
Contributor Author

May be if class sonata_notification.class.message is exists - admin should be enabled automaticaly? And may be this parameter cannot be enabled my configuration - it can only be disabled? Can it be done for current stable?

And i do not know how to deprecate configuration )

@covex-nn
Copy link
Contributor Author

covex-nn commented Apr 28, 2018

Since #326, even if admin is enabled, services for SonataAdmin are added only if notification backend is sonata.notification.backend.doctrine. I guess this PR can be closed, wdyt? Anyway, i've added upgrade note, it this PR will be accepted - it is ok too! =) Also PR in symfony/recipes-contrib should be updated (admin can be enabled all the time after new future release of NotificationBundle)

@core23 core23 merged commit 8ccbb32 into sonata-project:master Mar 8, 2019
@core23
Copy link
Member

core23 commented Mar 8, 2019

Thanks @covex-nn

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants