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

Backport Possible missing break in bin\keychain.php at line 77? to the CMS #6883

Merged
merged 1 commit into from
May 9, 2015
Merged

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented May 2, 2015

This Backports the fix from the framework into the CMS:
joomla-framework/keychain@7e6488d

Thanks goes to @photodude here: #6672

…e CMS

This Backports the fix from the framework into the CMS:
joomla-framework/keychain@7e6488d

Thanks goes to @photodude here: #6672
@photodude
Copy link
Contributor

@zero-24 Thanks for the acknowledgment. Glad to see this fix going into place.

@Kubik-Rubik
Copy link
Member

@zero-24 You PR can not be merged because it produces merge conflicts. Your staging is over 300 commits behind the current state! Please update your fork and the PR against the current state. Thank you!

@brianteeman
Copy link
Contributor

@Kubik-Rubik you need to look at the method you are using to apply this. There is only one file changed in this PR and that file has not been changed for 4 months. The merge issue is due to the way YOU are doing it. There is nothing wrong with the PR or any need for it to be updated

@mbabker
Copy link
Contributor

mbabker commented May 9, 2015

Merge on review (and since it's the same fix applied in the Framework repo)

mbabker added a commit that referenced this pull request May 9, 2015
Backport Possible missing break in bin\keychain.php at line 77? to the CMS
@mbabker mbabker merged commit fabc757 into joomla:staging May 9, 2015
@roland-d
Copy link
Contributor

roland-d commented May 9, 2015

@brianteeman I have tried to merge the same branch and I get the same errors. I am not doing this in any way different than the other PRs. Why should I? Perhaps I am doing it wrong as well and don't know how to do it. There are other ways to merge but we are trying to keep our log clean.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/6883.

@brianteeman
Copy link
Contributor

Well @mbabker managed to do it :)
On 9 May 2015 4:54 pm, "RolandD" notifications@github.com wrote:

@brianteeman https://github.com/brianteeman I have tried to merge the
same branch and I get the same errors. I am not doing this in any way
different than the other PRs. Why should I? Perhaps I am doing it wrong as
well and don't know how to do it. There are other ways to merge but we are

trying to keep our log clean.

This comment was created with the J!Tracker Application
https://github.com/joomla/jissues at issues.joomla.org/joomla-cms/6883
http://issues.joomla.org/tracker/joomla-cms/6883.


Reply to this email directly or view it on GitHub
#6883 (comment).

@zero-24 zero-24 deleted the patch-5 branch May 9, 2015 17:51
@zero-24 zero-24 restored the patch-5 branch May 9, 2015 17:54
@zero-24
Copy link
Contributor Author

zero-24 commented May 9, 2015

@roland-d @Kubik-Rubik hmm i guess the problem is that i have 2 accounts (zero-24 and zero24). One with push access and one without push access 😄

This branche is 300+ commits behind staging
https://github.com/zero-24/joomla-cms/tree/patch-5

But this (is the branche of the patch)
https://github.com/zero24/joomla-cms/tree/patch-5

is ok. Sorry that this is confused.

@zero-24 zero-24 added this to the Joomla! 3.4.2 milestone May 9, 2015
@brianteeman
Copy link
Contributor

That shouldnt make any difference. It is not reasonable to expect all pull
requests to always be up to date with current staging.
On 9 May 2015 18:56, "zero-24" notifications@github.com wrote:

@roland-d https://github.com/roland-d @Kubik-Rubik
https://github.com/Kubik-Rubik hmm i guess the problem is that i have 2
accounts (zero-24 and zero24). One with push access and one without push
access [image: 😄]

This branche is 300+ commits behind staging
https://github.com/zero-24/joomla-cms/tree/patch-5

But this (is the branche of the patch)
https://github.com/zero24/joomla-cms/tree/patch-5

is ok. Sorry that this is confused.


Reply to this email directly or view it on GitHub
#6883 (comment).

@Kubik-Rubik
Copy link
Member

@zero-24 Yes, this was the problem. We used the account that you use actively here... The "zero24" branch works without a problem.
@brianteeman Haha, yes, push the green button! :-D By the way, you can not know the way how we in the PLT do the commits properly (no, not the merge button at GitHub), so your comments are acceptable!

@mbabker
Copy link
Contributor

mbabker commented May 9, 2015

Just remember there are a few people not on PLT who may review/merge patches that should be kept in the loop on policy decisions...

@Kubik-Rubik
Copy link
Member

@mbabker I'm talking about squashing all commits of the PR first in one on your local machine to avoid a mess in the history.

@mbabker
Copy link
Contributor

mbabker commented May 9, 2015

Not always necessary, especially when the PR is only a single commit (like this one), unless you're trying to avoid the "merged PR whatever" commits, then someone decided something and forgot to tell me.

@Kubik-Rubik
Copy link
Member

@mbabker This is something we want to discuss at the PLT summit after JAB and establish a policy rule.

@zero-24 zero-24 deleted the patch-5 branch May 9, 2015 21:41
@brianteeman
Copy link
Contributor

As @mbabker said you need to make sure ALL people with commit rights know
On 9 May 2015 21:49, "Viktor Vogel" notifications@github.com wrote:

@mbabker https://github.com/mbabker This is something we want to
discuss at the PLT summit after JAB and establish a policy rule.


Reply to this email directly or view it on GitHub
#6883 (comment).

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

Successfully merging this pull request may close these issues.

None yet

7 participants