Problem/Motivation

JavaScript files are only aggregated from AJAX requests, when the maintenance mode constant is set. This was a type in a previous bugfix where files were aggregated even in maintenance mode.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3452672

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

PunamShelke created an issue. See original summary.

punamshelke’s picture

StatusFileSize
new787 bytes
punamshelke’s picture

Status: Active » Needs review
needs-review-queue-bot’s picture

Status: Needs review » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

cilefen’s picture

Version: 10.1.x-dev » 10.3.x-dev
Priority: Critical » Normal
Status: Needs work » Postponed (maintainer needs more info)
Related issues: +#3373328: ^10.1 CSS aggregation breaks during maintenance mode

I think that is intentional but the comment may be wrong. Can you verify by reading over the original issue?

punamshelke’s picture

Hi @cilefen

MIght be intentional but I have tested this aggregation and its not working properly and behaviour is not constant.
After analysis I have found this piece of code and I have compared with with 10.0 then looks like mistake might be I am wrong but want other also test this case before closing....

10.0
10.0

10.1 onwards
10.1

cilefen’s picture

Status: Postponed (maintainer needs more info) » Needs work

It’s understood that #3373328: ^10.1 CSS aggregation breaks during maintenance mode changed the code.

mmillford’s picture

The issue seems to be that it removed a `!` from the line declaring $optimize_js, so now js is only optimized in maintenance mode. #3373328 was focused on the css aggregation, so its understandable that the missing exclamation point for js optimization would be overlooked.

catch made their first commit to this issue’s fork.

catch’s picture

Version: 10.3.x-dev » 11.x-dev
Status: Needs work » Needs review

Was really confused by this issue because aggregation in general is working fine, then realised this affects JavaScript added via AJAX responses only.

I'm not sure if we have an existing test for this that can be adapted. Since it's a one character fix for what was essentially a typo committed to 10.1 already, moving to needs review.

catch’s picture

Title: Aggregate JavaScript files is getting optimized only in maintenance mode » JavaScript files added by AJAX responses are only optimized in maintenance mode
smustgrave’s picture

Status: Needs review » Needs work

Can the issue summary be updated.

This is a small change but should tests be a follow up?

catch’s picture

Issue summary: View changes
Status: Needs work » Needs review

Updated the issue summary. Given this was a typo and it's a regression in released versions, I personally think tests should be a follow-up here. Would be good to add to performance test coverage but we'll need an AJAX request that adds at least two JavaScript files at once, and I'm not sure where to find one of those yet.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

  • nod_ committed 379efd0a on 10.3.x
    Issue #3452672 by catch, PunamShelke, cilefen, smustgrave, mmillford:...

  • nod_ committed 1bcb6598 on 10.4.x
    Issue #3452672 by catch, PunamShelke, cilefen, smustgrave, mmillford:...

  • nod_ committed b6ea46a9 on 11.0.x
    Issue #3452672 by catch, PunamShelke, cilefen, smustgrave, mmillford:...

  • nod_ committed 07abb808 on 11.x
    Issue #3452672 by catch, PunamShelke, cilefen, smustgrave, mmillford:...
nod_’s picture

Version: 11.x-dev » 10.3.x-dev
Assigned: punamshelke » Unassigned
Status: Reviewed & tested by the community » Fixed

Since this is an issue in 10.x I backported to 10.3.x

Committed and pushed 07abb808af to 11.x and b6ea46a9e1 to 11.0.x and 1bcb65983f to 10.4.x and 379efd0a0b to 10.3.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.