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.

Sign up for GitHub

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

Jump to bottom

Add support for the new bundle structure #1085

Merged
merged 5 commits into from
Dec 13, 2019
Merged

Add support for the new bundle structure #1085

merged 5 commits into from
Dec 13, 2019

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Dec 10, 2019 •
edited
Loading

https://github.com/symfony/symfony/blob/4.4/UPGRADE-4.4.md#httpkernel

This adds support to find Contao resources in the bundle root folder. It also uses the new folder for the automatically generated asset helpers. I'm not sure if I forgot any place where we retrieve files?

@aschempp aschempp added this to the 4.9 milestone Dec 10, 2019
@aschempp aschempp self-assigned this Dec 10, 2019
@fritzmg
Copy link
Contributor

fritzmg commented Dec 10, 2019

Hm, what about the public/ path of the application? I guess we cannot really support that, since the Contao Managed Edition application's "public/" path is actually the web/ path, right?

@aschempp
Copy link
Member Author

We don't need to support a public path of the application. That's a configuration in your config.yml (see https://github.com/terminal42/contao-standard/blob/master/config/config.yml#L24 for an example)

Toflar
Toflar previously approved these changes Dec 11, 2019
View reviewed changes
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Nice one!

leofeyer
leofeyer reviewed Dec 11, 2019
View reviewed changes
core-bundle/src/DependencyInjection/Compiler/AddAssetsPackagesPass.php Show resolved Hide resolved
core-bundle/tests/DependencyInjection/Compiler/AddAssetsPackagesPassTest.php Outdated
public function testUsesTheJsonManifestVersionStrategyForBundles(): void
{
$bundlePath = static::getTempDir().'/ManifestJsonBundle';

Copy link
Member

Choose a reason for hiding this comment

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

CS: no empty line

@leofeyer
Copy link
Member

Do we really need another fixtures bundle or can we also test this with the existing test-bundle?

@fritzmg
Copy link
Contributor

fritzmg commented Dec 11, 2019

We don't need to support a public path of the application. That's a configuration in your config.yml (see https://github.com/terminal42/contao-standard/blob/master/config/config.yml#L24 for an example)

Well yes, I am just asking if may be we should change the Managed Edition in that regards by default. Then the bundle structure and application structure would be consistent. Otherwise we will have to document another exception.

@aschempp
Copy link
Member Author

@fritzmg I don't understand what you're referring to. There is no public folder in the application, and it does not make sense to create an asset instance for the app.

@fritzmg
Copy link
Contributor

fritzmg commented Dec 11, 2019 •
edited
Loading

@fritzmg I don't understand what you're referring to. There is no public folder in the application, and it does not make sense to create an asset instance for the app.

There is in a regular symfony skeleton. That's where the index.php and other public assets or entry points of the application will reside. In the Contao Managed Edition it's in web/. This will then differ from the bundle structure.

@aschempp
Copy link
Member Author

Ah, you mean that the web folder should be renamed as well? I agree, but not sure if that's part of this PR.

@fritzmg fritzmg mentioned this pull request Dec 11, 2019
Closed
@leofeyer
Copy link
Member

I agree, but not sure if that's part of this PR.

It is not.

leofeyer
leofeyer requested changes Dec 11, 2019
View reviewed changes
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

The logic of the passes needs to be changed IMHO (see https://github.com/contao/contao/pull/1085/files#r356659931).

leofeyer
leofeyer requested changes Dec 12, 2019
View reviewed changes
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

You did not answer my question:

Do we really need another fixtures bundle or can we also test this with the existing test-bundle?

core-bundle/tests/DependencyInjection/Compiler/AddAssetsPackagesPassTest.php Show resolved Hide resolved
@leofeyer leofeyer merged commit 8ed2005 into master Dec 13, 2019
@leofeyer leofeyer deleted the feature/resources branch December 13, 2019 10:58
@leofeyer
Copy link
Member

Thank you @aschempp.

@leofeyer leofeyer changed the title Added support for new bundle structure Add support for the new bundle structure Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofeyer leofeyer leofeyer approved these changes

@Toflar Toflar Toflar left review comments

@ausi ausi Awaiting requested review from ausi

@bytehead bytehead Awaiting requested review from bytehead

Assignees

@aschempp aschempp

Labels
feature
Projects
None yet
Milestone
4.9
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@aschempp @fritzmg @leofeyer @Toflar