Adding CSS or JS files twice changes weight

Ran into this in #1458436: drupal_add_js/css() weight trickery fails if called for the same files multiple times, just found this after posting there, slightly different symptom but same cause.

Re-uploading tim.plunkett's patch but also reverting #505528: JS files of the same weight are not included in the order of the calls which by rights ought to no longer be necessary.

Sorry, didn't see this issue.
Opened a new one at #1670450: drupal_get_js() stable sort / drupal_add_js automatic weight.

The difference is, my issue has a patch that works :)
My patch is D7, but D8 is probably going to be the same.

EDIT:
Now also fixed for D8.

Ok to close as duplicate, as the other has a working solution?

I'd say yes, just link here from the other post.

#19, doing it.
#1670450: drupal_get_js() stable sort / drupal_add_js automatic weight

@tim.plunkett, can you comment on the other issue?

Fine with any solution.
I had already produced a lot of patches before I noticed the dupe.
But I'm ok to re-upload them. This is why I did ask in #18, and then did wait another month even after #19, before I did anything.

Or maybe I misread #19, in which case I apologize.

Point to duplicate

Here is the latest patch from #1670450: drupal_get_js() stable sort / drupal_add_js automatic weight, with test included.
I don't have time for testing atm, so in worst case I have to re-roll.

It looks like this patch blocks #1770902: Theme of parent site executing test leaks into all tests

First of all, I glanced over some of the patches in this issue, and it's a bit confusing that there are 3-4 entirely different patches in here, of which some have tests, and especially the tests are 1) testing different functionality, and 2) when they test the same stuff, they have different expectations.

That's a little bit weird, and based on that I've the suspicion that we might have merged one too many other issues into this one...? @tim.plunkett, you seem to be involved here since the beginning, so I wonder what your stance and interpretation is? :)

That aside, I reviewed the latest patch:

- Overall, the code looks good from a style perspective.

- However, I wonder whether it is actually safe to "just remove/wipe" the original/previous items from the stack. A previous call to drupal_add_*() might have specified additional $options, which may not be specified in a later call, so the originally added $options will get lost upon the second call. So shouldn't we actually merge the previous item with the new data?

- Second, since an item is removed and re-added to the stack on a subsequent call, this has the consequence that a later call from e.g. within another module or a theme will revert the item back to its original location. What I mean is this:

Node:      drupal_add_css('core/modules/node/node.css');
SuperNode: drupal_add_css('modules/supernode/node.css'); # intends to override
Innocent:  drupal_add_css('core/modules/node/node.css'); # just wants node.css; isn't aware of SuperNode module

The current behavior of Drupal core is that supernode's override of node.css will be loaded. With the latest patch, however, Node module's original node.css will be loaded.

While that makes sense from a procedural standpoint, I wonder whether the change of behavior and expectations might cause some havoc in contrib and custom modules, which may not always be fully aware of the exact order in which they are invoked. That especially becomes problematic if you think about non-core JS/CSS files; i.e., when contrib module A attempts to override contrib module B's file and contrib module C isn't aware of module A and thus just loads module B's file. This worries me.

- Lastly, I think the test assertions could be hardened a bit. There also doesn't seem to be any coverage yet for the original issue of adding the identical file twice (with and without another file in between) and ensuring that its weight does not change. We need to add some assertions that verify the raw $items stack in drupal_add_*() there, since most of the actual expectations cannot really be verified by looking at the rendered output only.

What do you think?

@sun,
the issue linked in #23 (which is now closed as duplicate) has more explanations on the patches in #24.

In short:
- The first patch in #24 introduces tests, that are meant to fail with the current implementation. Probably those tests will also fail with previous patches in this issue.
- The second patch introduces the same tests as the previous one, and the solution.

- However, I wonder whether it is actually safe to "just remove/wipe" the original/previous items from the stack. A previous call to drupal_add_*() might have specified additional $options, which may not be specified in a later call, so the originally added $options will get lost upon the second call. So shouldn't we actually merge the previous item with the new data?

I think we should first make the existing behavior predicable, and make it behave as it was designed. This is what the patch in #24 does.

Your suggestion goes beyond that, and does actually change the expectation on how the system should behave. I agree with the intention, but imo we should first get the bugfix in.

So, here is the suggested plan of action:

1. Get the patch #23/#24 in. (if you want additional tests, fine) This is the "bug fix" aspect.
2. Do the D7 backport.
3. Open a new issue to discuss changes in the expected behavior (merge etc). This will be a feature request.

- Second, since an item is removed and re-added to the stack on a subsequent call, this has the consequence that a later call from e.g. within another module or a theme will revert the item back to its original location.

Let supernode set a weight on drupal_add_css() -> done.
We might find a smarter solution, but for the bugfix it is ok.

Sorry for not clarifying, but my comments were actually about the patch in #24, which contains the following changes, and those have the consequences I described. So if we do not want to change the behavior (and thus expectations) with this fix, then this change doesn't work:

+++ b/core/includes/common.inc
@@ -2678,6 +2675,8 @@ function drupal_add_css($data = NULL, $options = NULL) {
+        unset($css[$data]);
         $css[$data] = $options;

@@ -3833,6 +3811,8 @@ function drupal_add_js($data = NULL, $options = NULL) {
+        unset($javascript[$options['data']]);
         $javascript[$options['data']] = $options;

Thanks for clarifying.
But actually this does not change anything except the order.

The override happens in this line.

$css[$options['data']] = $options;

no matter if we unset before. The unset() only changes the order.
So, it is exactly what we had before.
(or did i miss something?)

-------------------

This said: Rethinking my comment in #26 bottom: The weight is not a solution to prevent the override. It never was. The override happens no matter of the weight, and this is how it was all the time.
http://api.drupal.org/api/drupal/core%21includes%21common.inc/function/d...

Yes, that's essentially in line with what I was saying -- the unset()s in this patch have consequences that cause a change in behavior of how CSS/JS files are being overridden.

AFAICS, the unset()s have been added here to eliminate the weight-futzing. That is, because an item without a 'weight' is compared to other items, and if the other items do not have a 'weight' either, then the items are essentially left in the order in which they appear in the array.

However, the last patch in #10 basically achieved the same without unsetting any array keys... (I guess it's clear by now that I don't feel comfortable with the unset() approach due to the outlined consequences ;)) I think we should try to replace the unset()s with the approach taken in there.

What do you think?

Nope.
The unset() in this patch does not have the consequences that you outlined.
The line after the unset has those consequences. And this is there before the patch.

Issue #1670450: drupal_get_js() stable sort / drupal_add_js automatic weight, where the last patches are taken from, explains why the micro-weight approach fails, and why stable sorting and adding to the end of the array is a good solution.

If you don't believe it, try to write a test that proves the patch to be a regression :)

One essential factor of confusion is that #1770902: Theme of parent site executing test leaks into all tests revealed that the test that is being responsible for testing this functionality is currently hiding that this very test actually fails in HEAD.

This puts us into an very unfortunate situation — the tests are verifying expectations and they pass, but the actual behavior of Drupal is different, which inherently means that code in contributed and custom modules may rely on the actual behavior. This doesn't really matter for D8, since we can change the behavior, but the identical fix most probably cannot be backported to D7.

On another look, I know see and understand what you mean with regard to my first point:

+        unset($css[$data]);
         $css[$data] = $options;

The rationale being: If a previous call passed $options and a later call does not, then the $options of the previous call are lost, since the effective item is replaced with the new one. Although I'd actually consider that to be a bug, that's "good enough" for this issue, since it means we don't introduce a regression.

My second point doesn't seem to be addressed though. However, as mentioned before, we can change the behavior for D8 and require modules + themes that depend on the current behavior to adjust themselves accordingly.

Aside from that, you're right in that this functionality should be tested more correctly (and I actually stated the same in #25 ;)).

Thus, I've added some tests to assert the original problem of this issue, generally cleaned up the code, and merged in the testing framework fix, so we're sure that it doesn't fool us.

That said, I think I successfully managed to prove you wrong ;)

The test failure that this patch will show is caused by the unset(), which destroys the original position of an element in the items. This actually resembles my second point in a much more simple way.

I made sure to add an inline comment to the test that explains the expectation, including the very trivial reason for why the expectation exists. :)

That said - debugging this test some more here made me understand how to fix #1770902: Theme of parent site executing test leaks into all tests without requiring this patch.

Therefore, attached patch reverts the additional merge and only leaves the improved test coverage.

@sun,
if you actually want to prove a regression, you need to post two patches:
- one with only the test, which passes
- one with the test plus the patch to common.inc from above, which fails.

So, if I understand correctly, your presumed "expected behavior" is
Items with the same weight should be sorted in the order of their first inclusion.
(a, b, a -> a, b)
while my assumption was
Items with the same weight / other sort criteria should be sorted in the order of their last inclusion.
(a, b, a -> b, a)

