the js alter hook replaces core files with bootstrap - fixed versions by matching directory structure and filenames and replacing where matches are found with the requested JS. That works with JS aggregation off, but when it's on the files are renamed with *.min.js, so they don't match the paths that are supposed to be replacing.

This patch removes $min if present in the path being checked.

I've also changed the JS key being updated to retain the original JS key, which other modules may be looking for in JS alters. Updating the "data' array item is sufficient to specify the bootstrap replacement.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

fearlsgroove’s picture

FileSize
1.35 KB

Oops patch named wrong. Same file.

glynster’s picture

Perfect this patch helped with fieldsets and collapsing breaking, for me this was in Ubercart checkout, this saved me as I was pulling my hair out!!

Now I may have found one other place which is the Password section inside the Account Settings. When JS aggregation is off the password strength etc works well. As soon as I aggregate that JS stops working.

Any ideas?

fearlsgroove’s picture

FileSize
902 bytes

Without whitespace changes.

glynster’s picture

Seems we have another issue with the Views Ajax throbber. Once aggregation is on all theming is stripped. I wish I could help more, the best I can do is find things that seem to break :(

glynster’s picture

Seems this patch never made it into the last dev update.

rudetrue’s picture

It looks like this commit broke the patch; it removes the $min variable:
http://cgit.drupalcode.org/bootstrap/commit/includes/alter.inc?id=f05a44...

rudetrue’s picture

FileSize
934 bytes

I re-rolled and replaced the $min variable. I didn't analyze it very thoroughly, but it seems right to me.

fearlsgroove’s picture

The commit that added grunt workflow was what created this bug (I think), so since that was rolled back it shouldn't be needed anymore. Not certain tho haven't tested.

ShaunLaws’s picture

I've just stumbled across this problem as I'm preparing a site for production and have just enabled JS aggregation. I'm looking at patching the theme, but am wondering if this line in the patch is correct:

+ $path = dirname($path) . '/' . str_replace(variable_get('preprocess_css', ''), '', preg_replace('/^[_]/', '', $file->filename));

Shouldn't the test be based on variable_get('preprocess_js'?

torgosPizza’s picture

#9, yes - that does appear to be a typo in the patch.

glynster’s picture

You should not need this patch anymore as the latest dev version deals with this.

fearlsgroove’s picture

Status: Needs review » Closed (works as designed)

Closing this since it's not relevant anymore. This was an issue only for a few commits on -dev after the grunt tools were added and before they were removed.