Skip to content

Conversation

@hostep
Copy link
Contributor

@hostep hostep commented Aug 15, 2018

Please: do not backport this PR to 2.2, the solution for 2.2 is different and already exists: #17626

Description

The Magento Swagger module contains already minified js files:

This PR marks them to not be minified again during static content deploy, because they get changed in a way that they are no longer valid JS.
This PR renames them so they contain .min and reference them as such from the layout xml file as well. This way they don't get minified again in production mode when minification is enabled.

I've already gone ahead and created a fix for Magento 2.2 as well , since the solution presented in here doesn't work on 2.2, this is due to #13687 which only exists in 2.3
PR for Magento 2.2: #17626

Fixed Issues (if relevant)

  1. 2.2.5 Swagger: With JS minification enabled, the swagger-ui-bundle.js becomes corrupted #16927: 2.2.5 Swagger: With JS minification enabled, the swagger-ui-bundle.js becomes corrupted

Manual testing scenarios

  1. Set up a clean Magento 2.3-develop installation (using commit 05f9df7)
  2. Run: bin/magento config:set dev/js/minify_files 1
  3. Run: bin/magento deploy:mode:set production
  4. Go to: https://myinstallation.domain/swagger
  5. See JS errors: Uncaught SyntaxError: Invalid regular expression flag & Uncaught ReferenceError: SwaggerUIBundle is not defined

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @hostep. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.0 milestone Aug 16, 2018
@magento-engcom-team
Copy link
Contributor

Hi @miguelbalparda, thank you for the review.
ENGCOM-2811 has been created to process this Pull Request

@hostep
Copy link
Contributor Author

hostep commented Aug 16, 2018

@miguelbalparda: is it possible to put this on hold for a bit? I just thought of another way to fix this and the fix would be the same in 2.2 and 2.3 and is probably a better way of solving this than what I was currently suggesting.

I'll try to have something ready within a few hours to see if it works.

@miguelbalparda
Copy link
Contributor

Definitely @hostep, thanks for the quick reply. Just let me know the next steps and I'l be happy to help.

Copy link
Contributor

@miguelbalparda miguelbalparda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On hold for now, don't process.

…odule so they aren't getting minified again in production, fixes magento#16927 - for Magento 2.3
@hostep hostep force-pushed the fix-issue-16927-for-magento-23 branch from d15683f to 6aeec47 Compare August 16, 2018 17:30
@hostep hostep changed the title Exclude already minified js files from minification in the Swagger mo… Use '.min' in filenames of already minified js files in the Swagger module so they aren't getting minified again in production, fixes #16927 - for Magento 2.3 Aug 16, 2018
@hostep
Copy link
Contributor Author

hostep commented Aug 16, 2018

@miguelbalparda: I took a completely different approach now, I think it's a better solution and easier to maintain. I've amended my previous commit and force pushed my branch. I also updated the description and title in the opening post so it makes more sense with this new solution. Sorry for the mess! :)

@magento-engcom-team
Copy link
Contributor

Hi @miguelbalparda, thank you for the review.
ENGCOM-2811 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

Hi @hostep. Thank you for your contribution.
We will aim to release these changes as part of 2.3.0.
Please check the release notes for final confirmation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants