Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow underscore in extended static files in theme #32620

Merged
merged 6 commits into from
Jun 16, 2021

Conversation

bentideswell
Copy link
Contributor

@bentideswell bentideswell commented Mar 29, 2021

This PR fixes this issue.

Description

Adds the underscore character to the regex to allow for underscore in module name when extending static file in theme. This is inline with PSR-4 and also inline with other areas of Magento (see related pull requests).

Manual Testing

  1. Extend a file in your custom theme (original doesn't need to exist):
    app/design/frontend/YourVendor/YourTheme/Module_Third_Party/web/css/source/_module.less

  2. Compile the static assets:

    bin/magento setup:static-content:deploy -f

You will now get the error:

Error happened during deploy process: Could not parse theme static file '/path/to/your/install/app/design/frontend/YourVendor/YourTheme/Module_Third_Party/web/css/source/_module.less'

If you then make the single character change in this PR and recompile, it will compile successfully.

Related Pull Requests

11765
14397

Fixed Issues

#32619

@mrtuvn
Copy link
Contributor

mrtuvn commented Mar 30, 2021

Should we allow such case ?
https://devdocs.magento.com/videos/fundamentals/create-a-new-module/
In my opinion named module with underscore may look confused. If we allow underscore i think we should update area related documents too

Can you add more describe how impact to user with this case scenario ?

@bentideswell
Copy link
Contributor Author

Magento already allows this. It is possible to create a module with an underscore in the name (I have several as a way to provide a basic sub-module system). The only issue is with static theme file collecting, the regex here forbids it. There have been other PRs accepted in the past for this issue in other areas (11765 & 14397) and both were quickly accepted.

Magento already allows this and PSR-4 allows this (which Magento claims to support).

By user, I meant more developer but the impact is that if a module is installed with an underscore in the module name that has a static file (I tested with view/frontend/web/css/source/_module.less) then this file cannot be extended in a custom theme due to the regex listed in the issue and PR I created for this issue.

This is a single character code change that brings the static theme file collecting inline with everywhere else in Magento.

@mrtuvn
Copy link
Contributor

mrtuvn commented Mar 30, 2021

Thanks you for clarify! Can you cover this change with some automation tests

Also update Manual testing scenarios in description section for QA team can reproduce for test

@bentideswell
Copy link
Contributor Author

I have updated the original post with a better manual testing section.

I'm not sure how to cover this in unit testing, sorry!

@gabrieldagama gabrieldagama added the Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. label Apr 8, 2021
@mrtuvn mrtuvn added the Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests label Apr 10, 2021
@engcom-Foxtrot engcom-Foxtrot self-assigned this Apr 15, 2021
…amework\App\Utility\Files::accumulateThemeStaticFiles.
@engcom-Foxtrot
Copy link
Contributor

@magento run all tests

@engcom-Foxtrot engcom-Foxtrot added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Apr 15, 2021
@mrtuvn
Copy link
Contributor

mrtuvn commented Apr 16, 2021

@magento run Static Tests, Integration Tests

Added required Magento copyright text to less file
@bentideswell
Copy link
Contributor Author

@magento run Static Tests, Integration Tests

@engcom-Foxtrot
Copy link
Contributor

@magento run all tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@ihor-sviziev
Copy link
Contributor

@magento run Functional Tests B2B

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@ihor-sviziev
Copy link
Contributor

@magento run WebAPI Tests

@magento-automated-testing
Copy link

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

@ihor-sviziev ihor-sviziev added the Severity: S1 Affects critical data or functionality and forces users to employ a workaround. label May 5, 2021
@magento-engcom-team
Copy link
Contributor

Hi @ihor-sviziev, thank you for the review.
ENGCOM-9071 has been created to process this Pull Request

@dmitriyprime
Copy link
Contributor

✔️ QA passed

@m2-assistant
Copy link

m2-assistant bot commented Jun 16, 2021

Hi @bentideswell, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@mrtuvn
Copy link
Contributor

mrtuvn commented May 4, 2022

Hi just wonder is this issue can happen if theme name with 2 underscores ?
Example app/design/frontend/Magento/luma_custom_enhance/

@bentideswell
Copy link
Contributor Author

Theme's aren't part of this issue, as far as I can tell, as the issue relates to PSR-4 autoloading of classes. With a theme name, there is no autoloading tied to the name of the theme as the theme doesn't create it's own PHP classes.

There may be some overlap though, especially in some of the regular expressions that are used to collect files that disallowed underscores. As this issue affected modules I created, I only debugged it from that perspective. If you have issues with a theme with underscores, you will need to:

  1. find the places where it fails
  2. test that it's actually failing because of the underscores
  3. find the code causing the issue in the core
  4. provide a fix or ask on here for help (I'm happy to take a look if you find the problem code)

All of this being said, I had no choice but to push to have the issue resolved as it affected modules my business had already sold and people were using. If it's a custom theme you're building, it might be easier to just change the name to something that works (if the underscore truly is your issue) and move on. I don't know enough about your situation but when I went through this, if it was possible for me to just remove the underscores without affecting many live sites, I would have definitely considered it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Component: App Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Progress: accept Release Line: 2.4 Severity: S1 Affects critical data or functionality and forces users to employ a workaround.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Underscores not allowed in module names in \Magento\Framework\App\Utility\Files::accumulateThemeStaticFiles
7 participants