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

Set "extra.symfony.bundles" property at composer.json in order to be compatible with Symfony Flex #97

Closed
wants to merge 2 commits into from

Conversation

phansys
Copy link
Member

@phansys phansys commented Jul 1, 2020

Subject

Set "extra.symfony.bundles" property at composer.json in order to be compatible with Symfony Flex.

I am targeting this branch, because this change respects BC.

Triggered by sonata-project/SonataAdminBundle#6173.

Changelog

### Added
- Added "extra.symfony.bundles" property at `composer.json` in order to be compatible with Symfony Flex.

@phansys phansys requested a review from a team July 1, 2020 21:29
@jordisala1991
Copy link
Member

That should not be needed: symfony/flex#612

@jordisala1991
Copy link
Member

I mean, it seems like we dont have to change the bundle type, but add the extra section to composer.json (that would be better, since this is not a bundle only)

@phansys
Copy link
Member Author

phansys commented Jul 1, 2020

I agree we should not change the type, but I don't know how to make this compatible with Flex in other way. Do you have an example about the approach you're mentioning?

Sorry, didn't noticed about the linked PR. Updating.

@phansys phansys changed the title Use "symfony-bundle" as Composer type Set "extra.symfony.bundles" property at composer.json in order to be compatible with Symfony Flex Jul 1, 2020
@phansys phansys added the minor label Jul 1, 2020
@phansys
Copy link
Member Author

phansys commented Jul 1, 2020

I've applied your proposal @jordisala1991. Thank you so much!
Maybe we can take the same approach later with "sonata-project/form-extensions".

@jordisala1991
Copy link
Member

I think it should be good now, but we have to try if it works. It could apply to doctrine extension too

jordisala1991
jordisala1991 previously approved these changes Jul 1, 2020
@jordisala1991 jordisala1991 requested a review from a team July 1, 2020 22:46
@phansys
Copy link
Member Author

phansys commented Jul 2, 2020

I think it should be good now, but we have to try if it works.

Surely we must release at least one of these packages before.

@franmomu
Copy link
Member

franmomu commented Jul 2, 2020

mmm maybe I misunderstood the PR @jordisala1991 mentioned, but looked like this feature wasn't implemented.

@jordisala1991
Copy link
Member

Seems like you are right, then if the auto discovery is enabled for all package types, then why it does not work?

Have you tried if it works? I am not a user of flex

VincentLanglet
VincentLanglet previously approved these changes Jul 2, 2020
core23
core23 previously approved these changes Jul 2, 2020
@franmomu
Copy link
Member

franmomu commented Jul 2, 2020

Looks like Flex searches the bundle class in the paths defined in autoload and then looks for the bundle class there, in the root directory, so it doesn't find it because ours is inside Bridge\Symfony in this case.

Based on that, I'm not sure if we change the composer type to symfony-bundle it would work. So... I think we either create a recipe registering the bundle (and gets accepted which doesn't seem easy) or create a separate package as symfony-bundle with the symfony related code.

@phansys
Copy link
Member Author

phansys commented Jul 2, 2020

mmm maybe I misunderstood the PR @jordisala1991 mentioned, but looked like this feature wasn't implemented.

Thank you for the analysys, I didn't see that comment.

I've checked locally using the constraint sonata-project/twig-extensions:dev-composer-type from my fork, and effectively can confirm the feature is not working.

Based on that, I'm not sure if we change the composer type to symfony-bundle it would work

I just tested symfony/recipes-contrib#975 locally, since "sonata-project/form-extensions" is already using the type "symfony-bundle". Regardless the change at CI seems to work properly, locally the class is not being registered.

@jordisala1991
Copy link
Member

So it is a bug on flex, or we are doing something wrong here?

@SerheyDolgushev can you help us here?

@wbloszyk
Copy link
Member

wbloszyk commented Jul 2, 2020

What about release sonata-project/twig-bundle and sonata-project/twig-extensions without bridge? Like KnpMenu and KnpMenuBundle.
Some thing like seperate FlashManager from StatusClassRenderer needs next major.

@jordisala1991
Copy link
Member

jordisala1991 commented Jul 2, 2020

I don't think having more repositories to mantain is a good idea.

@VincentLanglet
Copy link
Member

I don't think having more repositories to mantain is a good idea.

One day, we’ll have a monorepository 😁

@phansys
Copy link
Member Author

phansys commented Jul 2, 2020

After a manual removal of the entry for "sonata-project/form-extensions" at symfony.lock and its files under vendor/sonata-project/form-extensions/, I can confirm that symfony/recipes-contrib#975 is working fine.
This was a problem in my check. SInce I was testing in a real environment where I already have other Sonata packages required, composer remove was not removing the dependency, so Flex was not updating either.

@phansys phansys marked this pull request as draft July 2, 2020 12:46
@SerheyDolgushev
Copy link

