[RTM] Use Symfony asset component for Contao assets #1165

aschempp · 2017-10-31T09:42:33Z

I started using Symfony Encore for our application asset management and stumbled over the Symfony Asset service. I think there are a ton of useful feature for Contao.

It knows the path to an asset by asking for the composer package.
It automatically adds the TL_ASSETS_URL prefix if configured.
It automatically adds the package version to the URL, which results in clever cache busting on updates.

The provided code is totally working, but there are a lot more features we can implement. Missing TODOs:

Use asset URLs for TL_JAVASCRIPT and TL_CSS
Add support for asset URLs (absolute) to the Contao\Combiner
Re-implement Controller::setStaticUrls instead of using constants in the context service.
Maybe add Resources/public of all bundles as packages.
Maybe add a package for the backend theme
Unit Tests

Toflar · 2017-10-31T09:59:07Z

src/Asset/ContaoContext.php

+
+use Symfony\Component\Asset\Context\ContextInterface;
+
+class ContaoContext implements ContextInterface


Nothing should rely on the constants anymore. If you need to access TL_* it's better to find a different way because the constants will not be available in 5 anymore.

The only other way is to read from $GLOBALS in the back end and from global $objPage in the front end, which does not seem less wrong than using the constants to me.

Yeah, or we find a new way because apparently we will still need that feature in the future :-)

Added ToDo no. 3 for this. We should probably inject the PageModel and "duplicate" the code from Controller::setStaticUrls.

aschempp · 2017-11-01T21:12:22Z

I've added a dependency to https://github.com/terminal42/asset-bundle. This will automatically add all public bundle files as asset packages, completing the new Contao setup.

in Twig templates:

<img src="{{ asset('img/icon.png', 'rock_solid_slider') }}">
<img src="{{ asset('folder.gif', 'terminal42_folderpage') }}">

in Contao templates:

<img src="<?= $this->asset('img/icon.png', 'rock_solid_slider') ?>">
<img src="<?= $this->asset('folder.gif', 'terminal42_folderpage') ?>">

in Contao HTML module/element:

<img src="{{asset::img/icon.png::rock_solid_slider}}">
<img src="{{asset::folder.gif::terminal42_folderpage}}">

leofeyer · 2017-11-07T16:45:49Z

I think we should finish this PR with the current feature set. The only item from the todo list that needs to be done is "Unit Tests". The rest can be added later.

aschempp · 2017-11-08T21:40:08Z

@Toflar I have rewritten the contexts to use the page model, is that how you intended it?

leofeyer

Since there are only two contexts ("assets" and "files"), maybe we don't need the $field property. Why not have a ContaoAssetsContext and ContaoFilesContext?

leofeyer · 2017-11-11T10:10:59Z

src/Asset/ContaoContext.php

