Skip to content

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

Merged
merged 1 commit into from
May 9, 2020

Conversation

SerheyDolgushev
Copy link
Contributor

@SerheyDolgushev SerheyDolgushev commented Apr 8, 2020

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:

  1. 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. It is disabled only when there is extra.symfony.bundles configuration in the package (even if this configuration is an empty array).
  2. New extra.symfony.bundles package configuration was introduced. It should contain the list of bundles and the environments where those bundles should be enabled. Example:
    "extra": {
        "symfony": {
            "bundles": {
                "VendorName\\BundleName1": ["all"],
                "VendorName\\BundleName2": ["dev", "test"]
            }
        }
    }
    

Original PR description:

I just do not see any reasons why ezplatform-bundle packages couldn't be enabled automatically after the installation (in the same way as symfony-bundle packages are).

Tagging @andrerom here

@andrerom
Copy link

andrerom commented Apr 8, 2020

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.

@SerheyDolgushev
Copy link
Contributor Author

I was thinking of adding 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.

@nicolas-grekas what do you think?

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Apr 10, 2020

Was thinking this days to provide the same for sulu-bundle. Maybe we can find a convention. Or maybe use a extra field to provide the classes which should be added to the bundles.php.

{
    "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.

@SerheyDolgushev
Copy link
Contributor Author

Talked to @nicolas-grekas on Symfony slack channel and he liked the idea suggested by @alexander-schranz. So this PR introduces extra.symfony.bundle-classes config.

When any package is installed, following bundles will be enabled automatically after the installation:

  1. If package type is symfony-bundle all bundle classes will be extracted from it (current behavior).
  2. If package has extra.symfony.bundle-classes configuration, all the bundle classes will be used from it (implemented in this PR).

Example usage:

{
    "name": "contextualcode/ezplatform-search-binary-extractor",
    "type": "ezplatform-bundle",
    ...
    "extra": {
        "symfony": {
            "bundle-classes": ["ContextualCode\\EzPlatformSearchBinaryExtractorBundle\\EzPlatformSearchBinaryExtractorBundle"]
        }
    }
}

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Apr 16, 2020

@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?

@SerheyDolgushev
Copy link
Contributor Author

@alexander-schranz I like this idea. @nicolas-grekas what do you think?

@SerheyDolgushev
Copy link
Contributor Author

Would change it to:

"symfony": {
    "bundle-classes": {
        "ContextualCode\\EzPlatformSearchBinaryExtractorBundle\\EzPlatformSearchBinaryExtractorBundle": ["all"],
        "SomeDebugBundle": ["dev"]
    }
}

@alexander-schranz
Copy link
Contributor

@SerheyDolgushev looks good for me 👍

@SerheyDolgushev
Copy link
Contributor Author

SerheyDolgushev commented Apr 16, 2020

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?
So maybe symfony.bundle-envs would make more sense?
@alexander-schranz what do you think?

@alexander-schranz
Copy link
Contributor

alexander-schranz commented Apr 16, 2020

@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.

@nicolas-grekas
Copy link
Member

"SomeDebugBundle": ["dev"]

Makes total sense yes, same as in manifest.json files.
I'd even suggest renaming the key to just "bundles" for consistency.

@SerheyDolgushev
Copy link
Contributor Author

So symfony-bundle packages are handled as they were before. But this PR provides support for:

    "extra": {
        "symfony": {
            "bundles": {
                "ContextualCode\\CoolBundle1": ["all"],
                "ContextualCode\\CoolBundle2": ["dev", "tests"]
            }
        }
    }

@alexander-schranz @nicolas-grekas

@alexander-schranz
Copy link
Contributor

Looks good for me 👍

@fabpot
Copy link
Member

fabpot commented Apr 17, 2020

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.

@SerheyDolgushev
Copy link
Contributor Author

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 symfony-bundle are handled now? I guess it would be a brilliant change, as it will not require any additional changes for existing bundles, and they will be automatically enabled after the composer require.

But also, I think, it make sense to add extra.symfony.bundles, just to being able to enable specific bundles in a specific environment.

So the final behavior would be:

  1. Extract bundles from all the packages, but not just symfony-bundle.
  2. If the current package is listed under the composers' dev packages, all its bundles will be enabled for dev and test environments. Otherwise, they will be enabled for all environments (current behavior).
  3. Extract the list of bundles from extra.symfony.bundles and override specified environments for them. It also will work if in some cases the bundles were not extracted during Better explain in the README the dangers of using Flex #1.

So this PR will implement the following:

  1. Extracted bundles will be enabled automatically for all packages, but not just symfony-bundle.
  2. It will use extra.symfony.bundles to change the environments for extracted bundles and for specifying additional bundles that are not extracted automatically by some reason.

Gentlemans @nicolas-grekas @andrerom @alexander-schranz what do you think?

@nicolas-grekas
Copy link
Member

Sounds good to me, with just one change: if extra.symfony.bundles is found, autodiscovery should be disabled. This will provide a way to skip some bundles when appropriate.

@SerheyDolgushev
Copy link
Contributor Author

SerheyDolgushev commented Apr 18, 2020

Just to summarize this PR:

  1. 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. It is disabled only when there is extra.symfony.bundles configuration in the package (even if this configuration is an empty array).
  2. New extra.symfony.bundles package configuration was introduced. It should contain the list of bundles and the environments where those bundles should be enabled. Example:
    "extra": {
        "symfony": {
            "bundles": {
                "VendorName\\BundleName1": ["all"],
                "VendorName\\BundleName2": ["dev", "test"]
            }
        }
    }
    

And that is basically all. I tested the latest changes on a few bundles and it worked as expected.

@SerheyDolgushev
Copy link
Contributor Author

I can't mention I was listening to Zaz and Charles Aznavour while I was implementing @nicolas-grekas review.

@SerheyDolgushev SerheyDolgushev changed the title Automatically enable "ezplatform-bundle" packages Enable bundles autodiscovery by default, providing extra.symfony.bundles for customizations Apr 18, 2020
@SerheyDolgushev SerheyDolgushev changed the title Enable bundles autodiscovery by default, providing extra.symfony.bundles for customizations Enable bundles autodiscovery by default and provide extra.symfony.bundles for customizations Apr 18, 2020
@SerheyDolgushev SerheyDolgushev changed the title Enable bundles autodiscovery by default and provide extra.symfony.bundles for customizations Enable bundles autodiscovery by default and provide extra.symfony.bundles configuration for customizations Apr 18, 2020
@SerheyDolgushev SerheyDolgushev changed the title Enable bundles autodiscovery by default and provide extra.symfony.bundles configuration for customizations Enable bundles autodiscovery by default and provide "extra.symfony.bundles" configuration for customizations Apr 18, 2020
@fabpot
Copy link
Member

fabpot commented Apr 19, 2020

I see that you've added a new test file, I was more expecting new test cases on existing tests.

@SerheyDolgushev
Copy link
Contributor Author

@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.

@fabpot
Copy link
Member

fabpot commented Apr 19, 2020

I would prefer to keep all tests in the same file, yes.

@SerheyDolgushev SerheyDolgushev force-pushed the enable-ezplatform-bundles branch from 76ca18c to c7a3fa8 Compare April 19, 2020 21:05
@SerheyDolgushev
Copy link
Contributor Author

@fabpot done in c7a3fa8

@natepage
Copy link

@fabpot @nicolas-grekas

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 symfony/recipes-contrib gonna be updated to accept recipes for packages that are not symfony-bundle?

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 recipes-contrib for a few packages we're working on, those packages follow the same philosophy which is to implement the core features to be framework agnostic and then create bridges for them:

package/
-- src/
---- Bridge/
------ Laravel/
------ Symfony/
-------- PackageBundle.php
---- ... Core files ...     

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 library but it also provides a Symfony bundle and having a recipe for it would make DX amazing!

@fabpot
Copy link
Member

fabpot commented May 8, 2020

I think that the extra.symfony.bundles feature should be removed from this PR. It is orthogonal to the goal and duplicates what recipes are for, so I'm in favor of adding it.

@andrerom
Copy link

andrerom commented May 8, 2020

@fabpot So you only want following change then to be clear? (+test adjustments)

- if ($noRecipe && 'symfony-bundle' === $package->getType()) {
+ if ($noRecipe) {

@fabpot
Copy link
Member

fabpot commented May 8, 2020

This PR should really just be about bundle auto-discovery for all packages.

@SerheyDolgushev SerheyDolgushev changed the title Enable bundles autodiscovery by default and provide "extra.symfony.bundles" configuration for customizations Enable bundles autodiscovery by default May 9, 2020
@SerheyDolgushev SerheyDolgushev changed the title Enable bundles autodiscovery by default Enable bundles auto-discovery by default May 9, 2020
@SerheyDolgushev
Copy link
Contributor Author

@fabpot requested changes have been applied. Please review them. I`ll squash commits after your confirmation.

@fabpot fabpot force-pushed the enable-ezplatform-bundles branch from b1a8eb9 to a4bea5a Compare May 9, 2020 12:10
@fabpot
Copy link
Member

fabpot commented May 9, 2020

Thank you @SerheyDolgushev.

@fabpot fabpot merged commit 89999fd into symfony:master May 9, 2020
@alexander-schranz
Copy link
Contributor

Its really sad to hear that the we can not configure it from the composer.json. It would be great to have everything in the package version control, but I think you decided to go with recipe repository to have all recipes under your control, I can understand that decision but from package maintainer it would be better to have a way to provide the recipe config with the package code directly.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented May 12, 2020

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.

@dunglas
Copy link
Member

dunglas commented May 12, 2020

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.

@alexander-schranz
Copy link
Contributor

@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.

@fabpot
Copy link
Member

fabpot commented Jul 29, 2020

@alexander-schranz Fixed now

@alexander-schranz
Copy link
Contributor

@fabpot Thank you!

tgalopin pushed a commit to tgalopin/flex that referenced this pull request Dec 3, 2020
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.

7 participants