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

[FrameworkBundle] Support use of hyphen in asset package name #30007

Merged
merged 2 commits into from Jan 29, 2019

Conversation

XuruDragon
Copy link

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.

framework:
    assets:
        packages:
            app-client-frontend:
                version: "%env(FRONTEND_VERSION)%"
                version_format: '%%2$s/dist/%%1$s'
                base_urls:
                  - "%env(FRONTEND_URL)%"

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 28, 2019
@nicolas-grekas nicolas-grekas changed the title Fix #28122 Support use of hyphen in asset package name [FrameworkBundle] Support use of hyphen in asset package name Jan 28, 2019
@nicolas-grekas
Copy link
Member

What about rebasing on 3.4 and consider it a bug fix?

damaya and others added 2 commits January 28, 2019 15:09
| 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)%"
```
@XuruDragon
Copy link
Author

@nicolas-grekas rebase on 3.4 is done

@fabpot
Copy link
Member

fabpot commented Jan 29, 2019

Thank you @XuruDragon.

@fabpot fabpot merged commit 5c58b6e into symfony:3.4 Jan 29, 2019
fabpot added a commit that referenced this pull request Jan 29, 2019
…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
@XuruDragon XuruDragon deleted the pr-28122 branch January 29, 2019 09:11
This was referenced Jan 29, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants