-
-
Notifications
You must be signed in to change notification settings - Fork 195
Enable bundles auto-discovery by default #612
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
Enable bundles auto-discovery by default #612
Conversation
I was thinking of proposing this some time ago as well, but I didn’t as I’m kind of expecting Fabien / Nicolas et. all to not want to add all custom bundle composer package types out there. So I think we are lacking a convention, or something to say ezplatform-bundle is a superset of symfony-bundle For instance composer should maybe allow type to be array (unlikely) or Flex should also look in composer.json keywords for ‘symfony-bundle’ (more likely), or something else entirely. |
@nicolas-grekas what do you think? |
Was thinking this days to provide the same for {
"extra": {
"symfony": {
"flex-bundle-classes": ["My\Bundle\MyBundle"]
}
}
} This would also don't need any hacks for sylius plugins they just say which class should be added to the bundles.php. |
Talked to @nicolas-grekas on Symfony slack channel and he liked the idea suggested by @alexander-schranz. So this PR introduces When any package is installed, following bundles will be enabled automatically after the installation:
Example usage: {
"name": "contextualcode/ezplatform-search-binary-extractor",
"type": "ezplatform-bundle",
...
"extra": {
"symfony": {
"bundle-classes": ["ContextualCode\\EzPlatformSearchBinaryExtractorBundle\\EzPlatformSearchBinaryExtractorBundle"]
}
}
} |
@SerheyDolgushev I would also support in which environment the bundle should be registered: E.g.: "symfony": {
"bundle-classes": {
"ContextualCode\\EzPlatformSearchBinaryExtractorBundle\\EzPlatformSearchBinaryExtractorBundle": {"all": true},
"SomeDebugBundle": {"dev": true},
}
} or what do you think? |
@alexander-schranz I like this idea. @nicolas-grekas what do you think? |
Would change it to:
|
@SerheyDolgushev looks good for me 👍 |
And I'm not sure if it worth adding this option for each bundle. I guess in most cases each package contains just one bundle? |
@SerheyDolgushev we have a monorepo/package currently for sulu core and not all our bundles will be registred in all environments of sulu so we need to define it per bundle if possible. |
Makes total sense yes, same as in |
So
|
Looks good for me 👍 |
Another idea (I've not tried, so it might not work well): we already have many heuristics to find the bundle class, what about looking for a bundle class in *all packages instead of limiting it to Symfony bundles. That would allow packages that are a lib and a bundle to be auto-detected as well. |
So seems like you are suggesting to enable all the bundles by default? Handle all the bundles in a way as But also, I think, it make sense to add So the final behavior would be:
So this PR will implement the following:
Gentlemans @nicolas-grekas @andrerom @alexander-schranz what do you think? |
Sounds good to me, with just one change: if |
Just to summarize this PR:
And that is basically all. I tested the latest changes on a few bundles and it worked as expected. |
I can't mention I was listening to Zaz and Charles Aznavour while I was implementing @nicolas-grekas review. |
extra.symfony.bundles
for customizations
extra.symfony.bundles
for customizationsextra.symfony.bundles
for customizations
extra.symfony.bundles
for customizationsextra.symfony.bundles
configuration for customizations
extra.symfony.bundles
configuration for customizationsa172ac3
to
b11f06e
Compare
I see that you've added a new test file, I was more expecting new test cases on existing tests. |
@fabpot Yes, I decided to use a separate file for these tests. In theory, they can be "merged" into https://github.com/symfony/flex/blob/master/tests/FlexTest.php, if it is critical. |
I would prefer to keep all tests in the same file, yes. |
76ca18c
to
c7a3fa8
Compare
Hi there, I don't know if it is the right place to discuss this and I'm happy to create a specific issue if required but I think it is related. Once this PR gets merged, are the validation rules to add recipes to The current PR enables autodiscovery for bundle classes and that's amazing already but if a package has some config to copy, env to create, etc. It still needs to create a recipe for all that to be automated. Yesterday I've created a PR to
This allows us to implement feature solving business cases and reuse them across different projects/platforms and make sure it works as expected on each of them centralised in one place. So yes the package itself is a |
I think that the |
@fabpot So you only want following change then to be clear? (+test adjustments) - if ($noRecipe && 'symfony-bundle' === $package->getType()) {
+ if ($noRecipe) { |
This PR should really just be about bundle auto-discovery for all packages. |
@fabpot requested changes have been applied. Please review them. I`ll squash commits after your confirmation. |
b1a8eb9
to
a4bea5a
Compare
Thank you @SerheyDolgushev. |
Its really sad to hear that the we can not configure it from the |
Recipes are like a Linux distribution. Putting them in the hand of package authors would wrongly give them a responsibility they should not have. Defining how a package is used is always an opinionated decision and doesn't belong to lib authors. That "opinion" belongs to a separate repo: the one we provide when using something in the context of Symfony. |
This is subject to debate. Even for OS packages, the trend (Snap, Docker, App Store, Google Play...) is to let editors packaging software as they want. It would help a lot of packages to include their recipes in their repositories (I have Sylius plugins in mind for instance). It will also make the versioning easier. Such recipes could be considered as « contrib » recipes, so the end user has the choice to execute it or not. |
@nicolas-grekas currently it seems not yet possible to submit recipes for other types: symfony/recipes-contrib#984 as the CI seems still check for the composer type when submitting recipes. |
@alexander-schranz Fixed now |
@fabpot Thank you! |
Bundles autodiscovery is now enabled for all the packages, but not only for
symfony-bundle
type packages. And it works in the same way as it worked before.PR description before @fabpot review:
Original PR description: