[FrameworkBundle] Support use of hyphen in assets package name #28128
Conversation
Support for using hyphens on yaml config for packages names, according to issue symfony#28122 (comment)
There was a problem hiding this comment.
can you please enhance the commit message, and update the PR description according to the required template (filled in)?
see https://github.com/symfony/symfony/blob/master/.github/PULL_REQUEST_TEMPLATE.md
| @@ -631,7 +631,7 @@ private function addAssetsSection(ArrayNodeDefinition $rootNode) | |||
| ->end() | |||
| ->fixXmlConfig('package') | |||
| ->children() | |||
| ->arrayNode('packages') | |||
| ->arrayNode('packages')->normalizeKeys(false) | |||
There was a problem hiding this comment.
should be put on a separate line
|
Some test should be added as well (example from the related issue would work well here). |
|
Sure, sorry the delay in my answer, I'll update the PR with required data. |
|
@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 <!-- see https://symfony.com/bc --> | Deprecations? | no | Tests pass? | yes (Manual tests only) | Fixed tickets | symfony#28122 | License | MIT | Doc PR | n/a According to issue symfony/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)%" ```
There was a problem hiding this comment.
| 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 issue symfony/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)%"
|
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. |
chalasr
changed the title
|
Hi there, the latest commit is ok? do you have any feedback about this? thanks in advance. |
|
@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? |
…ame (damaya, XuruDragon) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle] Support use of hyphen in asset package name This 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/a According to issue symfony/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. ```yaml framework: 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 PR 30b6a4f Support use of hyphen in asset package name
According to issue symfony/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.