I am not sure if any of these assumptions will hold in the current HEAD, or which one is closer to current reality.
I would assume that the micro-weight trick breaks the first assumption.

------------

My own thoughts that led to the patch were that modules which depend on the current behavior depend on something unpredictable and arbitrary, so relying on that is already a bug.

Unfortunately it will still be awkward even after the patch (no matter if we sort by order of first insertion or order of last insertion).

E.g. module X includes a.css and b.css, and expects them to be included in exactly this order.
Then module Y (which module X does not know about) includes only a.css.
Now it depends on which module fires first, to determine the order of css files included. No matter which sorting strategy we use. Bad :(
(note: module weight only helps if each drupal_add_css() is fired from the same hook. which is not necessarily the case)

Conclusion: Insertion order is a fragile thing.
Still I think we should attempt something more predictable than the current situation.

module X includes a.css and b.css, and expects them to be included in exactly this order.

Yes, this is exactly what is happening: see system_init()

That's an example only. The fundamental expectation and pattern is repeated in many different forms. If any extension (module/theme) overrides the asset of another extension, then the asset has to be loaded at the exact identical position as it would have appeared originally. Otherwise, CSS styles will no longer apply (because more specific styles are loaded earlier) and JavaScript will simply break (because other scripts may depend on the overridden script's original position).

Thus, the unset() is bogus. We need to make this patch work without it.

@sun,
Short story: I don't mind removing the unset(), if we agree this is the desired behavior. BUT...

Long story:
What you describe in #36 is: Module X includes a.css and b.css, and no other module may flip the order of the two css files.
I agree that this would be a desirable behavior.

However:
1) The current system generally does the opposite. This is why the patch says unset().
2) There is no way with our current toolset (weight and/or stable sort) to honestly guarantee this behavior. Removing the unset() just replaces one variation of the problem with another.

-----------

1) The current system generally does the opposite.

Example:
module X:
drupal_add_css(a.css) -> micro-weight of 0.000
drupal_add_css(b.css) -> micro-weight of 0.001
module Y:
drupal_add_css(y.css) -> micro-weight of 0.002
drupal_add_css(a.css) -> micro-weight of 0.003

Resulting order:
b.css, y.css, a.css

So, the current system generally does the opposite of what you describe.
When I created the patch, I simply tried to reproduce this, but make it more consistent.

2) By removing the unset(), we just have the same issue in another flavor.

Example:
Module X:
drupal_add_css(b.css)
Module Y:
drupal_add_css(y.css)
drupal_add_css(a.css)
drupal_add_css(b.css)
drupal_add_css(c.css)
Module Z:
drupal_add_css(z.css)
drupal_add_css(b.css)

Desired order, as module Y would like it, and as you describe in #36:
y, a, b, c, z

Order of drupal_add_css():
b, y, a, b, c, z, b
(drupal_add_css does not know which module is calling)

Current behavior resulting order:
y.css (weight 0.001)
a.css (weight 0.002)
c.css (weight 0.003)
z.css (weight 0.004)
b.css (weight 0.005)

Stable sort resulting order, without the unset():
b, y, a, c, z
(desired order is messed up by module X, which includes b.css before module Y does it)

Stable sort resulting order, with the unset():
y, a, c, z, b
(desired order is messed up by module Z, which includes b.css after module Y does it)

My predication: Your desired behavior cannot be implemented with our current toolset.
Why? Because drupal_add_css() does not know whether the css was included from the same module or a different one.
And, we don't know whether the "evil" module's drupal_add_css() happens before or after.

-------------

Btw, I added the x.css and y.css in the example, to guarantee that all files have a different micro-weight, so we don't have to discuss undefined sort order due to unstable sort functions.

The examples above can easily be turned into a test, if we agree on a desired output that is technically possible.

@tim.plunkett:
The discussion was about the intended order of stylesheets, if they are included multiple times.
Basically, sun has the idea they should be ordered by each stylesheet's first inclusion.
Whereas I said they should be ordered by each stylesheet's last inclusion.

sun's argument being that his suggested order is more reasonable.
My argument being that my suggested order is closer to the current behavior.

I think none of these is perfect, there are still examples to prove each of them wrong.
But definitely better than the current unpredictable behavior, and maybe the best we can get.

If you post your own patch, you probably should declare which direction you find more reasonable.

One solution is to check if the css has already been added, and if so, reuse the weight from the original entry. If you want to change the weight, then it would be required to use the hook_css_alter() method.

