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

Remove "default" themes from 20.x #1600

Merged
merged 1 commit into from
May 12, 2021
Merged

Remove "default" themes from 20.x #1600

merged 1 commit into from
May 12, 2021

Conversation

fballiano
Copy link
Contributor

This PR removes app/design/frontend/default and skin/frontend/default.

Those themes are very old, not good looking and nobody (who keeps magento somewhat updated) is using them.

With this PR we delete almost 580 (useless) files from the repository.

Fixed Issues

  1. Fixes Discussion: Remove app/design/frontend/default/iphone and app/design/frontend/default/modern #1037

Manual testing scenarios (*)

Just browse a new installation and see that the RWD theme (the default since some time) doesn't need the old "default" ones to work.

Questions or comments

  • The only people who could have problems with this PR is somebody using composer. But really, who's using composer and this extremely old themes at the same time? If they're not using composer and just copying the new version on top of the other there will be no problem cause the old files with still be there.
  • I've big stores in productions without these 2 folders since years without any problems (with commercial themes or custom made ones)

@github-actions github-actions bot added Component: Bundle Relates to Mage_Bundle Component: Catalog Relates to Mage_Catalog Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: Contacts Relates to Mage_Contacts Component: Customer Relates to Mage_Customer Component: Downloadable Relates to Mage_Downloadable Component: Newsletter Relates to Mage_Newsletter Component: Page Relates to Mage_Page Component: Payment Relates to Mage_Payment Component: PayPal Relates to Mage_Paypal Component: Persistant Relates to Mage_Persistant Component: Review Relates to Mage_Review Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales Component: Sendfriend Relates to Mage_Sendfriend Component: Shipping Relates to Mage_Shipping Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Wishlist Relates to Mage_Wishlist Template : default Relates to base template XML Layout labels May 5, 2021
@lgtm-com
Copy link

lgtm-com bot commented May 5, 2021

This pull request fixes 17 alerts when merging 2485d0d into 46ebf30 - view on LGTM.com

fixed alerts:

  • 11 for Unused variable, import, function or class
  • 5 for Missing variable declaration
  • 1 for Expression has no effect

@addison74
Copy link
Contributor

I agree. Relics from the time when the design was not responsive.

PS - /errors directory should be revised too.

@luigifab
Copy link
Contributor

luigifab commented May 6, 2021

This remove the legacy frontend theme:

image

I don't know what to think.

Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

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

I approve since these themes are incredibly old (e.g. IE6 support) and in the off chance someone wants them they can just grab the files from the repo.

@fballiano
Copy link
Contributor Author

@luigifab was your frontend set to use the default? because the new installations have RWD by default if I didn't get everything wrong when I tested it.

Copy link
Contributor

@luigifab luigifab left a comment

Choose a reason for hiding this comment

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

No, it was set to my dev website very long time ago.

@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
6 runs  ±0  6 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 721501a. ± Comparison against base commit 46ebf30.

@addison74
Copy link
Contributor

I see this PR as merged but I would like to clarify a potential issue that could appear. As you know OM is shipped with /app/design/frontend/default. Into this folder there are the following directories:

  • blank
  • default
  • iphone
  • modern

Most of the themes on the market are requesting installations into this directory. If a file is missing from theme Magento fallback will solve the issue and will search in /frontend/default/default then in frontend/base.

If we remove /default/default directory are we creating any errors with fallback model?

@Flyingmana
Copy link
Contributor

Yes, these themes will possibly break then.

@fballiano
Copy link
Contributor Author

fballiano commented May 16, 2021 via email

@addison74
Copy link
Contributor

addison74 commented May 16, 2021

@Flyingmana: In this case we should remove only /blank, /modern and /iphone directories in /app/designfrontend/default. Also, blank, blue, french, german, iphone, modern directory in /skin/frontend/default.

@fballiano : It is just a matter of fallback model in Magento. OM should keep backward compatibility related to default folder for old themes installed in /frontend/default directory. Please note that although OM wants to be a better Magento it is doomed to work with old products such as themes and extensions. What we do through OM is just to dress the code, to add facilities, but 99% of the people who are using the platform adapt it to their own needs using this "old stuff".

An eloquent phrase says "If it ain't broke, don't fix it".

@fballiano
Copy link
Contributor Author

fballiano commented May 16, 2021 via email

@tmotyl
Copy link
Contributor

tmotyl commented May 17, 2021

@addison74 would a solution for you be to have a new package on openmage github with these old themes?
This would alow to easily install them by whoever still needs them.

@addison74
Copy link
Contributor

@tmotyl: I think my original post was misunderstood. I did not mean to allow myself to continue using old themes from this directory /frontend/default which was delivered by Magento but to a completely different situation.

If I install a theme called Luna in the directory /frontend/default/luna then if a layout/template file is not in the theme directory then Magento looks for it in the /front/default directory on the same level, if it doesn't find it looks up one level in the /frontend/base.

This PR deletes the default directory and I asked to evaluate if deleting it could affect this fallback. I did a simulation by renaming the directory and nothing happened. But I did not check if there were any errors in the logs and the page load time was longer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Bundle Relates to Mage_Bundle Component: Catalog Relates to Mage_Catalog Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: Contacts Relates to Mage_Contacts Component: Customer Relates to Mage_Customer Component: Downloadable Relates to Mage_Downloadable Component: Newsletter Relates to Mage_Newsletter Component: Page Relates to Mage_Page Component: Payment Relates to Mage_Payment Component: PayPal Relates to Mage_Paypal Component: Persistant Relates to Mage_Persistant Component: Review Relates to Mage_Review Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales Component: Sendfriend Relates to Mage_Sendfriend Component: Shipping Relates to Mage_Shipping Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Wishlist Relates to Mage_Wishlist Template : default Relates to base template XML Layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants