[FrameworkBundle] Added the assets helper again #17041
Conversation
| @@ -34,8 +34,7 @@ | |||
|
|
|||
| <service id="templating.helper.assets" class="Symfony\Bundle\FrameworkBundle\Templating\Helper\AssetsHelper"> | |||
There was a problem hiding this comment.
wait, we still have the service definition for a class which does not exist ?
There was a problem hiding this comment.
Yep, and we have this check in the FrameworkExtension https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L543-L547
|
Actually, I think that the AssetsHelper should be updated to use the new system in 2.8 already |
|
@stof Are you refering to the Asset component? Already in 2.8 we use the Asset component in the |
| protected function tearDown() | ||
| { | ||
| $this->helper = null; | ||
| } |
There was a problem hiding this comment.
this has no real value (unless I'm missing something?), so I propose to remove it. If it does add something, I propose to move it just below setUp.
There was a problem hiding this comment.
Indeed, there's no value in this method. Let's remove it.
There was a problem hiding this comment.
Well, actually I'm using the same packages, so maybe we can use setUpBeforeClass here.
There was a problem hiding this comment.
@dosten what for? Before each test setUp() is run and it sets a new value of the $helper property. There's no need for tearDown().
There was a problem hiding this comment.
We can use setUpBeforeClass because we use the same packages in each test, and remove the tearDown
|
Any news on this? can be merged? @fabpot |
|
I just stumbled upon this issue, getting a As mentioned in the PR description, according to the upgrade guides, the |
|
The UPGRADE file is wrong, in 3.0 the helper should provide integration with the Asset component. |
|
I just reviewed this PR and everything is fine, IMHO. Thanks for fixing this @dosten. I'm also creating a PR to fix the upgrade guides since those need to be fixed in 2.7. Status: Reviewed |
|
👍 |
1 similar comment
|
👍 |
|
Thank you @dosten. |
This PR was merged into the 3.0 branch. Discussion ---------- [FrameworkBundle] Added the assets helper again | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | License | MIT This PR is a follow up of #14972, we deprecated and removed the AssetsHelper in 2.7/3.0 doing impossible to use the Asset component and the PHP templates together, I've submitted this PR to be merged in 3.0 because IMO this is a bug fix, but we documented the deprecation and removal of the helper, what we should do here? (https://github.com/symfony/symfony/blob/3.0/UPGRADE-3.0.md#frameworkbundle and https://github.com/symfony/symfony/blob/2.8/UPGRADE-2.7.md#frameworkbundle) cc/ @wouterj Commits ------- 98cb838 Added the assets helper again
…s removal of assets helper (regularjack) This PR was merged into the 2.7 branch. Discussion ---------- [FrameworkBundle] Fix upgrade guides concerning erroneous removal of assets helper | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As mentioned in #17041, the upgrade guides for 2.7 and 3.0 erroneously mention that the `templating.helper.assets` service is removed in 3.0. #17041 cannot fix these issues since it targets 3.0. This PR should only be merged after #17041 since it depends on it. Commits ------- 5abac56 Fix upgrade guides concerning erroneous removal of assets helper
This PR is a follow up of #14972, we deprecated and removed the AssetsHelper in 2.7/3.0 doing impossible to use the Asset component and the PHP templates together, I've submitted this PR to be merged in 3.0 because IMO this is a bug fix, but we documented the deprecation and removal of the helper, what we should do here? (https://github.com/symfony/symfony/blob/3.0/UPGRADE-3.0.md#frameworkbundle and https://github.com/symfony/symfony/blob/2.8/UPGRADE-2.7.md#frameworkbundle)
cc/ @wouterj