AssetResolver::getJsAssets cache id doesn't consider attached settings, leads to ajax issues

Needs tests etc but setting NR to get eyes on it

Good find.

First of all I was going to say we should hash the settings into the cache key, but the settings logic isn't cached. I've pushed a second branch which attempts to do zero libraries work if there are no libraries, but also doesn't cache anything. Didn't run tests on that yet, approach could be completely flawed.

First of all I was going to say we should hash the settings into the cache key, but the settings logic isn't cached.

I don't think we can hash the settings, they contain ajaxTrustedUrl which has keys like form_action_p_pvdeGsVG5zNF_XLGPTvYSKCf43t8qZYSwcfZl2uzM and ajax which has keys like edit-actions-submit--mclJzp_PNkM. It seems at least the edit-actions-submit suffix changes on each request.

Some of the settings logic is cached, and the rest of it is not. I will admit I don't really understand what's going on in this function.

I've pushed a second branch which attempts to do zero libraries work if there are no libraries, but also doesn't cache anything. Didn't run tests on that yet, approach could be completely flawed.

Seem to be a few test fails related to ajax, not sure if they are random fails and I don't see a retry button.

The whole function could use a refactor, but that's probably blocked on #2614936: Improve unit test coverage for AssetResolver → .

catch → closed merge request !10592

catch → reopened merge request !10592

OK I made some more progress. I think this was probably broken when we added caching in the first place in 2015, which I worked on...

The main problem is that the decision whether to add settings depends on whether any library that is already loaded or about to be loaded adds the drupalSettings library as a dependency. If it doesn't, we want to ignore all the settings added, because there's nothing to process them. We do it like this because various settings get added 'just in case' even if nothing will use them.

However the already loaded libraries are not included in the cache key (and shouldn't be) so that logic is flawed.

What I've done is move that logic out of the cached section entirely, but tried to keep the same overall conditions. I made various mistakes and these resulted in test failures, especially in Ajax\FrameworkTest, so we do have pretty good coverage to show that this isn't breaking some things that already work, but obviously no coverage of the bug itself yet.

AssetResolver does have some unit test coverage in AssetResolverTest, so maybe we can add some coverage for this issue without having to do all of #2614936: Improve unit test coverage for AssetResolver → . Or maybe we can extend FrameworkTest to account for this case too. Or both.

This looks obvious now reading the diff I ended up with, it was not obvious before actually making the change exactly what logic can be cached and what can't. I'm pretty sure we actually cache what can be cached and don't cache what can't now. A bonus is this should result in better cache hit rates too.

Test run is green except for one random failure.

There are some lines moved around here that don't need to be with the final result, might move them back where they were to reduce the diff.

This looks great. I stepped through this with the problematic webform mentioned in the IS. At first I thought it was still problematic in that it was still getting and setting the cache when $libraries_to_load was empty, but then I was relieved to see that $asset_settings was still merged in regardless of what happened with the cache.

It might be possible to skip the cache get/set still, I had to give up trying to skip that to get tests green, but now they're green we could try again.

The main barrier is the $settings_in_header variable which is set based on whether any libraries requested are in the header and also depend on settings, but if no libraries are requested, that is FALSE. So potentially accounting for that, and moving some variable creation back around, might be enough.

As per slack it's probably not necessary to skip caching when $libraries_to_load is empty, as there is still a fair bit of logic that can be cached, such as hook invocations and asset optimisation.

Before the latest version of the MR I made an attempt to skip the entire hunk if libraries were empty, but it failed and it's reasonable for modules to alter things when the list is empty etc., so yeah I think current state here is as far as we should go.

I don't have any good ideas to test this yet - we could probably write something that tests the implementation, but we could have done that before the change here and the test would have been wrong.