[#24858] - Revert "[4.0] Wrong order for attachActiveAssetsToDocument in MetasRenderer.php"
- Fixed in Code Base
- 23 Jun 2019
- Medium
- Build: 4.0-dev
- # 24858
- Diff
- Fedik:revert-24848-4.0-dev
- Success Hound No violations found. Woof! Details
- Success continuous-integration/appveyor/pr AppVeyor build succeeded Details
- Success continuous-integration/drone/pr Build is passing Details
- Pending JTracker/HumanTestResults Human Test Results: 1 Successful 0 Failed. Details
User tests: Successful: Unsuccessful:
Reverts #24848
That changes has more drawback than actually fixing (#24848 (comment))
Critical to me: the changes in #24848 not allow to alter $doc anymore, because at onBeforeCompileHead $doc has incomplete set of js/css/options.
To enable an asset there many way, they can be enabled in layout, in all events between route and render.
But alter $doc js/css/options possible only at onBeforeCompileHead.
Testing
Add to any system plugin an event:
function onBeforeCompileHead()
{
var_dump(array_keys(JFactory::getDocument()->_scripts));
}Expected:
You should see paths/to/core.min.js In the dump output.
Actual:
paths/to/core.min.js not exist in the output
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
@wilsonge Please decide which one is right.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24858.
such event already exist, called onWebAssetBeforeAttach https://github.com/joomla/joomla-cms/blob/4.0-dev/libraries/src/WebAsset/WebAssetManager.php#L343
it keeps the logical behavior of the onBeforeCompileHead
@joeforjoomla you forgot about a plugins that may want to alter the $doc,
and onBeforeCompileHead is the only way to do it, so that changes make a big problem.
It more break the logical behavior of the onBeforeCompileHead 
it keeps the logical behavior of the onBeforeCompileHead
@joeforjoomla you forgot about a plugins that may want to alter the $doc,
andonBeforeCompileHeadis the only way to do it, so that changes make a big problem.
So how have you did it till now? If you did it till now you will be able to do it even in the future because nothing changes.
And however if you really care about this, my suggestion was to add a new event onAfterCompileHead that would be the most logical way. So close this PR and make a new one in this sense.
to add a new event onAfterCompileHead that would be the most logical way
nope, unfortunately,
it may sounds logical, but it change the behavior of onBeforeCompileHead, that is very old event, that used for alter head data,
and if we introduce onAfterCompileHead then this will force everyone to rewrite their existing solutions,
while your problem ->enableAsset('jquery'); can be solved easily by moving to another event, or even to the layout. And it even not a BC, it did not exist before.
the behavior of
onBeforeCompileHead, that is very old event, that used for alter head data,
Nope, this event is not used to alter head data, but to add/remove assets before that the document head is compiled.
while your problem
->enableAsset('jquery');can be solved easily by moving to another event, or even to the layout. And it even not a BC, it did not exist before.
I have not a problem. The problem here is the most logical way that things are supposed to work.
this event is not used to alter head data
yeah sure, tell me then, what is used?
This event was used to alter head data in the past but wrongly and because there has never been a better way and a onAfterCompileHead. Altering head data here is a sort of 'hack'
the behavior of
onBeforeCompileHead, that is very old event, that used for alter head data,Nope, this event is not used to alter head data, but to add/remove assets before that the document head is compiled.
Actually, that is altering the <head> data
The problem isn't pull request ping pong or workflows. The problem is at an architectural level the web asset API is not compatible with the Document API, and that with the web asset API in place as a parallel and standalone thing you have two different places where assets can be registered to be "enabled" in the document (I still hate that terminology). Attaching the web asset data to the document before the compile event means you can't touch that API anymore, and you're stuck reverse engineering the resulting scripts and stylesheets arrays inside the document to find everything an asset includes if you need to do something with it (so here's hoping you don't have a "complex" asset in your chain). Attaching the web asset data after the compile event means something is firing off after plugins have had their last chance to alter the Document (and no, it doesn't really matter here that the asset API has its own event, that is an event specific to that API).
Engineering wise, you HAVE to get everything on a single path, SOMEHOW. From a core perspective, you really MUST consider the Document frozen after onBeforeCompileHead is fired. Otherwise, it's just going to be an endless circle of debate and explaining why everything is wrong.
and if we introduce onAfterCompileHead then this will force everyone to rewrite their existing solutions,
Bad idea. At that point you're dealing with string parsing, not data objects. Just use onAfterRender.
I think, at current state, onBeforeRender is pretty fine to use for enable assets. Then onBeforeCompileHead will continue working as before.
and if we introduce onAfterCompileHead then this will force everyone to rewrite their existing solutions,
Bad idea. At that point you're dealing with string parsing, not data objects. Just use onAfterRender.
that is what actually I tried to say :)
From a core perspective, you really MUST consider the Document frozen after onBeforeCompileHead is fired
Technically it become "frozen" itself (but silently), because any modification after that event will not affect the output.
And so you have to use onAfterRender for a terrible things
Yes the problem is at an architectural level.
By the way even relying on the onBeforeCompileHead to alter docs through plugins has always been wrong. It's subject to race condition of the order of execution of plugins, additionally Joomla has a addScript method but not a removeScript method.
Let's suppose that you have system plugin 1 and system plugin 2. The plugin 1 adds a script, the plugin 2 wants to remove that script from the document. This only works if the plugin 1 is executed before plugin 2. To have a similar behavior working in a reliable way we should split this process into 2 events, otherwise there is the bad solution... parsing string during the onAfterRender.
By the way even relying on the onBeforeCompileHead to alter docs through plugins has always been wrong. It's subject to race condition of the order of execution of plugins
It is not wrong,
the plugins can be sorted, and so the order of execution also.
You just made assumption that it only one way to add scripts, and have stick to it, but it is not only way.
the plugins can be sorted, and so the order of execution also.
Indeed it's wrong. You can't rely to plugins sorted. A user is free to sort and change the order of plugins in backend, so relying on this is crazy.
What you need to accomplish what you have in mind, would be something like this:
// Trigger the onBeforeCompileHead event
$app = Factory::getApplication();
$app->triggerEvent('onBeforeCompileHead');
// Attach Assets
$wa = $this->_doc->getWebAssetManager();
$wa->attachActiveAssetsToDocument($this->_doc);
// Trigger the onAlterDocumentBeforeCompileHead event
$app->triggerEvent('onAlterDocumentBeforeCompileHead');
onAlterDocumentBeforeCompileHead
this is BC for existing plugins, and will cost much to them, while moving your ->enableAsset('jquery'); to another place has a zero cost.
A user is free to sort and change the order of plugins in backend, so relying on this is crazy.
If order are important then it will be in plugin description/documentation.
The issues you're describing about plugin order of operations are an issue of any event dispatching system. It's why event listeners can't make assumptions about what order they are running in, and one of the umpteen thousand reasons why I have issues with how core manages its plugin integrations.
Hmm, do I understand right that if this PR here is not accepted, the change from #24848 is some kind of BC break? If so, then maybe https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4 needs to be updated?
@richard67 #24848 is not kind of BC break. Reverting with this PR would be kind of BC break
Ah, so in case if this PR here gets accepted, then it has to be mentioned in docs https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4, too, I see.
@richard67 if this is accepted, and let's hope that it is because #24848 is not only a B/C break but also a huge break in functionality, documentation does not need to be updated. WebAsset library is new in 4.0 so there was no B/C break in the first place. @joeforjoomla just needs to understand that he's using it incorrectly. This has already been explained by @Fedik, who is essentially the author of WebAsset library.
Well, it seems that i'm wasting my time trying to explain how an architecture should be supposed to work. By the way if someone should move something to the onWebAssetBeforeAttach that is new in 4.0 in order to avoid an exception and fatal error, yes this is a B/C break and must be documented in https://docs.joomla.org/Potential_backward_compatibility_issues_in_Joomla_4,
do I understand right that if this PR here is not accepted, the change from #24848 is some kind of BC break?
yes, because it will be not possible to get all scrits/styles at onBeforeCompileHead event
What about testing instructions? Code review?
Mainly code review.
But also can test like next:
Enable an asset in the index.php of the template:
$this->getWebAssetManager()->enableAsset('jquery');
and make a system plugin event:
function onBeforeCompileHead()
{
var_dump(JFactory::getDocument()->_scripts);
}In the dump you should see path/to/jquery.js. That will be correct behavior.
I have updated, description, added testing.
@wilsonge please have a look. I think it important issue.
You have merged that as "Fix b/c break when enqueuing assets" which fixing very minor B/C (it even hardly can call B/C, as it never exits before), but introduce pretty heavy B/C.
To fix that issue we can just change a recommendation, to use onBeforeRender as last point to add assets/css/js.
Who have used $doc->addScript() in onBeforeCompileHeadstill be able to do it without rewriting their solutions, which is a zero B\C.
I have tested this item
Added the suggested code to file "plugins/system/actionlogs/actionlogs.php".
Output on frontpage without this PR applied:
array(2) { [0]=> string(30) "/media/mod_menu/js/menu.min.js" [1]=> string(43) "/media/system/js/fields/passwordview.min.js" }
Output on frontpage with this PR applied:
array(8) { [0]=> string(28) "/media/system/js/core.min.js" [1]=> string(33) "/media/system/js/keepalive.min.js" [2]=> string(37) "/media/vendor/jquery/js/jquery.min.js" [3]=> string(41) "/media/legacy/js/jquery-noconflict.min.js" [4]=> string(50) "/media/vendor/bootstrap/js/bootstrap.bundle.min.js" [5]=> string(40) "/templates/cassiopeia/js/template.min.js" [6]=> string(30) "/media/mod_menu/js/menu.min.js" [7]=> string(43) "/media/system/js/fields/passwordview.min.js" }
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24858.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24858.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24858.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24858.
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2019-06-23 16:00:15 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
?
|
||
Seems like more people are in agreement than disagreement with this revertion so merging
Hmm, will this not result in a pull request ping pong? After this one here which reverts #24848 will be merged, @joeforjoomla makes a new PR to revert this one here and so restore #24848 , and then you make again a PR to revert that and so on and so on? And @wilsonge will merge them all? ;-)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/24858.