search

OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
868 stars 436 forks source link

[WIP] Project cleanup? #416

Closed sreichel closed 1 year ago

sreichel commented 6 years ago

I doubt there will be a M1.10 with bigger code changes, so why not to tidy up this project?

Cleanup:

"Improvements beyond bug fixes" (features):

"full" IDE support / DOC block updates

development

However, I would also consider this project to be more of a "power-user" derivative and think that dropping support for old versions and even making PHP 7 the minimum would not be unreasonable.

I'd keep/update the master (1.9.3.x) branch as usual to not break anything, but start a new 1.9.3.x-dev that includes also BC breaking changes (like PHP7 required, or removing deprecated methods)

seansan commented 6 years ago

ps. would not remove downloader as of yet

LeeSaferite commented 6 years ago

I think the Venn diagram of power users and connect users would be quite small.

LeeSaferite commented 6 years ago

FWIW, I'm totally on board with this idea. I would love to see some real love applied to the M1 codebase.

A project I've started multiple times and would love to complete is fixing the broken modularization of M1. One aspect of that is controller locations. Since in the early days you could only have one module per shortname, they moved all the admin code into the Mage_Adminhtml module. After they added the ability to register multiple modules on a shortname (I like to think my module pushed them to do that, hah!), they started adding admin related controllers in their respective modules, but they never moved the original controllers to their correct modules. Moving them is fairly easy. And we can replace the moved controllers with stubs that call to the correct controller and log a deprecation warning. Another aspect of the modularization issue is underclared and circular deps in the modules. That's a tad harder to resolve, but is still possible. I know that M1 is an EOL platform at this point, but I think with some community effort it could live on in parallel for quite a while.

In any case, I think we should make a todo list of all the suggestions and get some community feedback

sreichel commented 6 years ago

A project I've started multiple times and would love to complete is fixing the broken modularization of M1. One aspect of that is controller locations.

W'd like to see this ... but how to start? Guess waiting for a todo lit will take ages, so why not start a feature or dev branch and start commiting?

edannenberg commented 6 years ago

Intrigued by #435 I ran PhpStorm's code inspection on the repo. Oh my. :) I guess a lot of them are to be expected due to the custom aspects of Magento's core but def. plenty of code smells like broken function signature calls.

Was only interested in undefined vars like in the linked issue today, not too bad, but still a few that should be looked at. (untick "Variable might have not been defined" in inspection). Created a PR for an obvious one, the rest would need a closer look for fixing.

sreichel commented 6 years ago

Yep, there are a lot of them ...

Im not sure, about fixing or just remove the whole method ... there are some that could not work and would produce a warning or fatal error (#419). This code is not used anywhere ... remove it.

LeeSaferite commented 6 years ago

@sreichel BTW, I've done the layout refactor, I just need to get a PR issued for it at some point.

dng-dev commented 5 years ago

i would be in with removing core modules!

kkrieger85 commented 4 years ago

@sreichel @Flyingmana Is this list still WIP? Could you please update tasks? Maybe with references to other issues?

sreichel commented 4 years ago

@kkrieger85 Had no time during pasts weeks. I'll update ASAP.

ADDISON74 commented 2 years ago

Excellent job. It is a pity for so much effort if the work is not completed.

sreichel commented 2 years ago

@ADDISON74 biggest part is done. DOCblocks for Adminhtml would be a large PR. So i dont worry,. The only PR i'd like to see get merged is #249 ... (still needs testing)

ADDISON74 commented 2 years ago

@sreichel - PR #249 is closed. What shall we do in this case? Re-open it?