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

Removed unused autoloaders Mage_Autoload_Simple and Magento\Autoload\Simple #2859

Merged
merged 2 commits into from
Jan 10, 2023

Conversation

fballiano
Copy link
Contributor

This PR may be wrong but it seems to me these 2 autoloaders are never used, am I missing something? Why do we have 3 autoloaders in our repo?

PS for @sreichel: I think phpstan should run when we're deleting files but it's not running on this PR.

@github-actions github-actions bot added Component: lib/Mage Relates to lib/Mage Component: lib/* Relates to lib/* Component: lib/Magento Relates to lib/Magento labels Dec 28, 2022
Copy link
Contributor

@Flyingmana Flyingmana left a comment

Choose a reason for hiding this comment

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

its not a bugfix, and its not an urgently needed change, so it should not go into this version. Preferable it should go into an upcoming one (as modules might still make use of these classes)

@fballiano
Copy link
Contributor Author

@Flyingmana I'm of the opposite opinion, we have the first minor release since the beginning of OM, if there's the time to make some change this is the time IMHO, we may not have another minor for months or years.

so I'd like do have some other opinions on this.

@fballiano
Copy link
Contributor Author

The truth is, this should be on v20, but it's become unbearable to constantly have conflict when everything gets done on v19.

@Flyingmana
Copy link
Contributor

The truth is, this should be on v20, but it's become unbearable to constantly have conflict when everything gets done on v19.

the conflict potential here is very low, especially if we got to only do bugfixes.
Because how likely is it, that we need to do a bugfix in this files here?
Most conflicts currently are from the codestyle changes I think? So I understand that we have them there, and that we want to do the codestyle changes in all branches. But there will be a point when we are finished with them.

sreichel added a commit that referenced this pull request Dec 29, 2022
sreichel added a commit that referenced this pull request Dec 29, 2022
@fballiano
Copy link
Contributor Author

But still, let's take the classmap autoloader, there's no documentation and those files are never called by anyone in the core. why should somebody have used it really? and they're upgrading? we keep worrying about dinosaurs more than microsoft with windowsXP 😂

@Flyingmana Flyingmana requested review from Flyingmana and removed request for Flyingmana December 30, 2022 18:29
@Flyingmana Flyingmana dismissed their stale review December 30, 2022 18:32

critic was received

@Flyingmana
Copy link
Contributor

But still, let's take the classmap autoloader, there's no documentation and those files are never called by anyone in the core. why should somebody have used it really? and they're upgrading? we keep worrying about dinosaurs more than microsoft with windowsXP 😂

most importantly, because the promise of an LTS version is, to break as little as possible.
And from a product perspective, users receive breaking changes a lot more intensive then anything else.

And when you compare this with windowsXP, then please do it right 🙄
I did only want to keep it in the current version, removing it in the next one was okay.
Windows is known for offering compatibility over multiple versions, and especially windowsXP keept compatibility of over 10 years of prior major releases.
Even with the most defensive Release proposal in the RFC we are at a maximum of 6 years, just for keeping support for it.

@fballiano
Copy link
Contributor Author

oh cmon don't takes things so literally.

what's literal it's that we're the only software in the world that works backwards, we keep modifying the old version neglecting the new one. the main branch should have been 20.0 since the day it was created and 19.0 should have been only about the LTS, not about "let's do everything there". I'll never get tired of saying that. the way we're managing it is wrong.

@Flyingmana
Copy link
Contributor

oh cmon don't takes things so literally.

what's literal it's that we're the only software in the world that works backwards, we keep modifying the old version neglecting the new one. the main branch should have been 20.0 since the day it was created and 19.0 should have been only about the LTS, not about "let's do everything there". I'll never get tired of saying that. the way we're managing it is wrong.

it was keept like this, because at the time 90% of contributions were bugfixes and forward its possible to merge them at once.
Its the responsibility of reviewers and mergers to make sure no features are merged to 19.0.
And there is nothing which says we cant change the default branch by now 👀

@fballiano
Copy link
Contributor Author

what? I keep saying that v20 should be default since my first contribution and seems nobody likes the idea.

@fballiano
Copy link
Contributor Author

btw I'll finish https://github.com/fballiano/openmage-rfcs/blob/branchrules/draft/0000-new-branching-rules-2022.md ASAP, I didn't post it because I asked for collaboration here #2327 (comment) but didn't get any answer

@sreichel
Copy link
Collaborator

sreichel commented Dec 30, 2022

Its the responsibility of reviewers and mergers to make sure no features are merged to 19.0.

WHAT??? New features that did not break anything have been merged since the beginning! v20 was never intended to be "default" - with allowing BC breaks.

@fballiano
Copy link
Contributor Author

I think we should touch v19 as little as possible but I accept that's how we work cause otherwise we wouldn't approve anything heheheh.

But this opinions war happens because there was never a clear RFC about branch rules.

@sreichel
Copy link
Collaborator

I agree with v20 being default, when we completly ignore BC breaks. I'd like to add typhints everywhere 😎 ...

@fballiano
Copy link
Contributor Author

I agree with v20 being default, when we completly ignore BC breaks. I'd like to add typhints everywhere 😎 ...

I'd love to see that

@sreichel
Copy link
Collaborator

IMHO ... not by wording, but in case of OpenMage "LTS" was always meant to be "drop-in" replacement for Magent 1.9.

Get all fixes. Get all features. Nothing breaking.

When we add everythin to v20 and only port security-related issues back, why should i change from existing M1 to OM, instead of make a bigger step and head over to M2?

@sreichel
Copy link
Collaborator

@fballiano real-case scenario ... you run a large shop with dozens of extensiond based on M1 or OM19 and we do not add improvements anymore ... options?

  • stay on v19: you'll get only sec-related fixes
  • switch to v20: to stay with my type hints example ... you possibly have to adjust a view extensions that are not maintained anymore

In 2nd case i'd really think about to switch to M2.

@Flyingmana
Copy link
Contributor

I agree with v20 being default, when we completly ignore BC breaks. I'd like to add typhints everywhere 😎 ...

All of the suggested Ideas ( in this rfc https://github.com/OpenMage/rfcs/blob/9d5c830fe01238b4f199745afc28037e7740b29b/draft/0000-release-schedule.md ) includes having regular new Major versions, which do allow all kind of BC breaks.

@fballiano
Copy link
Contributor Author

fballiano commented Dec 31, 2022

IMHO ... not by wording, but in case of OpenMage "LTS" was always meant to be "drop-in" replacement for Magent 1.9.

Get all fixes. Get all features. Nothing breaking.

ok, I can live with not trying to break things, but having to manage conflicts because we don't want to drop IE8 in 2022 is not worth it IMHO.

What I don't want to keep doing is having these huge "merge" PR that are not well tested, with a lot of time used to fix conflicts (generating potential problems), because we don't have enough manpower to manage these 2 branches and live happily :-)

Most of the differences between v20 and v19 could be backported with not many problems and we wouldn't even need a v20.

I'm saying that in a positive way, not polemic in any way.

In 2nd case i'd really think about to switch to M2.

I see and it is true, but:

  • php is moving super fast lately, the stores will have to patch things up anyway to upgrade php to 8.0, 8.1 etc
  • if they're not willing to do (which is fine) I think they wouldn't even update the core
  • sadly everybody I know will replatform some day, but we may keep the interest on OM if we have a strong identity and we don't live forever in the shadow of M1, which is our great foundation, but shouldn't be the anchor that drags us to ground forever (I know, it's a really hard situation to manage). people should want to stay with OM because it's a good platform and it's truly community based and they share our vision/ideals :-)

@Flyingmana
Copy link
Contributor

What I don't want to keep doing is having these huge "merge" PR that are not well tested, with a lot of time used to fix conflicts (generating potential problems), because we don't have enough manpower to manage these 2 branches and live happily :-)

its okay, I can do the merges again from now on, if every 2-4 weeks is good enough

@ADDISON74
Copy link
Collaborator

This is an off-topic post, in relation to the previously expressed opinions.

Magento/OM users must be divided into fully or partially dependent of developers and independent developers. My opinion is that an independent user will be interested in continuing with OM because he already has the necessary knowledge to manage his projects. Even a partial one can do it. Only the fully dependent will pay anyway to have a functional solution, it doesn't matter on which platform it will be M1 or M2.

If I were in the partial dependent category, it would suit me not to make too many changes, especially in extensions, but we all know that some mandatory changes come from the programs that make the platform functional. We cannot continue for long using OS's like CentOS 6, Debian 8 and we are forced to adapt to the new versions of PHP, MySQL, Apache/Nginx. I think that these are the ones who appreciate an LTS version that does not create issues for them. If I am from the category of users independent of the development I would like OM to have an air of freshness and to be brought up to date as they say, all waste to be removed.

I would prefer that starting with 2023 the one we call OM 20 be the main branch (called main) and the one we call OM 19 become the branch we touch as little as possible (called lts). when necessary, certain PRs from the hand can be easily transferred to the lts. Thus we ensure the best possible BC and offer stability (as much as the programs will allow). At the same time, if the extensions of these installations are brought up to date, they will be able to benefit from the OM solution for a long time from now.

In conclusion, it is time for a main and lts branches, to break away from certain patterns that we have inherited over the years such as version 1.9.4.x version 20.0. were used at a time when Magento existed, now it is only OpenMage.

@ADDISON74
Copy link
Collaborator

@fballiano said a phrase above that should be remembered. Although OM is built on the foundation of Magento, the time has come to break away from Magento by promoting OM as a platform that ensures compatibility for those who want to keep their installations unchanged and that brings novelty, an alternative to all other existing Magento 2, Prestashop, OpenCart. Woocart. I thought so and that's why I frequent this repository precisely because I see OpenMage as a viable solution for now and the future. I've been here for about 3 years, but I don't see many of the people I was used to. Probably because they no longer give OpenMage a chance, but if they made the decision based on this reason, it is a wrong one. This year I talked with several extension developers for M1 and I explained to them that OM continues Magento and is not a tired platform, on the contrary, it will be a modern one. There are already about five who have agreed to continue selling extensions without support and one who is willing to offer support to those who buy. I remember what a revolution it was to make Magento 1.9.2.4 compatible with PHP 7.0, Inchoo coming up with a solution at that time. Well, OM has done much more than that and the changes are visible. When the code will be completely cleaned, when we will state that we are fully compatible with PHP 8.2, small issues and PRs, I wonder why I would change something that is functional with something that requires important resources (migration, extensions, in fact time and money) like Magento 2?

@fballiano
Copy link
Contributor Author

its okay, I can do the merges again from now on, if every 2-4 weeks is good enough

there's no need, there are always opened PR that need reviews that nobody wants to do, I document every conflict so you can help there.

Copy link
Member

@elidrissidev elidrissidev left a comment

Choose a reason for hiding this comment

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

Looks good. No usages found.

@elidrissidev elidrissidev merged commit e7cd88b into OpenMage:1.9.4.x Jan 10, 2023
@fballiano fballiano deleted the remove_unused_autoloader branch January 10, 2023 19:55
fballiano added a commit that referenced this pull request Jan 10, 2023
@fballiano
Copy link
Contributor Author

cherry-picked to v20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: lib/Mage Relates to lib/Mage Component: lib/Magento Relates to lib/Magento Component: lib/* Relates to lib/*
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants