Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Dynamic bundle assets#31975

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:4.4fromgarak:dynamic-bundle-assets
Jul 9, 2019
Merged

Conversation

@garak
Copy link
Contributor

@garakgarak commentedJun 10, 2019•
edited
Loading

QA
Branch?4.4
Bug fix?no
New feature?yes
BC breaks?no (new method in interface as annotation)
Deprecations?no
Tests pass?yes
Fixed tickets#29213
LicenseMIT
Doc PRnone (yet)

Everything is explained in linked issue

@derrabus
Copy link
Member

This PR should target the 4.4 branch. If you only add a@method annotation to theBundleInterface instead of the real method, it's not a BC break anymore.

@garak
Copy link
ContributorAuthor

This PR should target the 4.4 branch. If you only add a@method annotation to theBundleInterface instead of the real method, it's not a BC break anymore.

So, should I close this PR and open another one on 4.4?

@fabpot
Copy link
Member

@garak No, you change the target of this PR without creating a new one.

@garakgarak changed the base branch frommaster to4.4June 20, 2019 06:50
@garak
Copy link
ContributorAuthor

@fabpot I just tried to change target to 4.4. and a bunch of stuff popped up.
Maybe a new PR is a better option... unless you can suggest a way to get rid of such mess
Thanks

@fabpot
Copy link
Member

@garak rebase your PR locally on 4.4 and push again, that should resolve the mess.

@garakgarakforce-pushed thedynamic-bundle-assets branch from535008f to33ec2d8CompareJune 20, 2019 13:08
@garak
Copy link
ContributorAuthor

I rebased upon 4.4.
I followed suggestion from@derrabus and replaced actual method with an annotation inside interface.
I also updated initial PR comment.

Hope everything is fine

@garak
Copy link
ContributorAuthor

I still see "Status: Needs Work" label. Should I do something else?

@garak
Copy link
ContributorAuthor

Friendly reminder: did needed work, do I need to do some more?

Copy link
Member

@fabpotfabpot left a comment

Choose a reason for hiding this comment

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

Having some tests for this new feature would be nice as well.

@garak
Copy link
ContributorAuthor

I addressed all notes from@fabpot, I guess. Some portions of code are not shown as modified, but they are, actually.
I also added a small expectation in tests, I couldn't find any direct tests of AssetsInstallCommand. Failures in CI are unrelated

@fabpotfabpotforce-pushed thedynamic-bundle-assets branch fromf99e81d toc16fcc9CompareJuly 9, 2019 06:13
@fabpot
Copy link
Member

Thank you@garak.

garak reacted with thumbs up emoji

