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

[Feature] Add Support Bundle Default Configuration #7

Closed
wants to merge 4 commits into from

Conversation

ad3n
Copy link

@ad3n ad3n commented Jul 30, 2015

This PR add support for default configuration and also rename alwaysRegisteredPlugin to defaultPlugins.

@matthiasnoback
Copy link
Owner

I appreciate the effort but I would've liked to discuss this first. It
deviates pretty much from the way this package currently works. Existing
users may not need that default configuration but now they have to
implement that method. Also I don't think that introducing the cyclic
dependency by passing arround $this is a good idea. It requires opening
up the API by making methods public that should have been protected or
private. Finally, tests are missing.

If it works in your situation I would encourage you to maintain your own
fork of this package. There's no problem with that of course! Besides, I
don't think this package will receive many updates so you won't have to do
a lot of backporting.

This PR add support for default configuration and also rename

alwaysRegisteredPlugin to defaultPlugins.

You can view, comment on, or merge this pull request online at:

#7
Commit Summary

  • add support for default plugin
  • add api doc annotation

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#7.

@ad3n
Copy link
Author

ad3n commented Jul 30, 2015

First point, i was remove abstract keyword the addConfiguration method, so if user want to add default config, he just need to override the method.

About passing $this to Extension class, i don't think we have other option.

And the test is green now :-)

@magarzon
Copy link

I don't see either the need for a main configuration, when you can have it in a CorePlugin.

If you want something shared between plugins, you can save it in the container, when CorePlugin configuration is loaded.

Maybe if you give us a concrete example of what you think it's 'global' configuration...

@matthiasnoback
Copy link
Owner

@ihsanudin again, I appreciate your effort, but this doesn't really add something that wasn't possible before using a core/main plugin. Also, because of this PR what you want to achieve is doable now for just your own project. I want this library to be as simple/small as possible.

@matthiasnoback
Copy link
Owner

Closing this too, since it won't be added. You can easily start your own project based on this if you like!

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

Successfully merging this pull request may close these issues.

None yet

3 participants