Skip to content

Conversation

@luigifab
Copy link
Contributor

@luigifab luigifab commented Feb 21, 2021

Description

This PR will hurt our feelings 💣 because it remove 99% of Mysql4 PHP class and the deprecatedNode XML node.
This will break 💥 many "outdated" modules.

There is a migration script: php shell/remove-mysql4.php
It will update all occurrences of mysql4 classes.

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Component: AdminNotification Relates to Mage_AdminNotification Component: Adminhtml Relates to Mage_Adminhtml Component: Admin Relates to Mage_Admin Component: Api PageRelates to Mage_Api Component: Backup Relates to Mage_Backup Component: Bundle Relates to Mage_Bundle Component: Catalog Relates to Mage_Catalog Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogRule Relates to Mage_CatalogRule Component: CatalogIndex Relates to Mage_CatalogIndex Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Cm/RedisSession Relates to Cm_RedisSession Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Cron Relates to Mage_Cron Component: Customer Relates to Mage_Customer Component: Dataflow Relates to Mage_Dataflow Component: Directory Relates to Mage_Directory Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: ImportExport Relates to Mage_ImportExport Component: Index Relates to Mage_Index Component: Install Relates to Mage_Install Component: Log Relates to Mage_Log Component: Media Relates to Mage_Media Component: Page Relates to Mage_Page Component: Paygate Relates to Mage_Paygate Component: PayPal Relates to Mage_Paypal Component: PaypalUk Relates to Mage_PaypalUk labels Feb 21, 2021
@jfksk
Copy link

jfksk commented Jul 16, 2021

@addison74

what happens if an extension uses the mysql4 format?

The PR includes a migration script for that case

Has anyone performed a test for such a situation? Do we have to modify all extensions we currently use?

I tried it w/ two stores which are around since m1.3/m1.4 - with a good amount of mysql4 resources. The smaller code-base is definitively fine, the larger one needs more testing but I think it'll be fine too. I only used the migration script and didn't change anything manually - so far...

jfksk
jfksk previously approved these changes Oct 16, 2021
Copy link

@jfksk jfksk left a comment

Choose a reason for hiding this comment

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

In production for > 1 month

@luigifab luigifab dismissed stale reviews from jfksk and kiatng via 8745323 December 21, 2021 21:32
@luigifab luigifab requested a review from kiatng December 21, 2021 21:32
kiatng
kiatng previously approved these changes Dec 22, 2021
Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Tested and worked for months now.

@addison74
Copy link
Contributor

When there are so many changes involving dozens and even hundreds of files I think it would be best to break them into several PRs. I don't always have the ability to analyze modified files in an IDE and choose to do this task in the browser on my tablet.

I understand that everything can be done in a PR but when there are several pieces it is easier to do a reverse if issues occur. I personally agree with this change but I cannot approve it without intensive testing and modify the existing extensions I am using.

As a personal opinion such a PR should have a higher number of approvals to avoid further inconveniences and should be merged by a maintainer.

@fballiano
Copy link
Contributor

I like this PR but:

  • it needs a README section
  • I'd rename the shell/rename-mysql.php script with a name that's a bit more verbose

@luigifab
Copy link
Contributor Author

luigifab commented Jul 4, 2022

For readme, for what version?
For script name, rename-mysql4-class-to-resource.php ?

@fballiano
Copy link
Contributor

right, we need another PR to modify the README in the v19 branch :-( that sucks... so... let's address it later but we should not forget it (actually we should put it in the release notes too)

I guess the "shell/" filename is ok, maybe I'll add a "-pr1470.php" suffix?

@midlan
Copy link
Contributor

midlan commented Jul 7, 2022

I like just rename-mysql4-class-to-resource.php
PR can be found easily by commit :)

@addison74 addison74 mentioned this pull request Aug 31, 2022
4 tasks
@sreichel sreichel mentioned this pull request Sep 1, 2022
4 tasks
@luigifab luigifab closed this Sep 2, 2022
@luigifab luigifab deleted the remove-mysql4 branch September 2, 2022 18:52
@sreichel
Copy link
Contributor

sreichel commented Sep 2, 2022

This should stay open.

@luigifab
Copy link
Contributor Author

luigifab commented Sep 3, 2022

Sorry, too much conflicts.
I continue to use it.

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

Labels

Component: Admin Relates to Mage_Admin Component: Adminhtml Relates to Mage_Adminhtml Component: AdminNotification Relates to Mage_AdminNotification Component: Api PageRelates to Mage_Api Component: Backup Relates to Mage_Backup Component: Bundle Relates to Mage_Bundle Component: Catalog Relates to Mage_Catalog Component: CatalogIndex Relates to Mage_CatalogIndex Component: CatalogInventory Relates to Mage_CatalogInventory Component: CatalogRule Relates to Mage_CatalogRule Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Cm/RedisSession Relates to Cm_RedisSession Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Cron Relates to Mage_Cron Component: Customer Relates to Mage_Customer Component: Dataflow Relates to Mage_Dataflow Component: Directory Relates to Mage_Directory Component: Downloadable Relates to Mage_Downloadable Component: Eav Relates to Mage_Eav Component: ImportExport Relates to Mage_ImportExport Component: Index Relates to Mage_Index Component: Install Relates to Mage_Install Component: Log Relates to Mage_Log Component: Media Relates to Mage_Media Component: Page Relates to Mage_Page Component: Paygate Relates to Mage_Paygate Component: PayPal Relates to Mage_Paypal Component: PaypalUk Relates to Mage_PaypalUk Component: Poll Relates to Mage_Poll Component: ProductAlert Relates to Mage_ProductAlert Component: Rating Relates to Mage_Rating Component: Reports Relates to Mage_Reports Component: Review Relates to Mage_Review Component: Rss Relates to Mage_Rss Component: Rule Relates to Mage_Rule Component: Sales Relates to Mage_Sales Component: SalesRule Relates to Mage_SalesRule Component: Sendfriend Relates to Mage_Sendfriend Component: Shipping Relates to Mage_Shipping Component: Sitemap Relates to Mage_Sitemap Component: Tag Relates to Mage_Tag Component: Tax Relates to Mage_Tax Component: Weee Relates to Mage_Weee Component: Widget Relates to Mage_Widget Component: Wishlist Relates to Mage_Wishlist Don't forget this PR shell Relates to shell scripts

Projects

None yet

Development

Successfully merging this pull request may close these issues.