I don't think onBeforeCompileHead should be used to add assets using Document API. This should be done earlier.

@joeforjoomla please add "[4.0]" at begin of Title.

I don't think onBeforeCompileHead should be used to add assets using Document API. This should be done earlier.

Well, the onBeforeCompileHead event has always been the most right place to add scripts and stylesheets to the document. It's the event done to add assets before than the head is compiled into the page. Preventing this now would be a sensational contradiction.

@joeforjoomla please add "[4.0]" at begin of Title.

Sorry @franz-wohlkoenig too late tonight.. forgot the "[4.0]"

Well, the onBeforeCompileHead event has always been the most right place to add scripts and stylesheets to the document.

It wasn't. It's meant to work directly with elements in the head. With this PR it's completely broken. Try this plugin for test:

public function onBeforeCompileHead()
{
	$this->app->getDocument()->_styleSheets = [];
}

Before PR this removes all stylesheets. After PR this is ignored and stylesheets managed by WebAssetManager are added anyways.

The onBeforeCompileHead is supposed to be used to add stylesheets or any other kind of assets to the head, not to remove all stylesheets after that they are added to the document. This is something that makes no sense.

onBeforeCompileHead is meant for altering head compilation. Assets added using Document API should be added earlier. onAfterDispatch or onBeforeRender is fine for this.

onBeforeCompileHead is meant for altering head compilation. Assets added using Document API should be added earlier. onAfterDispatch or onBeforeRender is fine for this.

Nope. Absolutely not. This is fully contradictory.

Now you can do this:

public function onBeforeCompileHead()
{
	$document->addScript('jquery.js');
}

but you can't do this:

public function onBeforeCompileHead()
{
	$document->getWebAssetManager()->enableAsset('jquery');
}

If you really need to do such a thing:

$this->app->getDocument()->_styleSheets = [];

it would be better to add a new event such as onAfterCompileHead

Pinging @wilsonge and @mbabker

The onBeforeCompileHead event is the last hook point to allow developers to alter the document before HTML starts getting spit out (ETA: as it relates to assets and page metadata, i.e. SEO purposes), once this event is run realistically the document's assets should be considered to be in a frozen state. So this PR is correct.

(And yes, using the onBeforeCompileHead event to add or remove assets is perfectly acceptable and suggested practice for a long time)

Thanks @mbabker for the clarification to everyone reading this PR

it would be better to add a new event such as onAfterCompileHead

No. This doesn't work at all.

The onBeforeCompileHead event is the last hook point to allow developers to alter the document before HTML starts getting spit out

That's why WebAssetManager should run before this event, not after.

This is why everything should be in one API and not several. You can't have 2 different asset managing APIs that are incompatible, you're going to run into a deep rooted logic problem sooner or later.

Either way, before or after this PR is not the most optimal of situations. Either the asset manager gets attached to the document then the current event runs, which leaves the asset manager in a frozen state and you as the developer stuck trying to reverse engineer what it did to the document, or you leave the order of operations as they are now and fire an event when the manager gets attached to the document which breaks the expectation that onBeforeCompileHead is the place to "fix" things (as essentially the asset manager can "undo" changes you made in the event).

Personally, I would engineer the system so that once onBeforeCompileHead is fired the Document is considered to be in a read-only state. Because of the magic internals of the Document API, the head renderer (or meta) is usually the last renderer to be executed (and I think Dimitris hacked even more magic in to make sure that if you're using the split renderers that the script renderer runs last if you've got it at the end of your body otherwise it would render first and you wouldn't be able to add scripts from modules). How you get the system to that point is up to you all, but that's how I'd do it and I wouldn't be trying to fight with two incompatible APIs in the process.

Personally, I would engineer the system so that once onBeforeCompileHead is fired the Document is considered to be in a read-only state.

I agree. It doesn't make sense to make the document read-only state BEFORE the onBeforeCompileHead is fired. This is the main event supposed to add/remove assets to the document.

Once you revert this logic based on this PR and keep things working as they have always worked till now, you are free to engineer the system better. @SharkyKZ sorry but you are you are thinking in a non-logical way

I'm going to merge this for now as it's clearly more b/c with what we had before (and I don't think altering that was intended - the idea was to better link assets together). However I think there's definitely merit's to @mbabker 's comments and if someone wants to propose changes in the future in a dedicated PR I'm ok with that

I understood the issue, but
it was not wrong, it had a purpose to BC and to allow to hack a head scripts/styles/options that in $doc->_scripts etc. A plugins may use onBeforeCompileHead for merge scripts/styles, for optimization etc (I have such plugin). After this patch it become harder, much more harder, because now it need to parse HTML to get a list of attached scripts/styles, and to replace them.

Currrently it's no more possible to add assets during the onBeforeCompileHead

For such purpose there was an event onWebAssetBeforeAttach, that happen before onBeforeCompileHead. so at onBeforeCompileHead $doc has complete js/options/css set.

Another drawback of this patch, is that if Asset add a script options with WebAssetAttachBehaviorInterface::onAttachCallback then it become impossible to override that options.

A 2 possible solutions:

1. Revert this changes
2. Introduce another event, that should provide final $doc state (with full set of js/css/options) but before actually rendering.

upd. It more easy to revert, there already many events that allow to add/enable asset/script