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

[WIP] Project cleanup? #416

Closed
sreichel opened this issue Jan 3, 2018 · 13 comments
Closed

[WIP] Project cleanup? #416

sreichel opened this issue Jan 3, 2018 · 13 comments
Assignees

Comments

@sreichel
Copy link
Contributor

sreichel commented Jan 3, 2018

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
Copy link
Contributor

seansan commented Jan 3, 2018

  • Optimize queries where possible? (add/remove indexes)
  • DB optimizations (other then indexes)

ps. would not remove downloader as of yet

@LeeSaferite
Copy link
Contributor

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

@LeeSaferite
Copy link
Contributor

LeeSaferite commented Jan 9, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

sreichel commented Feb 1, 2018

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
Copy link
Contributor

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

@dng-dev
Copy link
Contributor

dng-dev commented Nov 28, 2018

i would be in with removing core modules!

@kkrieger85
Copy link
Contributor

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

@sreichel
Copy link
Contributor Author

sreichel commented Dec 3, 2019

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

@sreichel sreichel self-assigned this May 23, 2020
@sreichel sreichel changed the title [Suggestion] Project cleanup? [WIP] Project cleanup? Jun 23, 2020
@addison74
Copy link
Contributor

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

@sreichel
Copy link
Contributor Author

@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
Copy link
Contributor

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

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

No branches or pull requests

8 participants