@fabpotfabpot merged commitc16fcc9 intosymfony:4.4Jul 9, 2019
fabpot added a commit that referenced this pull requestJul 9, 2019
This PR was squashed before being merged into the 4.4 branch (closes#31975).Discussion----------Dynamic bundle assets| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no (new method in interface as annotation)| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#29213| License       | MIT| Doc PR        | none (yet)Everything is explained in linked issueCommits-------c16fcc9 Dynamic bundle assets
@garakgarak deleted the dynamic-bundle-assets branchJuly 9, 2019 06:38
fabpot added a commit that referenced this pull requestJul 12, 2019
…ereguiluz)This PR was squashed before being merged into the 4.4 branch (closes#32452).Discussion----------[Bundles] Rename getPublicPath() as getPublicDir()| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->| BC breaks?    | no     <!-- seehttps://symfony.com/bc -->| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->| Fixed tickets | -| License       | MIT| Doc PR        | I'll add this if approvedWhile documenting#31975 (seesymfony/symfony-docs#11930) I realized that the `getPublicPath()` method name is not consistent with the rest of Symfony.In Symfony, "path" is usually associated to routes and we use "dir" for things similar to this:* `getCacheDir()` and `getLogdir()` to override Symfony structure (https://symfony.com/doc/current/configuration/override_dir_structure.html)* `binDir`, `configDir`, `srcDir`, `varDir`, `publicDir` in Symfony Flex recipes (https://github.com/symfony/recipes) to override the dir structureSo, this PR proposes to rename `getPublicPath()` as `getPublicDir()`Commits-------4ab2f99 [Bundles] Rename getPublicPath() as getPublicDir()

publicfunctiongetPublicPath():string
{
return'Resources/public';
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO that should return the absolute dir to be consistent with getLogDir etc,

return$this->getProjectDir().'/var/log';

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@Tobion here we're not in the project context, but in the bundle one. Anyway, bundle always returned such path, this is not changing anything

symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull requestAug 13, 2019
…on for bundle directories (yceruto)This PR was squashed before being merged into the 4.4 branch (closes #32845).Discussion----------[HttpKernel][FrameworkBundle] Add alternative convention for bundle directories| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony/symfony#32453| License       | MIT| Doc PR        | TODOWe already know that bundles must be compatible with many Symfony's versions, so it is very likely that current bundles won't be able to use this feature soon, unless they create symbolic links to support both structures.The point is that this is already happening, so in the future when our bundles stop to support <=4.3 then you'll be sure to change the current directory structure.We have recently added the `getPublicDir()` method insymfony/symfony#31975, here I'm removing it in favor of hardcoding a new convention.I've added some functional tests in which I've changed everything to this structure:```-- ModernBundle   |-- config/   |-- public/   |-- src/       |-- ModernBundle.php   |-- templates/   |-- translations/```WDYT?Commits-------6996e1cbe2 [HttpKernel][FrameworkBundle] Add alternative convention for bundle directories
nicolas-grekas added a commit that referenced this pull requestAug 13, 2019
…on for bundle directories (yceruto)This PR was squashed before being merged into the 4.4 branch (closes#32845).Discussion----------[HttpKernel][FrameworkBundle] Add alternative convention for bundle directories| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#32453| License       | MIT| Doc PR        | TODOWe already know that bundles must be compatible with many Symfony's versions, so it is very likely that current bundles won't be able to use this feature soon, unless they create symbolic links to support both structures.The point is that this is already happening, so in the future when our bundles stop to support <=4.3 then you'll be sure to change the current directory structure.We have recently added the `getPublicDir()` method in#31975, here I'm removing it in favor of hardcoding a new convention.I've added some functional tests in which I've changed everything to this structure:```-- ModernBundle   |-- config/   |-- public/   |-- src/       |-- ModernBundle.php   |-- templates/   |-- translations/```WDYT?Commits-------6996e1c [HttpKernel][FrameworkBundle] Add alternative convention for bundle directories
symfony-splitter pushed a commit to symfony/twig-bundle that referenced this pull requestAug 13, 2019
…on for bundle directories (yceruto)This PR was squashed before being merged into the 4.4 branch (closes #32845).Discussion----------[HttpKernel][FrameworkBundle] Add alternative convention for bundle directories| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony/symfony#32453| License       | MIT| Doc PR        | TODOWe already know that bundles must be compatible with many Symfony's versions, so it is very likely that current bundles won't be able to use this feature soon, unless they create symbolic links to support both structures.The point is that this is already happening, so in the future when our bundles stop to support <=4.3 then you'll be sure to change the current directory structure.We have recently added the `getPublicDir()` method insymfony/symfony#31975, here I'm removing it in favor of hardcoding a new convention.I've added some functional tests in which I've changed everything to this structure:```-- ModernBundle   |-- config/   |-- public/   |-- src/       |-- ModernBundle.php   |-- templates/   |-- translations/```WDYT?Commits-------6996e1cbe2 [HttpKernel][FrameworkBundle] Add alternative convention for bundle directories
symfony-splitter pushed a commit to symfony/doctrine-bridge that referenced this pull requestAug 13, 2019
…on for bundle directories (yceruto)This PR was squashed before being merged into the 4.4 branch (closes #32845).Discussion----------[HttpKernel][FrameworkBundle] Add alternative convention for bundle directories| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony/symfony#32453| License       | MIT| Doc PR        | TODOWe already know that bundles must be compatible with many Symfony's versions, so it is very likely that current bundles won't be able to use this feature soon, unless they create symbolic links to support both structures.The point is that this is already happening, so in the future when our bundles stop to support <=4.3 then you'll be sure to change the current directory structure.We have recently added the `getPublicDir()` method insymfony/symfony#31975, here I'm removing it in favor of hardcoding a new convention.I've added some functional tests in which I've changed everything to this structure:```-- ModernBundle   |-- config/   |-- public/   |-- src/       |-- ModernBundle.php   |-- templates/   |-- translations/```WDYT?Commits-------6996e1cbe2 [HttpKernel][FrameworkBundle] Add alternative convention for bundle directories
symfony-splitter pushed a commit to symfony/http-kernel that referenced this pull requestAug 13, 2019
…on for bundle directories (yceruto)This PR was squashed before being merged into the 4.4 branch (closes #32845).Discussion----------[HttpKernel][FrameworkBundle] Add alternative convention for bundle directories| Q             | A| ------------- | ---| Branch?       | 4.4| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |symfony/symfony#32453| License       | MIT| Doc PR        | TODOWe already know that bundles must be compatible with many Symfony's versions, so it is very likely that current bundles won't be able to use this feature soon, unless they create symbolic links to support both structures.The point is that this is already happening, so in the future when our bundles stop to support <=4.3 then you'll be sure to change the current directory structure.We have recently added the `getPublicDir()` method insymfony/symfony#31975, here I'm removing it in favor of hardcoding a new convention.I've added some functional tests in which I've changed everything to this structure:```-- ModernBundle   |-- config/   |-- public/   |-- src/       |-- ModernBundle.php   |-- templates/   |-- translations/```WDYT?Commits-------6996e1c [HttpKernel][FrameworkBundle] Add alternative convention for bundle directories
This was referencedNov 12, 2019
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@derrabusderrabusderrabus requested changes

@fabpotfabpotfabpot approved these changes

+2 more reviewers

@TobionTobionTobion left review comments

@jvasseurjvasseurjvasseur left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

5.0

Development

Successfully merging this pull request may close these issues.

8 participants

@garak@derrabus@fabpot@Tobion@jvasseur@xabbuh@chalasr@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp