search

magento / magento2

Prior to making any Submission(s), you must sign an Adobe Contributor License Agreement, available here at: https://opensource.adobe.com/cla.html. All Submissions you make to Adobe Inc. and its affiliates, assigns and subsidiaries (collectively “Adobe”) are subject to the terms of the Adobe Contributor License Agreement.
http://www.magento.com
Open Software License 3.0
11.54k stars 9.32k forks source link

Community themes contain resources for Commerce edition modules #21446

Open clockworkgeek opened 5 years ago

clockworkgeek commented 5 years ago

Summary (*)

The three standard themes have several folders which reference modules that do not exist in Community edition. They mostly contain _module.less files and this leads to unused style rules in the deployed CSS. Also it's a little bit annoying.

Examples (*)

The following folders are superfluous:

adminhtml/Magento/backend/Magento_AdvancedCheckout
adminhtml/Magento/backend/Magento_Banner
adminhtml/Magento/backend/Magento_CatalogPermissions
adminhtml/Magento/backend/Magento_CustomerBalance
adminhtml/Magento/backend/Magento_GiftCard
adminhtml/Magento/backend/Magento_GiftRegistry
adminhtml/Magento/backend/Magento_GiftWrapping
adminhtml/Magento/backend/Magento_Reward
adminhtml/Magento/backend/Magento_Rma
adminhtml/Magento/backend/Magento_VersionsCms
adminhtml/Magento/backend/Magento_VisualMerchandiser
frontend/Magento/black/Magento_AdvancedCheckout
frontend/Magento/black/Magento_Banner
frontend/Magento/black/Magento_CatalogEvent
frontend/Magento/black/Magento_GiftCard
frontend/Magento/black/Magento_GiftCardAccount
frontend/Magento/black/Magento_GiftRegistry
frontend/Magento/black/Magento_GiftWrapping
frontend/Magento/black/Magento_Invitation
frontend/Magento/black/Magento_MultipleWishlist
frontend/Magento/black/Magento_Rma
frontend/Magento/black/Magento_VersionsCms
frontend/Magento/luma/Magento_AdvancedCheckout
frontend/Magento/luma/Magento_CustomerBalance
frontend/Magento/luma/Magento_GiftCard
frontend/Magento/luma/Magento_GiftCardAccount
frontend/Magento/luma/Magento_GiftWrapping
frontend/Magento/luma/Magento_Invitation
frontend/Magento/luma/Magento_MultipleWishlist
frontend/Magento/luma/Magento_Reward
frontend/Magento/luma/Magento_Rma

Proposed solution

Move the mentioned resources to the appropriate theme or the modules' own view folders.

magento-engcom-team commented 5 years ago

Hi @clockworkgeek. Thank you for your report. To help us process this issue please make sure that you provided the following information:

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento-engcom-team give me 2.3-develop instance - upcoming 2.3.x release

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

@clockworkgeek do you confirm that you was able to reproduce the issue on vanilla Magento instance following steps to reproduce?

magento-engcom-team commented 5 years ago

Hi @engcom-backlog-nazar. Thank you for working on this issue. In order to make sure that issue has enough information and ready for development, please read and check the following instruction: :point_down:

ghost commented 5 years ago

Hi @clockworkgeek thank you for you report, this modules not deployed to frontend or adminhtml, if you recheck pub/static folder this files not exists. deepinscreenshot_select-area_20190226095518 deepinscreenshot_select-area_20190226095546

clockworkgeek commented 5 years ago

Thank you @engcom-backlog-nazar for looking into this. LESS files are not static so will not be deployed to pub/static. They are copied to var/view_preprocessed and then compiled into styles.css, styles-m.css, and styles-l.css.

magento-engcom-team commented 5 years ago

:white_check_mark: Confirmed by @engcom-backlog-nazar Thank you for verifying the issue. Based on the provided information internal tickets MAGETWO-98455, MAGETWO-98456 were created

Issue Available: @engcom-backlog-nazar, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

m2-assistant[bot] commented 5 years ago

Hi @cmtickle. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

sdzhepa commented 5 years ago

Hello @clockworkgeek

Thank you for contribution and collaboration!

I can confirm that from the first glance there is no sense to have these *.less files in Open Source Codebase. And it is logical and obvious decision to move them to Commerce Source.

But it will have negative consequences on composer installation approach. Especially how Magento builds the packages for composer We do not have separate theme packages(blank/luma) for Open Source and Commerce yet

To develop complex fix for this issue need to have access to "Magento Composer package builder" which is internal only.

So, unfortunately, we have to close this PR and related issue and wait for fix from Magento Core

Also, want to admit that the internal team several times tried to fix this issue by moving *.less but changes were reverted.

Additional information Internal Jira ticket with this issue:

Currently has REOPEN status and blocked by another architecture ticket:

clockworkgeek commented 5 years ago

Thank you @sdzhepa for looking into this in detail. I hope you will find a solution.

andrewbess commented 4 years ago

Hello @sdzhepa I think we can move the less files of adminhtml area from Magento CE to Magento EE (into modules). Sergey, what do you think about it?

sdzhepa commented 4 years ago

Hello @andrewbess

It seems your proposition has sense.

I see that internal Jira ticket MAGETWO-44582 "Theme Extension Strategy" still in status = OPEN Let's try to ask @antonkril to help with this issue or with involving appropriate contact to discuss it here.

antonkril commented 4 years ago

Makes sense. @buskamuza will probably be the best person to investigate the reason why the previous attempts failed? cc @maghamed

m2-assistant[bot] commented 4 years ago

Hi @andrewbess. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if you want to validate it one more time, please, go though the following instruction:

andrewbess commented 4 years ago

@antonkril I think it because it doesn't look like the correct solution. Please take a look at the screenshot below.

wrong-file-path
antonkril commented 4 years ago

Yeah, I remembered that discussion now. In both options, we end up with files that are potentially unused. But your proposal was about adminhtml, which, with AdminUI modules should not be a problem.

andrewbess commented 4 years ago

So, I add the checker of the fixed file here