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

Add support for global admin extensions via config #6651

Merged
merged 1 commit into from
Dec 6, 2020
Merged

Conversation

core23
Copy link
Member

@core23 core23 commented Dec 5, 2020

Subject

Is is now possible to register a global admin extension via the configuration. This was only possible for tags.

The next step (new PR) is to allow all extension configuration for tags.

I am targeting this branch, because this feature is BC.

Changelog

### Added
- Add support for global admin extensions via config

@core23 core23 requested a review from a team December 5, 2020 14:25
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

There is

admins:
    specify one or more admin service ids to which the Extension should be added
excludes:
    specify one or more admin service ids to which the Extension should not be added (this will prevent it matching
    any of the other settings)
extends:
    specify one or more classes. If the managed class of an admin extends one of the specified classes the extension
    will be added to that admin.
implements:
    specify one or more interfaces. If the managed class of an admin implements one of the specified interfaces the
    extension will be added to that admin.
instanceof:
    specify one or more classes. If the managed class of an admin extends one of the specified classes or is an instance
    of that class the extension will be added to that admin.
uses:
    Specify one or more traits. If the managed class of an admin uses one of the specified traits the extension will be
    added to that admin.
priority:
    Can be a positive or negative integer. The higher the priority, the earlier it’s executed.

You should add a description of your new option too.

@@ -516,14 +516,15 @@ public function getConfigTreeBuilder()

->arrayNode('extensions')
->useAttributeAsKey('id')
->defaultValue(['admins' => [], 'excludes' => [], 'implements' => [], 'extends' => [], 'instanceof' => [], 'uses' => []])
->defaultValue([])
Copy link
Member Author

Choose a reason for hiding this comment

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

This config was wrong for many years. If you do not have any extension, you should not register any invalid extension config.

E.g. the 'admins' => [] node tries to register something like this:

        sonata_admin:
            extensions:
                admins: []

The correct config should be something like this:

        sonata_admin:
            extensions:
                app.publish.extension:
                    global: true
                    admins:
                        - app.admin.article
                    implements:
                        - App\Publish\PublishStatusInterface
                    excludes:
                        - app.admin.blog
                        - app.admin.news
                    extends:
                        - App\Document\Blog
                    instanceof:
                        -  App\Document\Page
                    uses:
                        -  App\Trait\Timestampable

@core23 core23 force-pushed the tag-extension branch 2 times, most recently from b8ea221 to 4757783 Compare December 5, 2020 14:33
Copy link
Member

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

Now I understand what's doing this global options, do we need it ?

It can be done with

implements:
    - Sonata\AdminBundle\Admin\AdminInterface

isn't it ?

@VincentLanglet
Copy link
Member

Ok, if I understand, this option already exists but was not usable in the config.
It makes sens to merge this, or deprecate the ability to do it for tags

@VincentLanglet VincentLanglet requested a review from a team December 5, 2020 15:07
@core23
Copy link
Member Author

core23 commented Dec 5, 2020

Now I understand what's doing this global options, do we need it ?

It can be done with

implements:
    - Sonata\AdminBundle\Admin\AdminInterface

isn't it ?

No implements is for model classes only, not for admin classes.

Ok, if I understand, this option already exists but was not usable in the config.
It makes sens to merge this, or deprecate the ability to do it for tags

My idea is to allow all options for the config and and tags.

@core23 core23 requested a review from a team December 5, 2020 20:02
@OskarStark OskarStark merged commit ff709e6 into 3.x Dec 6, 2020
@OskarStark OskarStark deleted the tag-extension branch December 6, 2020 16:09
@OskarStark
Copy link
Member

Thanks @core23 😍

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

Successfully merging this pull request may close these issues.

3 participants