In any case, documentation for the drupal_add_css/js should be updated to reflect the behaviour of the code (ie. mention that adding the same css/js twice can lead to indeterminate order).

Looks like this has changed to AssetResolver::sort().

The issue with the previous patches seems to be issues with strings needing to be cast to a float; see #2426731: Dragging fields in Manage Display doesn't work anymore.

I was bitten by this too: since I need to fix this quickly and the patches above look quite complex to test thoroughly, I put together a quick fix. Temporarily moving to D7 to see what the bot thinks of this solution, I will move this back to D8 ASAP. The patches above are still the official ones to review.

Sorry for the noise.

I created a test issue for this at #2447361: Test issue for Adding CSS or JS files twice changes weight. Sorry again for the noise.

Btw, this looks major-ish to me: things broke badly on the project I'm working on due to it.

I added the "to be discussed" from #39 into the issue summary.
To mention it again:

The two options are:

1. Stylesheets ordered by each stylesheet's first inclusion.
   (could be considered more reasonable, to be discussed)
2. Stylesheets ordered by each stylesheet's last inclusion.
   (closer to current behavior)

I've been looking into this and it seems like we want the way core currently works if we're doing an override (Stylesheets ordered by each stylesheet's last inclusion); otherwise we want it to be ordered by each stylesheet's first inclusion (patch in here is trying to solve). This means we need the original weight and the order in which the files where added.

The test that's failing is running this in short.

  drupal_static_reset('drupal_add_css');
  $system = drupal_get_path('module', 'system');
  $simpletest = drupal_get_path('module', 'simpletest');

  drupal_add_css($system . '/system.base.css');
  drupal_add_css($simpletest . '/tests/system.base.css');
  drupal_add_css($simpletest . '/tests/system.base.css');
  drupal_add_css($system . '/system.base.css');

  $scripts = drupal_get_css();

  // Make sure that the simpletest css was overwritten by the system css.
  var_dump(strpos($styles, $system . '/system.base.css'));
  // If false, it failed.

we want the way core currently works if we're doing an override (Stylesheets ordered by each stylesheet's last inclusion); otherwise we want it to be ordered by each stylesheet's first inclusion

By override, do you mean to add a stylesheet with same name but different path? Whereas "otherwise" is same name same path?

I want to point out that both of this can happen at once, and your spec seems to be ambiguous for this case. Maybe you can clarify?

@donquixote

Does this clear it up?

drupal_static_reset('drupal_add_css');
$system = drupal_get_path('module', 'system');

drupal_add_css($system . '/system.base.css'); // Keep this.
drupal_add_css($system . '/system.base.css');
drupal_add_css($system . '/system.base.css');
drupal_add_css($system . '/system.base.css');
drupal_add_css($system . '/system.base.css'); // Core will currently use this weight.

drupal_static_reset('drupal_add_css');
$system = drupal_get_path('module', 'system');
$simpletest = drupal_get_path('module', 'simpletest');

drupal_add_css($system . '/system.base.css');
drupal_add_css($simpletest . '/tests/system.base.css');
drupal_add_css($simpletest . '/tests/system.base.css'); // Patch in here will use this file.
drupal_add_css($system . '/system.base.css'); // Use this as an override it has the same basename. This is the test that's failing.

So it will not be possible to un-override or over-override? Only the first override works?

Overall the distinction of override vs otherwise seems arbitrary and inconsistent to me.

drupal_static_reset('drupal_add_css');
$system = drupal_get_path('module', 'system');
$simpletest = drupal_get_path('module', 'simpletest');

drupal_add_css($system . '/system.base.css');
drupal_add_css($simpletest . '/tests/system.base.css');
drupal_add_css($other_stylesheet);
drupal_add_css($simpletest . '/tests/system.base.css'); // Patch in here will use this file.
drupal_add_css($system . '/system.base.css'); // Use this as an override it has the same basename. This is the test that's failing.

I squeezed in the $other_stylesheet so that weight becomes relevant. I assume that system.base.css will get a weight to put it *after* $other_stylesheet - just to be sure i understand correctly.

With my current implementation the weight of the first one will be used

drupal_static_reset('drupal_add_css');
$GLOBALS['conf']['preprocess_css'] = FALSE;
$system = drupal_get_path('module', 'system');
$simpletest = drupal_get_path('module', 'simpletest');

