Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Support use of hyphen in assets package name#28128
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Support for using hyphens on yaml config for packages names, according to issuesymfony#28122 (comment)
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can you please enhance the commit message, and update the PR description according to the required template (filled in)?
seehttps://github.com/symfony/symfony/blob/master/.github/PULL_REQUEST_TEMPLATE.md
| ->fixXmlConfig('package') | ||
| ->children() | ||
| ->arrayNode('packages') | ||
| ->arrayNode('packages')->normalizeKeys(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be put on a separate line
fabpot commentedSep 4, 2018
Some test should be added as well (example from the related issue would work well here). |
damaya commentedSep 4, 2018
Sure, sorry the delay in my answer, I'll update the PR with required data. |
fabpot commentedOct 10, 2018
@damaya Will you have time in the coming days/weeks to finish this PR? |
| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no <!-- seehttps://symfony.com/bc -->| Deprecations? | no| Tests pass? | yes (Manual tests only)| Fixed tickets |symfony#28122| License | MIT| Doc PR | n/aAccording to issuesymfony/symfony-docs#10442, we tested in a demo bundle, for example in src/AppBundle/Resources/config/config.yml a package using hyphens: app-client-frontend, and withouth the patch it fails because the package is not recognized. With the patch, it works as expected.```framework: assets: packages: app-client-frontend: version: "%env(FRONTEND_VERSION)%" version_format: '%%2$s/dist/%%1$s' base_urls: - "%env(FRONTEND_URL)%"```
damaya left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| Q | A |
|---|---|
| Branch? | master |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes (Manual tests only) |
| Fixed tickets | #28122 |
| License | MIT |
| Doc PR | n/a |
According to issuesymfony/symfony-docs#10442, we tested in a demo bundle, for example in src/AppBundle/Resources/config/config.yml a package using hyphens: app-client-frontend, and withouth the patch it fails because the package is not recognized. With the patch, it works as expected.
framework:assets:packages:app-client-frontend:version:"%env(FRONTEND_VERSION)%"version_format:'%%2$s/dist/%%1$s'base_urls: -"%env(FRONTEND_URL)%"
damaya commentedOct 10, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Hi@fabpot, I updated my commit message and use a new line to improve the syntax. I just did manual tests and this patch worked as expected. |
damaya commentedOct 12, 2018
Hi there, the latest commit is ok? do you have any feedback about this? thanks in advance. |
fabpot commentedOct 12, 2018
@damaya I still think that we need to add a test to avoid any regressions in the future. Can you do that or do you need help? |
damaya commentedOct 12, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
nicolas-grekas commentedJan 28, 2019
…ame (damaya, XuruDragon)This PR was merged into the 3.4 branch.Discussion----------[FrameworkBundle] Support use of hyphen in asset package nameThis PR is a continuity of#28128| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets |#29122| License | MIT| Doc PR | n/aAccording to issuesymfony/symfony-docs#10442, we tested in a demo bundle, for example in src/AppBundle/Resources/config/config.yml a package using hyphens: app-client-frontend, and withouth the patch it fails because the package is not recognized. With the patch, it works as expected.```yamlframework: assets: packages: app-client-frontend: version: "%env(FRONTEND_VERSION)%" version_format: '%%2$s/dist/%%1$s' base_urls: - "%env(FRONTEND_URL)%"```Commits-------5c58b6e Add PackageNameTest to ConfigurationTest also add in the changelog the corresponding entry to this PR30b6a4f Support use of hyphen in asset package name
Uh oh!
There was an error while loading.Please reload this page.
According to issuesymfony/symfony-docs#10442, we tested in a demo bundle, for example in src/AppBundle/Resources/config/config.yml a package using hyphens: app-client-frontend, and withouth the patch it fails because the package is not recognized. With the patch, it works as expected.