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

Master 1.3.x #1229

Closed
wants to merge 10 commits into from
Closed

Master 1.3.x #1229

wants to merge 10 commits into from

Conversation

Yanual
Copy link

@Yanual Yanual commented Nov 6, 2017

To fixe #23558
Factorization of the following PRs:
#1180
#1178
#986

@dregad
Copy link
Member

dregad commented Nov 6, 2017

@Yanual thanks for the pull request. As mentioned by @atrol, the 1.3 branch is in maintenance mode. IMO his last line is not correct, we only apply security and bug fixes to it, no enhancements or new features.

On a strictly formal note for future patches, please make sure that your commit messages adhere to our guidelines - Update xxxx is useless as it does not give any indication of what the change is about or why it is needed.

Copy link
Member

@dregad dregad left a comment

Choose a reason for hiding this comment

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

Suggest closing without implementation. @vboctor let me know your thoughts.

@Yanual
Copy link
Author

Yanual commented Nov 6, 2017

@dregad
Atrol mentioned: "... Of course, there would still be improvements if a community member sent a pull request to https://github.com/mantisbt/mantisbt ..."

I'm a French, maybe that's why I do not have the finesse of the English language, for you what is the definition of "improvements", for me it's not only "bug fixes and security "

I propose an improvement in performance and not functional progress.

@atrol
Copy link
Member

atrol commented Nov 6, 2017

I'm a French, maybe that's why I do not have the finesse of the English language,

I'm a German, so don't expect a perfect English.

Atrol mentioned: "... Of course, there would still be improvements if a community member sent a pull request to https://github.com/mantisbt/mantisbt ..."

Seems I should have written: there could still be improvements ...

1.3.x is quite stable and any change in 1.3.x could introduce regressions and new bugs.
Changing base technology is always a risk and needs some time to review and test.

Keep in mind that 1.3.x supports older PHP versions, databases and browsers than 2.x.
You can't just reapply the changes from 2.x, but have also to ensure that it works in any 1.3.x supported environment.

Keep also in mind that we did not get similar issues from our user community and that I was not able to reproduce your issue, https://www.mantisbt.org/bugs/view.php?id=23558#c58096

Implementing your changes will not help that much users, e.g. check the download statistics:
https://sourceforge.net/projects/mantisbt/files/mantis-stable/
Quite a lot 2.8.0 downloads, but hardly any 1.3.12, so I am not sure if your changes are worth to risk stability.

There are not that many MantisBT core developers and AFAIK none of them is running 1.3.x in production.
Please understand that we have to focus, and the focus is on latest stable branch, but not the old 1.3.x.

But let's wait also for feedback from @vboctor

@dregad
Copy link
Member

dregad commented Nov 6, 2017

I'm a French, maybe that's why I do not have the finesse of the English language

And I'm Swiss... On a tous nos petits problèmes 😉

@Yanual
Copy link
Author

Yanual commented Nov 6, 2017

I understand your goal of stability.

I was just proposing to make a contribution with modifications already made by other experienced developers in the community. It does not seem to me that this code is subject to an incompatibility with the different version of php.

I gained more than a second with these changes. PHP7 saved me almost half a second.

I can continue to maintain these advances. I just wanted to share its with other users.
It's OK for me, and thank you for your time.

@dregad
Copy link
Member

dregad commented Nov 7, 2017

Thanks for your understanding.

@dregad dregad closed this Nov 7, 2017
@dregad dregad added the rejected label Nov 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants