[3.0][FrameworkBundle] Asset helpers should not be in the request scope #11653
Comments
|
The issue is that we cannot change the PathPackage to avoid using the Request itself without breaking BC. This is why it is still in the request scope. Note that when you specify a base url for assets instead of getting the base path from the request, the generated UrlPackage is in the container scope. |
|
... and we already have a couple of PRs dealing with this issue. |
|
The
Edit: mistook |
|
@fabpot Didn't see any PR adressing this particular issue, or anything related to asset packages (apart from #10973) or scopes. In a console app, the workaround for now is not pretty at all: $request = new Request();
$locale = $this->getContainer()->getParameter('locale');
$request->setLocale($locale);
$this->getContainer()->set('request', $request);
$this->getContainer()->get('translator')->setLocale($locale);
$this->getContainer()->enterScope('request');
$template = $this->getContainer()->get('twig')->render('AcmeDemoBundle:Email:email.html.twig');
$this->getContainer()->leaveScope('request'); |
|
@csarrazi but changing the way we depend on the request in the extended PathPackage requires changing the base class of the component too (if you get the request from the request stack in the constructor, you have the same issue than currently). and breaking compatibility is something we can do in 3.0, but not in 2.6. This is why we are still using the request scope currently (we are working on the 2.6 development, not on the 3.0 one). There was an attempt to stop using the request scope here when we introduced the RequestStack in Symfony, but we did not manage to do the refactoring in a BC way. |
|
@stof Sorry about BC. I was thinking about In any case, PathPackage is not part of the api, and the change I intended to do was to inject RequestStack. In any case, the CLI needs some love. Too many hacks are needed in order to be able to generate Uris, generate asset Uris, etc. |
|
@csarrazi Not being flagged as Generating Uris in the console does not require any hack. It only requires a small configuration, which is necessary because we cannot guess on which domain the application is running, and whether it is in a subfolder of the document root, given that the app is not accessed through HTTP: http://symfony.com/doc/current/cookbook/console/sending_emails.html#configuring-the-request-context-globally (the default value if not configured would generate a link Generating asset uris in the CLI is indeed more complex when your asset base urls are guessed based on the request (when you configure a base url when using a CDN for instance, there is strictly nothing to change to generate asset urls in the CLI) |
|
It's funny (or not), But we (me and @tucksaun) stubble upon the same issue. we solve this issue by a custom compiler pass in our bundle: <?php
namespace SensioLabs\Bundle\ToolkitBundle\DependencyInjection\Compiler;
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\Reference;
/**
* As we hook-up into the assets helper config, Symfony does not handle
* correctly the scope of templating.helper.assets.
* So we try to set-up the higher scope for this service.
*
* @see https://github.com/symfony/symfony/issues/11653
*/
class AssetsHelperPass implements CompilerPassInterface
{
public function process(ContainerBuilder $container)
{
if (!$container->hasDefinition('templating.helper.assets')) {
return;
}
$assetsHelper = $container->getDefinition('templating.helper.assets');
$defaultScope = $this->getPackageScope($container, $assetsHelper->getArgument(0));
$assetsHelper->setScope($defaultScope);
foreach ($assetsHelper->getArgument(1) as $arg) {
if ('request' === $this->getPackageScope($container, $arg)) {
$assetsHelper->setScope('request');
break;
}
}
}
private function getPackageScope(ContainerBuilder $container, Reference $package)
{
$definition = $container->getDefinition((string) $package);
return $definition->getScope();
}
}I think we could safely move some code from the |
|
@lyrixx this looks good for 2.6. Can you send a PR ? |
|
yep ;) |
…e as late as possible (lyrixx) This PR was merged into the 2.6-dev branch. Discussion ---------- [FrameworkBundle] Determine templating.engine.php scope as late as possible | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #11653 | License | MIT | Doc PR | - Commits ------- 169dadd [FrameworkBundle] Determine templating.engine.php scope as late as possible
Right now, using the
assettwig function results in the following error:As one may want to rely on the asset helper to generate URIs outside from a request (i.e., in a command-line, for sending e-mails), it should not depend on the
requestservice, but rather onrequest_stack, in order to be available in a wider scope.This would also allow the simplification of
Symfony\Bundle\FrameworkBundle\DependencyInjection\FrameworkExtension, by removing calls to thesetScope()methods, once support for scopes is dropped for 3.0.The text was updated successfully, but these errors were encountered: