(BC) Removed mysql4

[luigifab]

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

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

[sreichel]

Nice, but I can't hit "approve" ... someone is sure to get homicidal urges :P

If you have time for, can you check https://github.com/OpenMageModuleFostering how often mysql4 is used?

[jouriy]

This will break widely used Ess/M2epro and Aheadworks extensions (AW/Blog, AW/Core etc.), so likely hit 9/10 of stores..

[addison74]

This is a dangerous commit because a lot of extensions are still using the old style. I confirm Aheadworks is one. If it is merged all these extensions must be checked and updated for compatibility with the latest OpenMage version. Most of the M1 developers abandoned their projects, removed the products from the offer, some of them can do the changes but for a lot of money. As I see there are 847 files to be changed ...

[luigifab]

That's true. Perhaps we can propose a PHP script to do the class name changes automatically? Is there a way to clone all git repositories of OpenMageModuleFostering in one command line?

[addison74]

If this commit will have a chance to be merged you should focus on resolving all extensions out there not only those in OpenMageModuleFostering. God knows what issues could have these extensions being unmaintained for years.

I agree it is a good improvement to get rid off almost 1000 files but a migration script for current extensions is needed. With one command in Terminal based on this script all old Mysql4 references in php files to be converted.

Also, before being merged this change must be announced to all OpenMage users to avoid any "surprises". A backup and migration of the current used extensions must be done prior in a test environment before upgrading OpenMage in production.

I could draw a parallel with the Backup module that was not removed from the code. Magento Team preferred the solution to disable it. This commit is really a turning point in Magento :)

[colinmollenhour]

Thanks for the PR @luigifab.

Let's compare the cost vs benefit:

Cost:

• Will likely break some old code, but the fix is also simple yet tedious
• How much breakage remains to be determined.. What about encoded modules, would this make it impossible to fix?

Benefit:

• 18k lines of code removed, although not really code just empty placeholders
• Probably negligible performance improvement. But has anyone tested before/after? Would be interesting, I am guessing no perceivable difference but sometimes I am surprised
• A continued evolution of the project rather than refusing to make changes that are correct but only bad because it requires some minor fixes

Caveats:

• A tool could be written (perhaps as a feature of n98-magerun?) which would do some basic string replacement
• Is there a path forward for encoded modules? E.g. can they be fixed by restoring the specific deleted Mysql4 classes that the encoded files depend on?

It would be good to know how prominent these Mysql4 classes are and if there is any real performance improvement for merging this PR to have some data to inform the decision. My gut right now is to definitely not merge it into 1.9.4.x branch but strongly consider merging it into a newer branch with ideally some tooling to do the string replacement or at least a clear guide on how to fix old code (especially if there is a measurable performance improvement).

[addison74]

I agree @colinmollenhour. This project really needs improvements not only bug & security fixes. A project dies when there is a lack of enthusiasm. Magento 1 has received a special attention for almost 12 years from many users and developers and there are important resources for achieving almost anything. It is a pity this legacy is lost.

I now ask a technical question. Instead of changing code in extensions, could a compatibility be ensured by creating a dedicated module? Specifically, all those references from the nearly 1000 files are brought in one place. This module becomes an alias for everything Mysql4 means in Magento. It is indeed a relic and a cleanliness would be required.

Unfortunately those encoded extensions can only be solved by their developers. I never used any of them.

[joshua-bn]

You could potentially use the getModelInstance() method to "find" the mysql4 model. If it fails with resource, then try mysql4. I think to make this work, you need that.

IMO, for this project to work, it needs to be very careful about how it handles BC. If there are such BC changes that it will break people's sites regularly, why wouldn't someone just go to M2 or another ecom solution?

[Flyingmana]

Technical, what you are removing in this Pull Request is the compatibility Layer. They do not contain any logic, no potential for any Bugs, just pseudo classes existing for keeping the classnames working. They are also already marked as deprecated.

Performance wise, we have a bit below 1.000 affected files, one older (2012) class loader benchmark measures this with around 30ms, if they are all actually loaded. There were several optimizations since then. But generally I support the removal.

As this is a bigger change affecting our ecosystem, I would suggest to put it first through the RFC process. It should not be the responsibility of the Maintainer Team to decide on the Timeline and which steps are done if it affects so many users. With the RFC Process the Ecosystem/Community can vote on it, and if the Majority is in favor of removing it, we can do it. Additionally we can also propose different Plans for the removal Process, like just refactoring the Usage for Frontend Requests first, and do the actual removal at a later point. And also if we actually want to have some additional logic to preserve Backwards Compatibility (which is possibly more error prone then the current code is)

[midlan]

I like it as others. Another thing, this is BC break, the base branch should be changed to 20.x?

[Sekiphp]

It will be funny to maintain some projects like this one...

[jfksk]

Reliability is definitely one of the core characteristics of business software and therefor extremely important for OM but I also think that OM needs to move forward in a slowly accelerating way.
I like the RFC idea and would love to see a a little bit more generalized and extended. There are many things marked as deprecated for a long time now. Maybe we can schedule a step-by-step phase out? Maybe we could introduce an NextGen branch? However I'm getting off topic. I like the PR and I'll def. try it out within the next days. I am already remove other stuff in the upgrade process, why not that :)

[tmewes]

Sorry if I misunderstood something, but isn't the (NextGen) branch 20.0 exactly for such great work like @luigifab did here? I can completely understand that some of us need a very slow further development of OpenMage – what the 1.9.4.x branch is intended for. For many others (including myself), OpenMage is the chance to turn the dusty magento1 into a modern, fast and safe system without legacy like supporting outdated (encoded) modules that won't be patched anyway. In shops where such problematic modules are still used in production, adjustments have to be done in any case (better now than later). If such corpses still have to be dragged along, that's in my opinion just an unnecessary bloating of the code and a waste of the potential OpenMage has. As long as such (eventually) breaking changes are only merged into the 20.0 branch, there shouldn’t be any problems and both worlds are served and happy.

tl;dr: Why isn't this PR already merged in 20.0? :D

[kiatng]

I have just checkout this PR and merged it to my local repo for testing. I followed this guide. I used scoop to install the github CLI to my PC (I tried winget at first, but I couldn't manage). I'll run it for a few weeks, at least. If you really like this PR, then please help by reviewing, testing, and approving. This will help to "persuade" the maintainers to merge it sooner.

[luigifab]

I added a script to automatize search and replace.

[luigifab]

Besides me, who deployed this idea in production?

[jfksk]

I applied it now twice to WIP projects. The first one (a small store) is probably going live soon. I didn't see any problem. The other one (a large store) needs more testing. The process was very smooth in both cases. Apply the patch, run the script, enjoy less clutter. :)

[addison74]

Being an important PR that aims at several hundred changes must be analyzed in all aspects before being merged in OM 20.

For a strict change in OM I'm not worried but what happens if an extension uses the mysql4 format? Has anyone performed a test for such a situation? Do we have to modify all extensions we currently use?

[jfksk]

@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...

[addison74]

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]

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]

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

[fballiano]

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]

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

[sreichel]

This should stay open.

[luigifab]

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