Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Not only for CSS, but if the patch from #865536: drupal_add_js() is missing the 'browsers' option is applied, then JS browser conditional settings get ignored as well.
Comment | File | Size | Author |
---|---|---|---|
#4 | advagg-1952522-4-browser-conditionals.patch | 29.74 KB | mikeytown2 |
Comments
Comment #1
mikeytown2 CreditAttribution: mikeytown2 commentedI can't repo this for css; advagg does work with browser conditionals. Will test with the JS patch soon.
Gives me
This
gives me
As for why ie7.css is not included in the aggregate has to do with the preprocess being set to FALSE. Using hook_css_alter, if you change "themes/seven/ie7.css" to have preprocess = TRUE & change the weight of test1/2.css to be the same as ie7.css then things work as expected.
I'll work on a patch for the weight so it only compares the int value as I was having trouble getting my test style sheets to merge in with ie7.css
Putting this inside of a hook_css_alter call made things work as expected
Comment #2
mikeytown2 CreditAttribution: mikeytown2 commentedfurther investigation into the css issue hasn't turned up anything out of the ordinary. The weight issue was a red herring. changing the bundler to 1 caused all files in the ie7 group to be in one aggregate.
Comment #3
mikeytown2 CreditAttribution: mikeytown2 commentedComment #4
mikeytown2 CreditAttribution: mikeytown2 commenteddid find a bug in regards to
array_diff($a, $b);
. Correct usage in my case isarray_merge(array_diff($a, $b), array_diff($b, $a));
.The patch below has been committed. Long story short is you no longer have to apply the patch in #865536-204: drupal_add_js() is missing the 'browsers' option in order for browser conditionals to work.
Comment #5
markhalliwellFrom #2
Hmm, maybe this is what was throwing me. Yes, I want everything in one bundle if possible. In my mind though, I would think that browser conditionals are the exception to this? Shouldn't they be aggregated down to one bundle for each type of condition outside of the main bundle?
That's what I was mostly talking about. Perhaps you fixed that in the patch, I haven't quite read all the way through it.
Comment #6
mikeytown2 CreditAttribution: mikeytown2 commentedie7 group = browser conditional
Comment #7
markhalliwellWell yes, I understand that... but that wasn't happening for me. I was only getting one aggregated CSS file and one JS file. The conditionals were never output as a separate aggregate, perhaps they were thrown in the main aggregated file, dunno... didn't look too hard but it shouldn't do that right? TBH, I probably should have said that this was more of a documented note to check (and have others check) this issue. I'll need to re-test with the dev.
Comment #8
mikeytown2 CreditAttribution: mikeytown2 commentedThe 2 examples in #1 show that on my dev environment I was getting browser conditionals. ctrl-f for
<!--[if lte IE 7]>
to see where they land in the output. Core could have an issue and thus it's output could be wrong #1034208: drupal_sort_css_js() ignores media and browser keys. If AdvAgg still has an issue I would need reproducible code; given this I expect ___ but in reality I got ___.I do appreciate the feedback and testing; the array_diff() issue would have taken a long time to find on its own.
I started a list of core issues that advagg should look into #1956142: Intergrate D7 patches. Input on this list is greatly appreciated.