+     */
+    private function getFieldValue(): string
+    {
+        if (null === $this->page) {


if (null !== $this->page) { return (string) $this->page->{$this->field}; } /** @var Config $config */ $config = $this->framework->createInstance(Config::class); return (string) $config->get($this->field);

aschempp · 2017-11-14T13:39:48Z

Why not have a ContaoAssetsContext and ContaoFilesContext?

I don't think that would make anything better. It means we need three classes instead of one, including an abstract. Which will be exactly the same, a class that will be configured, just by extending instead of a constructor argument.

…text

leofeyer

Besides my two comments, I think there are a lot more places in the code where TL_ASSETS_URL is used. Are those not relevant?

leofeyer · 2017-11-15T10:26:41Z

src/Asset/ContaoContext.php

+    {
+        if (null === $page) {
+            /** @var Config $config */
+            $config = $this->framework->createInstance(Config::class);


Why are you using createInstance() instead of getAdapter() here? And shouldn't we initialize the framework first?

Both must be fixed indeed.

leofeyer · 2017-11-15T10:50:23Z

src/Resources/contao/library/Contao/Model.php

@@ -394,7 +394,7 @@ public function mergeRow(array $arrData)
 	 */
 	public function markModified($strKey)
 	{
-		if (!isset($this->arrModified[$strKey]))
+		if (!isset($this->arrModified[$strKey]) && isset($this->arrData[$strKey]))


Is this change related to the feature?

leofeyer · 2017-11-15T11:08:45Z

src/Resources/contao/library/Contao/Controller.php

@@ -1792,45 +1793,31 @@ public static function addEnclosuresToTemplate($objTemplate, $arrItem, $strKey='
 	/**
 	 * Set the static URL constants
 	 *
-	 * @param PageModel $objPage An optional page object
+	 * @deprecated Deprecated in Contao 4.5, to be removed in Contao 5. Use the asset contexts instead.


Since we have deprecated the method, we should no longer use it. Otherwise, if we still need to use it, we should not deprecate it. 😉

leofeyer · 2017-11-15T15:29:13Z

Thank you @aschempp.

fritzmg · 2017-12-12T15:39:34Z

In your examples you are using

<img src="<?= $this->asset('folder.gif', 'terminal42_folderpage') ?>">

however, shouldn't it be

<img src="<?= $this->asset('folder.gif', 'terminal42/contao-folderpage') ?>">

Or what exactly is $packageName?

Toflar · 2017-12-12T17:06:18Z

Yes it is, the example comment was before some more commits ;)

aschempp · 2017-12-12T17:33:39Z

@fritzmg terminal42_folder is correct. The bundle assets are based on the bundle classname, which in most cases and for correct setups should match the bundle alias for container configuration. We can't use the package name because we don't know which package a bundle belongs to (and a package theoretically can contain multiple bundles).

The diffeference is Contao componentes (assets), where the package name is the composer package (e.g. contao-components/jquery), because these do not have a bundle name.

leofeyer assigned aschempp Oct 31, 2017

leofeyer added the feature label Oct 31, 2017

Toflar reviewed Oct 31, 2017

View reviewed changes

leofeyer force-pushed the develop branch 14 times, most recently from 495cb11 to 1358688 Compare November 5, 2017 16:21

aschempp force-pushed the feature/assets branch 2 times, most recently from 91c4372 to 8f4dda4 Compare November 9, 2017 07:28

aschempp changed the title ~~[RFC] Use Symfony asset component for Contao assets~~ [RTM] Use Symfony asset component for Contao assets Nov 9, 2017

leofeyer requested changes Nov 11, 2017

View reviewed changes

leofeyer added this to the 4.5.0 milestone Nov 11, 2017

aschempp added 4 commits November 15, 2017 11:16

Add Contao components as asset packages

5a0b017

Added asset insert tag listener

51bf9fc

Use assets service for scripts in templates

031904d

Added dependency for terminal42/asset-bundle

4a3ca8e

aschempp added 5 commits November 15, 2017 11:16

Use page model in asset contexts and rewrite legacy method to use them

ac6f255

Move asset-bundle functionality to internal compiler pass

4499530

Correctly name the asset listener

7d84fc4

Added full unit tests coverage

69ef93d

Use the global page object instead of setting page model on asset con…

d6495f9

…text

leofeyer force-pushed the feature/assets branch from 17bab2e to d6495f9 Compare November 15, 2017 10:17

leofeyer requested changes Nov 15, 2017

View reviewed changes

leofeyer force-pushed the feature/assets branch from f7216b6 to cbbe3ad Compare November 15, 2017 13:20

Fix the coding style.

4bb7d55

leofeyer force-pushed the feature/assets branch from cbbe3ad to 4bb7d55 Compare November 15, 2017 14:03

leofeyer added 2 commits November 15, 2017 15:37

Fix some minor issues.

cca8353

Add a change log entry.

2dcb725

leofeyer merged commit eed0aea into contao:develop Nov 15, 2017

aschempp deleted the feature/assets branch November 15, 2017 16:20

fritzmg mentioned this pull request Dec 12, 2017

Some suggestions Sioweb/Contao4DummyBundle#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTM] Use Symfony asset component for Contao assets #1165

[RTM] Use Symfony asset component for Contao assets #1165

aschempp commented Oct 31, 2017 •

edited

Loading

Toflar Oct 31, 2017

leofeyer Oct 31, 2017

Toflar Oct 31, 2017

aschempp Oct 31, 2017

aschempp commented Nov 1, 2017

leofeyer commented Nov 7, 2017 •

edited

Loading

aschempp commented Nov 8, 2017

leofeyer left a comment

leofeyer Nov 11, 2017

aschempp commented Nov 14, 2017

leofeyer left a comment

leofeyer Nov 15, 2017

Toflar Nov 15, 2017

leofeyer Nov 15, 2017

leofeyer Nov 15, 2017

leofeyer commented Nov 15, 2017

fritzmg commented Dec 12, 2017

Toflar commented Dec 12, 2017

aschempp commented Dec 12, 2017


		use Symfony\Component\Asset\Context\ContextInterface;

		class ContaoContext implements ContextInterface

[RTM] Use Symfony asset component for Contao assets #1165

[RTM] Use Symfony asset component for Contao assets #1165

Conversation

aschempp commented Oct 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschempp commented Nov 1, 2017

leofeyer commented Nov 7, 2017 • edited Loading

aschempp commented Nov 8, 2017

leofeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aschempp commented Nov 14, 2017

leofeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leofeyer commented Nov 15, 2017

fritzmg commented Dec 12, 2017

Toflar commented Dec 12, 2017

aschempp commented Dec 12, 2017

aschempp commented Oct 31, 2017 •

edited

Loading

leofeyer commented Nov 7, 2017 •

edited

Loading