10000 [AssetsHelper] use proper class to fetch asset version strategy property by twencl · Pull Request #17260 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[AssetsHelper] use proper class to fetch asset version strategy property #17260

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
Closed
wants to merge 1 commit into from
Closed

[AssetsHelper] use proper class to fetch asset version strategy property #17260

wants to merge 1 commit into from

Conversation

twencl
Copy link
@twencl twencl commented Jan 4, 2016
Q A
Bug fix? [yes]
New feature? [no]
BC breaks? [no]
Deprecations? no]
Tests pass? [yes]
Fixed tickets [https://github.com//issues/17259]
License MIT
Doc PR

@xabbuh
Copy link
Member
xabbuh commented Jan 5, 2016

Can you please also add a test?

nicolas-grekas
nicolas-grekas reviewed Jan 5, 2016
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/Templating/Helper/AssetsHelper.php

$v = new \ReflectionProperty($package, 'versionStrategy');
Copy link
Member

Choose a reason for hiding this comment

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

why can't we just do $v = new \ReflectionProperty('Symfony\Component\Asset\Package', 'versionStrategy'); instead of the while loop?

@fabpot
Copy link
Member
fabpot commented Jan 7, 2016

@twencl Do you have time to finish this one? Thanks.

@twencl
Copy link
Author
twencl commented Jan 7, 2016

@fabpot Sorry. I'm slammed for the next couple days. If anyone can pick it up in my stead, that would be awesome. Otherwise it'll be next week for me.

@dosten dosten mentioned this pull request Jan 7, 2016
Closed
@dosten
Copy link
Contributor
dosten commented Jan 7, 2016

See #17298 (tests added)

@fabpot
Copy link
Member
fabpot commented Jan 7, 2016

closing in favor of #17298

@fabpot fabpot closed this Jan 7, 2016
fabpot added a commit that referenced this pull request Jan 7, 2016
@fabpot
bug #17298 [FrameworkBundle] Use proper class to fetch $versionStrate…
4d536bd 
…gy property (dosten)

This PR was submitted for the 2.8 branch but it was merged into the 2.7 branch instead (closes #17298).

Discussion
----------

[FrameworkBundle] Use proper class to fetch $versionStrategy property

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17259
| License       | MIT

This is a replacement of #17260

Commits
-------

e38fa13 Use proper class to fetch $versionStrategy property
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@twencl @xabbuh @fabpot @dosten @nicolas-grekas @carsonbot
0