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

[Asset] added the component #13234

Merged
merged 4 commits into from Feb 10, 2015
Merged

[Asset] added the component #13234

merged 4 commits into from Feb 10, 2015

Conversation

@fabpot
Copy link
Member

fabpot commented Jan 4, 2015

Q A
Bug fix? yes
New feature? yes
BC breaks? no
Deprecations? yes
Tests pass? yes
Fixed tickets #10973, #11748, #11876, #4883, #12474
License MIT
Doc PR not yet

TODO:

  • submit documentation PR

The current Asset sub-namespace in Templating has several (major) problems:

  • It does not cover all use cases (see #10973 and #4883 for some example)
  • It has some design issues (relies on the Request instance and so requires the request scope, very coupled with the PHP templating sub-system, see #11748 and #11876)

To decouple this feature and make it reusable in Silex for instance, and to fix the design issues and make it more extensible, I've decided to extract and rework the features provided into a new Asset component.

Basically, this component allows the developer to easily manage asset URLs: versioning, paths, and hosts.

Both the new and the old asset management features are kept in this PR to avoid breaking BC; the old system is of course deprecated and automatically converted to the new one.

Even if the features are quite similar, and besides the flexilibity of the new system, here are some differences:

  • PathPackage always prepend the path (even if the given path starts with /).
  • Usage is stricter (for instance, PathPackage requires a basePath to be passed and UrlPackage requires that at least on URL is passed).
  • In the configuration, named packages inherits from the version and version format of the default package by default.
  • It is not possible to override the version when asking for a URL (instead, you can define your own version strategy implementation -- the use cases explained in #6092 are easily implemented this way).
  • It's not possible to generate absolute URLs (see #13264 for a better alternative using composition; so using absolute_url(asset_path('me.png')) should work).
    #10973 was about adding shortcuts for bundles, which is a good idea; but given that you rarely reference built-in or third-party bundle assets and because we now have a one-bundle default approach named AppBundle, the same can be achieved with just a simple piece of configuration with the new assets feature:
framework:
    assets:
        packages:
            app:
                base_path: /bundles/app/
            img:
                base_path: /bundles/app/images/

Then:

{{ asset('images/me.png', 'app') }}
# /bundles/app/images/me.png

{{ asset('me.png', 'img') }}
# /bundles/app/images/me.png

#12474 discussed the possibility to add a version for absolute URL. It's not possible to do that in a generic way as the version strategy involves both the version and the path, which obviously cannot work when the path is an absolute URL already. Instead, one should use the asset_version Twig function to add the version manually.

@fabpot fabpot force-pushed the fabpot:asset-component branch 8 times, most recently from e716f76 to c9e9565 Jan 4, 2015
@fabpot fabpot force-pushed the fabpot:asset-component branch 2 times, most recently from 10c720c to 95f7b7f Jan 4, 2015
->canBeUnset()
->fixXmlConfig('base_url')
->children()
->scalarNode('version')->defaultValue(null)->end()

This comment has been minimized.

Copy link
@acasademont

acasademont Jan 5, 2015

Contributor

wouldn't it be better defaultNull?

->arrayNode('packages')
->useAttributeAsKey('name')
->prototype('array')
->fixXmlConfig('base_url')

This comment has been minimized.

Copy link
@acasademont

acasademont Jan 5, 2015

Contributor

could all this config below share the same code as the one before? extract it as a function maybe?

This comment has been minimized.

Copy link
@stof

stof Jan 19, 2015

Member

We could indeed use a private method returning an array node, and then using ->append() in both places

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 19, 2015

Author Member

Not sure it would bring us a lot.


$this->createPackageDefinitions($config, $container);

// PHP templating engine

This comment has been minimized.

Copy link
@acasademont

acasademont Jan 5, 2015

Contributor

should this be removed?

@acasademont
Copy link
Contributor

acasademont commented Jan 5, 2015

I like the overall simplification, no more weird behaviour depending on wether your packages contain ssl/non-ssl urls.

"php": ">=5.3.3"
},
"suggest": {
"symfony/http-foundation": ""

This comment has been minimized.

Copy link
@mickaelandrieu

mickaelandrieu Jan 5, 2015

Contributor

👍 thanks for this "totaly" standalone component !

@fabpot fabpot force-pushed the fabpot:asset-component branch 8 times, most recently from 6439559 to b0bc46f Jan 5, 2015
@@ -442,6 +444,54 @@ private function addTemplatingSection(ArrayNodeDefinition $rootNode)
;
}

private function addAssetsSection(ArrayNodeDefinition $rootNode)
{

This comment has been minimized.

@@ -311,6 +312,7 @@ private function addRequestSection(ArrayNodeDefinition $rootNode)

private function addTemplatingSection(ArrayNodeDefinition $rootNode)
{
/** @deprecated since 2.7, will be removed in 3.0 */

This comment has been minimized.

Copy link
@stof

stof Jan 5, 2015

Member

the ->info() of the node should be updated to mark it as deprecated when dumping the config reference

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 5, 2015

Author Member

done

$oldAssetsConfigured = false;
if (
isset($config['templating'])
&&

This comment has been minimized.

Copy link
@stof

stof Jan 5, 2015

Member

too much indentation here

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 5, 2015

Author Member

done

(
count($config['templating']['packages'])
||
count($config['templating']['assets_base_urls']['http'])

This comment has been minimized.

Copy link
@stof

stof Jan 5, 2015

Member

I would use !empty(...), which would also remove the need for the initial isset($config['templating'])

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 5, 2015

Author Member

done


if ($container->has('assets.packages')) {
$container->getDefinition('twig.extension.assets')->addTag('twig.extension');
} elseif ($container->has('templating.asset.packages')) {

This comment has been minimized.

Copy link
@stof

stof Jan 5, 2015

Member

the issue with this logic is that it does not allow to have both the new asset_path() function and the old asset() function in the same project, forcing an all-or-nothing upgrade. This means that the whole bundle ecosystem has to upgrade at the same time to be able to be used together in a project.

IMO, the old extension should always be registered for BC reasons

This comment has been minimized.

Copy link
@stof

stof Jan 5, 2015

Member

The alternative solution is to have the new extension providing a BC layer for the asset() extension

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 5, 2015

Author Member

I've done that because the new function was named asset as well initially. Now that the name is different, it's easier to support both. I will update the patch.

This comment has been minimized.

Copy link
@fabpot

fabpot Jan 5, 2015

Author Member

fixed

}

/**
* @return ContextInterface|null

This comment has been minimized.

Copy link
@stof

stof Jan 20, 2015

Member

this cannot be null anymore

// http://img.example.com/me.png?v1
echo $packages->getUrl('/me.pdf', 'doc');
// /somewhere/deep/for/documents/me.pdf?v1

This comment has been minimized.

Copy link
@stof

stof Jan 20, 2015

Member

Should we put so much details in the readme ? It would duplicate the documentation (and will probably not be updated when the doc team improves the doc because they won't remember that this component has a verbose readme). I think most of this content should rather be submitted in a doc PR

@fabpot fabpot force-pushed the fabpot:asset-component branch from 48d3d15 to c8b799c Feb 10, 2015
@fabpot fabpot force-pushed the fabpot:asset-component branch from c8b799c to 0750d02 Feb 10, 2015
@fabpot fabpot merged commit 0750d02 into symfony:2.7 Feb 10, 2015
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/travis-ci The Travis CI build failed
Details
fabbot.io Doing some proofreading and checking your coding style.
Details
fabpot added a commit that referenced this pull request Feb 10, 2015
This PR was merged into the 2.7 branch.

Discussion
----------

[Asset] added the component

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #10973, #11748, #11876, #4883, #12474
| License       | MIT
| Doc PR        | not yet

TODO:

 - [ ] submit documentation PR

The current Asset sub-namespace in Templating has several (major) problems:

 * It does not cover all use cases (see #10973 and #4883 for some example)
 * It has some design issues (relies on the Request instance and so requires the request scope, very coupled with the PHP templating sub-system, see #11748 and #11876)

To decouple this feature and make it reusable in Silex for instance, and to fix the design issues and make it more extensible, I've decided to extract and rework the features provided into a new Asset component.

Basically, this component allows the developer to easily manage asset URLs: versioning, paths, and hosts.

Both the new and the old asset management features are kept in this PR to avoid breaking BC; the old system is of course deprecated and automatically converted to the new one.

Even if the features are quite similar, and besides the flexilibity of the new system, here are some differences:

 * `PathPackage` always prepend the path (even if the given path starts with `/`).
 * Usage is stricter (for instance, `PathPackage` requires a basePath to be passed and `UrlPackage` requires that at least on URL is passed).
 * In the configuration, named packages inherits from the version and version format of the default package by default.
 * It is not possible to override the version when asking for a URL (instead, you can define your own version strategy implementation -- the use cases explained in #6092 are easily implemented this way).
 * It's not possible to generate absolute URLs (see #13264 for a better alternative using composition; so using `absolute_url(asset_path('me.png')) should work)`.

#10973 was about adding shortcuts for bundles, which is a good idea; but given that you rarely reference built-in or third-party bundle assets and because we now have a one-bundle default approach named AppBundle, the same can be achieved with just a simple piece of configuration with the new assets feature:

```yml
framework:
    assets:
        packages:
            app:
                base_path: /bundles/app/
            img:
                base_path: /bundles/app/images/
```

Then:

```jinja
{{ asset('images/me.png', 'app') }}
# /bundles/app/images/me.png

{{ asset('me.png', 'img') }}
# /bundles/app/images/me.png
```

#12474 discussed the possibility to add a version for absolute URL. It's not possible to do that in a generic way as the version strategy involves both the version and the path, which obviously cannot work when the path is an absolute URL already. Instead, one should use the `asset_version` Twig function to add the version manually.

Commits
-------

0750d02 removed usage of the deprecated forms of asset() in the core framework
f74a1f2 renamed asset_path() to asset() and added a BC layer
4d0adea [Asset] added a NullContext class
d33c41d [Asset] added the component
@fabpot fabpot deleted the fabpot:asset-component branch Feb 12, 2015
@lsmith77

This comment has been minimized.

Copy link
Contributor

lsmith77 commented on f74a1f2 May 1, 2015

does the following imply an unwanted regression with this change? liip/LiipMonitorBundle#103

This comment has been minimized.

Copy link
Member

xabbuh replied May 1, 2015

@lsmith77 looks like what is described in #14368

@lsmith77

This comment has been minimized.

Copy link
Contributor

lsmith77 commented on f74a1f2 May 1, 2015

does the following imply an unwanted regression with this change? liip/LiipMonitorBundle#103

This comment has been minimized.

Copy link
Member

xabbuh replied May 1, 2015

@lsmith77 looks like what is described in #14368

@svassaux
Copy link

svassaux commented Oct 18, 2015

The following does not work anymore:

img src="{{ asset('images/user.png', 'images') | imagine_filter('default') }}" alt="Image de profil" class="img-circle whitebg"

see http://stackoverflow.com/questions/33200339/symfony-2-7-asset-component-not-working-with-imagine-filter

@jakzal
Copy link
Member

jakzal commented Oct 18, 2015

@svassaux if you think there's a bug, please create a new issue.

@12th
Copy link

12th commented Oct 25, 2015

If use absolute_url() for image from the CLI the url is without host, because masterRequest is empty.
Please tell me how get absolute url for image, for example in email?

@acasademont
Copy link
Contributor

acasademont commented Oct 25, 2015

@12th
Copy link

12th commented Oct 25, 2015

@acasademont
Yes, I saw this example, but absolute route work only for twig url() for tag , and when try use absolute for asset(), this not work.

/**
     * Send email to player
     *
     * @param Letter $letter
     * @return int
     */
    public function sendLetter(Letter $letter)
    {
        $context = [];
        $toEmail = '';
        $fromEmail = '';
        $result = null;

        $email = $letter->getEmail();

        switch ($email->getType()) {
            case Email::TYPE_INVITE:
                $mailer = $this->parameters['mailers'][$letter->getEmail()->getGateway()->getSlug()];

                $invite = $this->_inviteHelper->findOneBy(['id' => $letter->getEntityId()]);

                $context['token'] = $letter->getToken();
                $context['entity'] = $this->_emailHelper->getEntity($letter);

                $toEmail = $invite->getEmail();
                $fromEmail = [$mailer['send_from'] => $mailer['sender_name']];

                break;
            default:
                $mailer = $this->parameters['mailers']['default'];
        }

        // For CLI mailing, because request does not has http request
        $requestContext = $this->router->getContext();
        $requestContext
            ->setHost($mailer['site_http_host'])
            ->setScheme($mailer['site_http_schema'])
        ;

        // Change default settings for sending special
        $transport = \Swift_SmtpTransport::newInstance($mailer['host'], $mailer['port'], true)
            ->setUsername($mailer['username'])
            ->setPassword($mailer['password'])
            ->setEncryption($mailer['encryption'])
        ;

        $this->mailer = \Swift_Mailer::newInstance($transport);

        $this->twig->setLoader(new \Twig_Loader_Array(['letter' => $email->getBodyHtml()]));

        $context  = $this->twig->mergeGlobals($context);
        $htmlBody = $this->twig->render('letter', $context);

        // Catch incorrect email
        try {
            $message = \Swift_Message::newInstance()
                ->setSubject($email->getSubject())
                ->setFrom($fromEmail)
                ->setTo($toEmail);

            if ( ! empty($htmlBody)) {
                $message
                    ->setBody($htmlBody, 'text/html')
                    ->addPart($email->getBodyText(), 'text/plain')
                ;
            } else {
                $message->setBody($email->getBodyText());
            }

            $result = $this->mailer->send($message);

        } catch (\Exception $ex) {
            $letter->setStatus(Letter::STATUS_ERROR_SEND);
            $this->_emailHelper->updateLetter($letter);

            VarDumper::dump($ex->getMessage());

            return false;
        }

        $letter->setStatus(Letter::STATUS_SENT);

        if ( ! (bool)$result) {
            $letter->setStatus(Letter::STATUS_ERROR_SEND);
        }

        $this->_emailHelper->updateLetter($letter);

        return $result;
    }

Services

app_main.mailer:
      class: App\MainBundle\Model\Mailer
      arguments:
          - @mailer
          - @router
          - @twig
          - @swiftmailer.transport.real

Config:

swiftmailer:
    transport:  "%mailer_transport%"
    host:       "%mailer_host%"
    username:   "%mailer_user%"
    password:   "%mailer_password%"
    port:       "%mailer_port%"
    encryption: "%mailer_encryption%"
    auth_mode:  login
    spool:     { type: memory }

When try use flush the queue after send, catch error User Error: Call to undefined method getSpool

...
$result = $this->mailer->send($message);

// now manually flush the queue
$spool = $this->mailer->getTransport()->getSpool();
$spool->flushQueue($this->_esmtpTransport);

If dump getMasterRequest() in generateAbsoluteUrl(), the request is null, and I get path without host

/**
     * Returns the absolute URL for the given absolute or relative path.
     *
     * This method returns the path unchanged if no request is available.
     *
     * @param string $path The path
     *
     * @return string The absolute URL
     *
     * @see Request::getUriForPath()
     */
    public function generateAbsoluteUrl($path)
    {
        if (false !== strpos($path, '://') || '//' === substr($path, 0, 2)) {
            return $path;
        }

       VarDumper::dump($this->requestStack->getMasterRequest());die;

        if (!$request = $this->requestStack->getMasterRequest()) {
            return $path;
        }



        if (!$path || '/' !== $path[0]) {
            $prefix = $request->getPathInfo();
            $last = strlen($prefix) - 1;
            if ($last !== $pos = strrpos($prefix, '/')) {
                $prefix = substr($prefix, 0, $pos).'/';
            }

            return $request->getUriForPath($prefix.$path);
        }

        return $request->getSchemeAndHttpHost().$path;
    }
@acasademont
Copy link
Contributor

acasademont commented Oct 25, 2015

@12th
Copy link

12th commented Oct 25, 2015

And whether it is possible to dynamically change depending on the sender's e-mail settings?

@acasademont
Copy link
Contributor

acasademont commented Oct 25, 2015

mmm you would need to create a "package" for each of them and have different url's in there

http://symfony.com/doc/current/reference/configuration/framework.html#packages

@12th
Copy link

12th commented Oct 25, 2015

Alternatively I try, but it's not the best option because the sender to the individual letters is different and is selected from the panel

@12th
Copy link

12th commented Oct 25, 2015

If set host in package base_path

framework:
    assets:
        version_format: %%s?%%s
        packages:
            img_email:
                version: 15.09.22
                base_path: http://site.com/email/

template lokk like:

<body style="margin:0; padding: 0; background:#2e2b2a url({{ absolute_url(asset('main/bg.png', 'img_email'))  }}) top center repeat;">

the html look like

<body style=3D"margin:0; padding: 0; background:#2e2b2a url(/http://site.com/email/main/bg.png) top center repeat;">

with slash at the beginning of the address

@12th
Copy link

12th commented Oct 25, 2015

as a solution to use

 // fix absolute url to image, because absolute_url() not work from CLI 
 $htmlBody = str_replace('%site_http_host%', 'http://'.$mailer['site_http_host'], $htmlBody);
@12th
Copy link

12th commented Oct 25, 2015

Solution there #15448

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.