SerheyDolgushev commented Jul 2, 2020

So it is a bug on flex, or we are doing something wrong here?

@SerheyDolgushev can you help us here?

Sorry, I'm not sure what is going here, but in symfony/flex#612 Nicolas and Fabien decided to enable auto-discovery for all bundles by default. So extra.symfony.bundles is ignored. When any package is installed, the flex tries to find all bundle classes and enable it.

@SerheyDolgushev
Copy link

After a manual removal of the entry for "sonata-project/form-extensions" at symfony.lock and its files under vendor/sonata-project/form-extensions/, I can confirm that symfony/recipes-contrib#975 is working fine.
This was a problem in my check. SInce I was testing in a real environment where I already have other Sonata packages required, composer remove was not removing the dependency, so Flex was not updating either.

When I test flex for packages, I do the following:

  1. Make sure I run composer remove <vendor>/<package> without --no-scripts option. Otherwise, the removed package will still be presented in symfony.lock
  2. Remove the package files from vendor/<vendor>/<package>.
  3. Run composer require <vendor>/<package> without --no-scripts. Flex will be skipped if --no-scripts option is passed.

@phansys
Copy link
Member Author

phansys commented Jul 2, 2020

Thank you so much for the response and the tips @SerheyDolgushev. That is what I'm doing now.
The problem in this case is what @franmomu has described before: #97 (comment).
Given the path where Sonata\Twig\SonataTwigBundle is declared, SymfonyBundle::isBundleClass() is returning false.

@phansys phansys dismissed stale reviews from core23, VincentLanglet, and jordisala1991 via cb7e123 July 2, 2020 15:00
@phansys
Copy link
Member Author

phansys commented Jul 2, 2020

I'll try later to check if adding "Sonata\\Twig\\Bridge\\Symfony": "src/Bridge/Symfony/" to "autoload.psr-4" helps.

@phansys
Copy link
Member Author

phansys commented Jul 2, 2020

These are the classpaths where Flex is searching for the bundle declaration with the original "autoload.psr-4" configuration:

"Namespace: Sonata\Twig\"
"Class: Sonata\Twig\TwigBundle"
"Path: src/"
"ClassPath: vendor/sonata-project/twig-extensions/src//TwigBundle.php"
"Namespace: Sonata\Twig\"
"Class: Sonata\Twig\SonataTwigBundle"
"Path: src/"
"ClassPath: vendor/sonata-project/twig-extensions/src//SonataTwigBundle.php"

This is the result adding "Sonata\\Twig\\Bridge\\Symfony": "src/Bridge/Symfony/":

"ClassPath: vendor/sonata-project/twig-extensions/src/Bridge/Symfony//SymfonyBundle.php"
"Namespace: Sonata\Twig\Bridge\Symfony\"
"Class: Sonata\Twig\Bridge\Symfony\SonataSymfonyBundle"
"Path: src/Bridge/Symfony/"
"ClassPath: vendor/sonata-project/twig-extensions/src/Bridge/Symfony//SonataSymfonyBundle.php"
"Namespace: Sonata\Twig\Bridge\Symfony\"
"Class: Sonata\Twig\Bridge\Symfony\TwigSymfonyBundle"
"Path: src/Bridge/Symfony/"
"ClassPath: vendor/sonata-project/twig-extensions/src/Bridge/Symfony//TwigSymfonyBundle.php"
"Namespace: Sonata\Twig\Bridge\Symfony\"
"Class: Sonata\Twig\Bridge\Symfony\SonataTwigSymfonyBundle"
"Path: src/Bridge/Symfony/"
"ClassPath: vendor/sonata-project/twig-extensions/src/Bridge/Symfony//SonataTwigSymfonyBundle.php"
"Namespace: Sonata\Twig\Bridge\Symfony\"
"Class: Sonata\Twig\Bridge\Symfony\BridgeSymfonyBundle"
"Path: src/Bridge/Symfony/"
"ClassPath: vendor/sonata-project/twig-extensions/src/Bridge/Symfony//BridgeSymfonyBundle.php"
"Namespace: Sonata\Twig\Bridge\Symfony\"
"Class: Sonata\Twig\Bridge\Symfony\SonataTwigBridgeSymfonyBundle"
"Path: src/Bridge/Symfony/"
"ClassPath: vendor/sonata-project/twig-extensions/src/Bridge/Symfony//SonataTwigBridgeSymfonyBundle.php"

Based on these results, I think the nearest name we could use is SonataTwigSymfonyBundle, in case we want to adapt the codebase to this limitation.

@phansys phansys requested a review from jordisala1991 July 2, 2020 16:15
@phansys
Copy link
Member Author

phansys commented Jul 2, 2020

I've created this PoC, and its working as expected.
@sonata-project/contributors, please let me know what you think.

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.

7 participants