Magento-lts: General Project Questions

[colinmollenhour]

Sorry if this is not the appropriate place for this discussion, just let me know if I should move it elsewhere. I just have some questions about the project:

Is there any collaboration with Magento, Inc to get these fixes merged in the official Magento? I assume the lifetime of M1 is nearing it's end but it seems like a good opportunity to improve the original product since you are essentially doing what they say they don't have time to do (manage issues and pull requests).

Where do you draw the line between an appropriate PR and an inappropriate one? Examples:

• I determined that storing the admin ACL inside each session is a terrible design and it is better to cache the ACL and just store the admin user_id in the session. This results in huge session storage reductions and fixes some issues with ACL not being reloaded when it should. I can implement it so that existing sessions are not broken and will be gracefully updated and the public/protected class interface is 100% compatible so there is 0 impact to users but it is a somewhat significant change otherwise.
• Similarly space in sessions can be saved by moving the validator data to it's own key and not duplicating it for every namespace.
• Vardoc fixes/improvements that make code completion work with PhpStorm much much better (e.g. using @return $this, fixing incorrect vardocs, adding @method annotations like @method Mage_Catalog_Model_Product[] getIterator(), etc).

Eh, I forgot all of the fixes and improvements I've made offhand but in general what are the rules for accepting/rejecting?

Thanks for your contributions!

[Flyingmana]

Thats a very good question, and I think its ok to ask this here.
So first, there is absolutely no collaboration with Magento, Inc here (which I would know about), and I think they cant simply take our merged PRs because of the missing CLA Step for it.
Also, even if we try our best, our QA process is more then bad compared to what they have for internal requirements.

I would say, we draw the Line on a per Issue base. If the required change is bigger or deeper in the core, we are a lot more strict about it.

The most important part about accepting changes is, No BC breaks
If it destroys a feature as little it may be, it will not be merged. Security fixes can be an exception for this.

Doc Fixes are ok I think, would need to check with others about this. I would just require to have them only in small changesets, not inside a patch which affects 100 files. Even 10 are to much for my taste here. Can make merging annoying.

Enhancing or changing behaviour is a harder question. Primary the LTS is about fixing Bugs, not improving the product.
Interestingly your example would hit the security part in my opinion and would so have a rather good chance of merging.

so simply said, we take the fixes, but maybe not the improvements. And I may be wrong with parts, I just wrote this down from mind.
Maybe some of the others will add some comments to later.

[beejhuff]

Daniel, I liked this answer. Now that's I'm working with @drobinson at AOE he's been walking me through the project and I've been checking out the open PR's when they come and and watching issues get worked in the queue...

Magento's extended the support date for M1 through 2018 (I think...) but my guess is that there's going to be a large group of customers who still won't be ready to migrate by then...have you thought about how the approach to PR's might evolve as we get closer to official EOL?

I agree 100% about the parts of Colin's PR's that would be close to security improvements (ie the session handling suggestions), even if not STRICTLY fixing a bug or vulnerability, are certainly about increasing correctness of design and reducing surface area for attack or unintended errors, even if the original motivator was pure performance .

The vardoc suggestion is clearly not, but I would argue as we get closer to EOL - those are particularly the kind of improvements we may need to start adding to make it more efficient and easier to maintain this for as long as we need to do that.

As part of my effort to thank David for all the help he's given bringing me up to speed here, I'm go to try and help out however I can on the repo. The kinds of improvements Colin mentioned that improve productivity in the IDE may be the kind of things we can use to make it a little easier to contribute which is nice for us newbs 💯

Side note - how are we handling regression testing to ensure bug-fixes don't have any unintended side-effects? Is it manual review by @Flyingmana @LeeSaferite or David?

[colinmollenhour]

Here's an example: https://gist.github.com/colinmollenhour/14ed23d029e5b991a3e31ad1fe0841d4

This is completely BC, but it changes the way the layout cache is stored. Eligible for a PR?

[LeeSaferite]

To me, changes like this are exactly the kind of patches we want. It doesn't break the existing interface at all and internally improves the code. The patches that we won't take are the ones that are BC breaking. Adding features is on a case by case, but I'm inclined to say we should be very careful about those.

This project could really use help in all facets. I think that M1, with all its warts, is a decent base that just needs some TLC. If we can get the project set up to run the official test suite and start adding tests for all of these patches, I think we can remove some of those warts.

I hold the OpenMage domain and GitHub account, but I'd be glad to turn it over to a community that will maintain the project. My work has kept me so very busy of late that I have been horrible at responding to tickets. I think a strong community willing to maintain the project is needed if we want to get anywhere.

Sorry if this is rambling. I'm on my phone between working and trying to dump some of thoughts.

[drobinson]

Agreed - especially since it's been almost 6 months since Magento has released anything at all pertaining to 1.x this project could become a much needed community driven fork. Before we'd discussed having some sort of process in place to ensure we continued giving this project the attention it deserves, but haven't had any action on that. Do you think we need a more defined process, or do we simply need more people involved?

[tmotyl]

+1

[Flyingmana]

definitely more people. Its always what is needed for an opensource project. A process helps preventing Fails, but does not bring progress.

So, we definitely need more People.
THX to the new github Feature of https://help.github.com/articles/about-pull-request-reviews/ PR Reviews, they can directly help. Every PR with at least 2 positive Non-Team reviews lowers the burden for us and we are enabled to process PRs faster.

[drobinson]

@colinmollenhour I think your question has been answered here, OK with closing?