drupal_add_css($system . '/system.base.css'); // Use the weight of this.
drupal_add_css($simpletest . '/tests/system.base.css');
drupal_add_css('misc/print.css');
drupal_add_css($simpletest . '/tests/system.base.css'); // Old patch will use this.
drupal_add_css($system . '/system.base.css'); // Use this as an override it has the same basename. This is the test that's failing. This patch will do this.

Moving to 7.x for testing.

Fixed the 2 fails from #38 and added 2 different ones in #51. Lame.

This is no longer an issue in 8.x - we still increment the weight by a tiny amount, but we never add the same library/asset twice - because we first get a list of libraries, not raw files, which is always unique and doesn't contain already loaded libraries.

Moving back to 7.x.

Just seen a js aggregate variable item with 444 aggregates in it. About 78 I accounted for due to overlay module being enabled (overlay's two js files get aggregated, which means duplicate aggregates for both child and parent pages if both are viewed inside/outside overlay). A lot of the rest looked like the result of this issue, such as form.js appearing in 218 aggregates with different weights each time (based on a sample).

I've also seen aggregation take sites down on deployments recently (again) per #886488: [D7] Add stampede protection for css and js aggregation and #1014086: Stampedes and cold cache performance issues with css/js aggregation - those were some of the first production Drupal 7 issues I worked on...

This issue results in both more aggregates to build, and more variable_set() calls (with a longer list) to store them, which won't help the stampeding at all.

Given that, going to bump this to critical.

While a lot of sites don't know they're hitting this issue (because they're not manually inspecting their js/css aggregation variables), it's probably responsible for more downtime on deployments and general sluggishness than any other Drupal 7 core issue at the moment.

https://www.drupal.org/project/advagg will "normalize" the weight by storing the order as an int in advagg_aggregates table; ignoring everything else like group, every page, and weight. These factors help decide the file groupings (bundles) but because it uses "image derivative style generation" with the filename as the lookup AdvAgg doesn't need to store that; it just needs to know the order in which file_get_contents() should be called. AdvAgg throws away a lot of the keys found in drupal_add_js when storing data in the DB; it's not needed to build the aggregate.

It's been over a year since I've done a deep dive into how core's aggregates work but it seems like the key is built off of the data key. The heart of the issue you seem to be hinting at but not directly saying is that the ordering of the files changes. Ignoring all other aspects, somehow the order is ABCD on one run and ABDC on another run. I've noticed this as well, it's what preventing optimal aggregates in the bundler in AdvAgg. I've created an issue that sorta addresses the issue #2500791: Have an admin section of the bundler offer ordering analysis/advice; build out the core dependencies and create a reorder function/algorithm . AdvAgg is consistent (see advagg_drupal_sort_css_js_stable()) but this is still an issue; it's deeper than just adding files twice.

The heart of the issue you seem to be hinting at but not directly saying is that the ordering of the files changes.

It's not even that as good as that.

Let's say you an aggregate with a single file. On one page the file is added once, on another page twice.

Slimmed down

  // Page 1.
  $file1 => ['weight' => 100.01];
   // Page 2.
  $file1 => ['weight' => 100.02];

Since the hash is based on the data array, that would result in two different aggregate hashes (although not files because the contents are identical in this case). Just the duplicate hash is enough to trigger a variable_set() though which is what tends to cause the downtime.

https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_bu...

function drupal_build_js_cache($files) {
  $contents = '';
  $uri = '';
  $map = variable_get('drupal_js_cache_files', array());
  // Create a new array so that only the file names are used to create the hash.
  // This prevents new aggregates from being created unnecessarily.
  $js_data = array();
  foreach ($files as $file) {
    $js_data[] = $file['data'];
  }
  $key = hash('sha256', serialize($js_data));
  if (isset($map[$key])) {
    $uri = $map[$key];
  }

  if (empty($uri) || !file_exists($uri)) {
...

https://api.drupal.org/api/drupal/includes!common.inc/function/drupal_bu...

function drupal_build_css_cache($css) {
  $data = '';
  $uri = '';
  $map = variable_get('drupal_css_cache_files', array());
  // Create a new array so that only the file names are used to create the hash.
  // This prevents new aggregates from being created unnecessarily.
  $css_data = array();
  foreach ($css as $css_file) {
    $css_data[] = $css_file['data'];
  }
  $key = hash('sha256', serialize($css_data));
  if (isset($map[$key])) {
    $uri = $map[$key];
  }

  if (empty($uri) || !file_exists($uri)) {
...

I don't see where weight gets used when generating the hash. The data key is the filename in both of these cases correct?

Doh you're right, misread what was happening. Back to major then...