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

Support modern bundle structure #893

Merged
merged 1 commit into from
Jan 6, 2023

Conversation

jdreesen
Copy link
Contributor

Q A
Bug fix? no
New feature? yes
Doc updated no
BC breaks? depends
Deprecations? no
Tests pass? yes/no
Fixed tickets #...
License MIT

Symfony changed its "Directory Structure Best Practice for Reusable Bundles" in its 4.4 version.

In the old one, everything was just in the bundle root dir / (or maybe inside /src, in which case this was considered "root").
In the new one, the PHP classes moved inside /src and the contents of /Resources are unpacked into the root / (with sometimes different names).

Below is a comparison of the old and the new structure:

Symfony < 4.4 Symfony >= 4.4
/
├─ AcmeBlogBundle.php
├─ Controller/
├─ README.md
├─ LICENSE
├─ Resources/
│  ├─ config/
│  ├─ doc/
│  │  └─ index.rst
│  ├─ translations/
│  ├─ views/
│  └─ public/
└─ Tests/
/
├─ config/
├─ docs/
│  └─ index.md
├─ public/
├─ src/
│  ├─ Controller/
│  ├─ DependencyInjection/
│  └─ AcmeBlogBundle.php
├─ templates/
├─ tests/
├─ translations/
├─ LICENSE
└─ README.md

JMSSerializerBundle doesn't support the new bundle structure when doing metadata autodetection at the moment, because it doesn't look at the new location for the serializer config, which would be /config/serializer instead of /Resources/config/serializer. The solution is to look in both possible location, as Symfony does e.g. here, here, here or here.

Explicitly configured metadata directories don't work either, because JMSSerializerBundle assumes the *Bundle class as the bundle root (instead of using the value returned by *Bundle::getPath() (as described here)) which means for the new bundle structure one has to currently use @MyBundle/../config/serializer because the *Bundle class is inside the /src folder. After this PR got merged, it would be @MyBundle/config/serializer, which could be considered as a BC break, although I'd say it is a bugfix.

@jdreesen jdreesen marked this pull request as draft June 28, 2022 13:22
@jdreesen jdreesen force-pushed the support-modern-bundle-structure branch from e500927 to 23bd20b Compare June 28, 2022 13:40
@jdreesen jdreesen marked this pull request as ready for review June 28, 2022 13:42
Copy link
Collaborator

@scyzoryck scyzoryck left a comment

Choose a reason for hiding this comment

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

Thanks for contribution! I left one comment.

@jdreesen
Copy link
Contributor Author

I don't think the CI failure is related to this PR.

@jdreesen
Copy link
Contributor Author

Should be fixed by #901

@jdreesen jdreesen force-pushed the support-modern-bundle-structure branch 2 times, most recently from 92b83ca to 91f0f12 Compare September 13, 2022 09:21
@goetas goetas modified the milestone: 5.0 Sep 13, 2022
@goetas
Copy link
Collaborator

goetas commented Sep 14, 2022

An issue with merging this is that the https://jmsyst.com/bundles/JMSSerializerBundle site wont work anymore as expected :(

@jdreesen
Copy link
Contributor Author

Huh? Why?

@goetas
Copy link
Collaborator

goetas commented Sep 14, 2022

that is the official website used for the jms projects. the documentation is auto generated from the repositories in https://github.com/schmittjoh/.

but that project is veeeeeeeeery old and i'm even scared of touching it to add new paths.... :(

@jdreesen
Copy link
Contributor Author

How is it generated? Can you point me to the repo? Maybe I can have a look...

At the moment, I don't understand which paths need to be added and why.

@goetas
Copy link
Collaborator

goetas commented Sep 14, 2022

it is a private repo :(

@jdreesen
Copy link
Contributor Author

Oh, then I can't help, I guess... :/

@mbabker
Copy link
Contributor

mbabker commented Nov 29, 2022

An issue with merging this is that the https://jmsyst.com/bundles/JMSSerializerBundle site wont work anymore as expected :(

That shouldn't be a problem. This PR isn't changing anything about this bundle's filesystem layout, it's only adding support for other bundles that use the newer suggested filesystem layout.

@jdreesen
Copy link
Contributor Author

This PR isn't changing anything about this bundle's filesystem layout, it's only adding support for other bundles that use the newer suggested filesystem layout.

That's correct.

@jdreesen
Copy link
Contributor Author

jdreesen commented Jan 6, 2023

@goetas does @mbabker's comment change anything about the ability of merging this PR?

@goetas
Copy link
Collaborator

goetas commented Jan 6, 2023

@mbabker the issue is that the current documentation generation project expects the bundle docs to be in Resources/doc, not in docs

@mbabker
Copy link
Contributor

mbabker commented Jan 6, 2023

I got that. But, the docs are only being generated for this bundle.

We aren't proposing moving the files in this bundle around, breaking the documentation generator. We are proposing updating this bundle to support the updated Symfony best practices by being able to scan multiple paths in other bundles.

If the changes in the DI extension class in this PR are breaking the documentation generator, then there's something really weird happening behind the scenes.

@jdreesen
Copy link
Contributor Author

jdreesen commented Jan 6, 2023

@goetas so what can we do to move this forward?

the issue is that the current documentation generation project expects the bundle docs to be in Resources/doc, not in docs

If this is the only concern, then I don't think it stops us from merging, because it doesn't change the structure of this bundle at all.

@goetas
Copy link
Collaborator

goetas commented Jan 6, 2023

ok, lets merge this and fix stuff later if gets broken.

@jdreesen do you mind rebasign this? as some stuff changed recently , just to be on the safe side

@jdreesen jdreesen force-pushed the support-modern-bundle-structure branch from 91f0f12 to 6d60950 Compare January 6, 2023 15:55
@jdreesen
Copy link
Contributor Author

jdreesen commented Jan 6, 2023

done

@goetas goetas merged commit 437d5e6 into schmittjoh:master Jan 6, 2023
@goetas
Copy link
Collaborator

goetas commented Jan 6, 2023

thanks!

@jdreesen jdreesen deleted the support-modern-bundle-structure branch January 6, 2023 15:57
@goetas
Copy link
Collaborator

goetas commented Jan 9, 2023

I have to admit that I've misunderstood initially this PR, my apologies.

Was different than what I've imagined. Thanks for the patience

@jdreesen
Copy link
Contributor Author

jdreesen commented Jan 9, 2023

No problem, I'm happy that it's finally merged :)

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.

4 participants