8000 [TwigBridge] use proper class to fetch asset version strategy property by xabbuh · Pull Request #14708 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[TwigBridge] use proper class to fetch asset version strategy property #14708

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
Merged
merged 1 commit into from
May 21, 2015
Merged

[TwigBridge] use proper class to fetch asset version strategy property #14708

merged 1 commit into from
May 21, 2015

Conversation

xabbuh
Copy link
Member
@xabbuh xabbuh commented May 20, 2015
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #14692
License MIT
Doc PR

@xabbuh xabbuh mentioned this pull request May 20, 2015
Closed
@linaori
Copy link
Contributor
linaori commented May 20, 2015

I'll see if this fixes the issue tomorrow, thanks for the quick patch!

Lines 110 and 113 feature $v->setAccessible(true);, this could be pushed down to just below the if-statement.

@xabbuh
Copy link
Member Author
xabbuh commented May 20, 2015

@iltar good catch, thanks

@xabbuh xabbuh added the Ready label May 20, 2015
@dosten
Copy link
Contributor
dosten commented May 20, 2015

👍

nicolas-grekas
nicolas-grekas reviewed May 20, 2015
View reviewed changes
src/Symfony/Bridge/Twig/Extension/AssetExtension.php
$v = $class->getProperty('versionStrategy');
} else {
$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.

You could simplify the whole if/else block with:

$class = new \ReflectionClass($package);

while ('Symfony\Component\Asset\Package' !== $class->getName()) {
    $class = $class->getParentClass();
}

$v = $class->getProperty('versionStrategy');

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, done

@nicolas-grekas
Copy link
Member

👍

@fabpot
Copy link
Member
fabpot commented May 21, 2015

Thank you @xabbuh.

@fabpot fabpot merged commit 01b7dd6 into symfony:2.7 May 21, 2015
fabpot added a commit that referenced this pull request May 21, 2015
@fabpot
bug #14708 [TwigBridge] use proper class to fetch asset version strat…
b248bd1 
…egy property (xabbuh)

This PR was merged into the 2.7 branch.

Discussion
----------

[TwigBridge] use proper class to fetch asset version strategy property

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

Commits
-------

01b7dd6 [TwigBridge] use proper class to fetch asset version strategy property
@xabbuh xabbuh deleted the issue-14692 branch May 21, 2015 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
5365
Assignees
No one assigned
Labels
Ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@xabbuh @linaori @dosten @nicolas-grekas @fabpot
0