joomla / joomla-cms

Home of the Joomla! Content Management System
https://www.joomla.org
GNU General Public License v2.0
4.79k stars 3.65k forks source link

[4.0] Asset Manager doesn't support template overrides #27024

Closed C-Lodder closed 5 years ago

C-Lodder commented 5 years ago

Steps to reproduce the issue

Create a new CSS file here:

administrator/templates/atum/css/vendor/choicesjs/choices.css

Inside this CSS file, add:

html { display: none }

Then go to your Article Manager

Expected result

The page should be blank

Actual result

The CSS file is not overridden

This works perfectly fine with JHtml::stylesheet as it checks if the CSS file exists at template level first.

C-Lodder commented 5 years ago

@Fedik https://github.com/joomla/joomla-cms/pull/25775 doesn't solve this either

Fedik commented 5 years ago

That not a bug, that is a feature ;)

Well, it was planed like that. So /media/vendor should be overridden only via AssetManager check comment https://github.com/joomla/joomla-cms/pull/26077#issuecomment-544872087

But that not hard to change, if it need.

C-Lodder commented 5 years ago

@Fedik Which directory do we override the .json file?

Fedik commented 5 years ago

Which directory do we override the .json file?

no no, it not like that, no need override "file", but definition of asset.

Take a look https://docs.joomla.org/J4.x:Web_Assets

json just a way to register a bunch of assets in the registry.

Example the asset of /media/vendor defined in media/vendor/joomla.asset.json. To override one of its(vendor) assets on template level you can define your asset with the same name in your template joomla.asset.json. Because the template json will be loaded last, it will override previous assets (from vendor json) by its name, if there the same name. Or programaticaly $AssetRegistry->add(new WebAssetItem('foobar', ['js' => ['new/path/to/file.js']]));

Fedik commented 5 years ago

@wilsonge what do you think about the issue?

The main idea was that all scripts/styles that have defined Assets should be loaded and overridden through AssetManager. Since it is a new thing and /media/vendor also "new" folder in j4, I thought it will be safe. But as we see people can still use HTMLHelper to add the files from /media/vendor that can lead to duplication when the file overridden on template via HTMLHelper.

From other side, if we allow override js/css files in /media/vendor by AssetManager itself (by HTMLHelper while attaching), it will produce the same mistake, example when someone override asset via AssetManager, and another one use HTMLHelper to load old file.

That kind of this issue #25309

We can make some kind of requirement (or statement) that all assets should be managed through AssetManager only. But we cannot prevent to use HTMLHelper directly, because that also should work for compatibility.

C-Lodder commented 5 years ago

@Fedik Using the following works:

{
      "package": "choices.js",
      "name": "choicesjs",
      "type": "style",
      "uri": "administrator/templates/xxxxxx/css/vendor/choicesjs/choices.min.css"
 },
{
      "package": "choices.js",
      "name": "choicesjs",
      "type": "preset",
      "dependencies": [
        "choicesjs#style",
        "choicesjs#script"
      ]
 }
Fedik commented 5 years ago

@C-Lodder the uri can be simpler vendor/choicesjs/choices.min.css :wink:

A bit more explanation. WebAsset use HTMLHelper in next way:

C-Lodder commented 5 years ago

ah nice, thanks :)