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

CRM-21445 ensure pay_later processor is set so we do not get a fatal … #11427

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Under circumstances described in CRM-21445 & https://issues.civicrm.org/jira/browse/CRM-21436?focusedCommentId=111107&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-111107 the form is treated as 'monetary' but has no processor assigned when it tries to load the payment form fields. This ensures that the manual processor is loaded

Before

Fatal per above

After

Fatal gone

Technical Details

For reliability we should assign the Manual processor whenever building a form

Comments

@eileenmcnaughton
Copy link
Contributor Author

@mattwire - proposed replacement for #11292

@eileenmcnaughton eileenmcnaughton changed the base branch from master to 4.7.29-rc December 19, 2017 03:01
@eileenmcnaughton
Copy link
Contributor Author

This was discussed for the rc on the dev-post release channel & it was agreed to merge it there. I thought it had been merged but I guess not so merging now

@eileenmcnaughton eileenmcnaughton merged commit f1506d9 into civicrm:4.7.29-rc Dec 20, 2017
@eileenmcnaughton eileenmcnaughton deleted the no_money_honey branch December 20, 2017 20:40
@yashodha
Copy link
Contributor

@colemanw
Copy link
Member

@yashodha can you give more details about why? How did you trace the cause of the error back to this PR?

@eileenmcnaughton
Copy link
Contributor Author

@yashodha @colemanw @totten yes - this solved one problem that some people were experiencing as critical but one that will affect a lot more people. We should try to get this available in the main download - e.g reverse it & recut 4.7.29 & replace. I took a look & there is a good reason we are experiencing 'whack a mole' around this bit of code & when I am back on line in mid to late Jan we should look at this.

I was nervous about this one when we discussed - mostly because I spent so much time on the digits + locale issue I didn't get enough to work through this properly. Also, because I wanted it to have longer in core to see if configs other than mine caused problems (which they did & which I can now replicate) but OTOH it seemed to be dealing with a serious issue. We got the worst of both worlds. There was agreement to merge but it it wasn't merged so it missed out on a few of the available precious days giving it only 2 in the end :-(. And then since the issue was not identified until after it is too late for a Feb fix (I won't be able to look at it in timeframe for it) I think it's going to be March now.

Still - I think the downloadable version should exclude this patch if at all possiible.

@eileenmcnaughton
Copy link
Contributor Author

Actually I think we can quash the original fatal too. This PR reverts this & also adds the other. I've put it against 4.7.29 since I think replacing 4.7.29 with a version without this bug is the only thing that can reasonably be done ASAP

#11454

I'm sorry about this error. I think I was in a bit a state after spending a day under pressure fixing the currency locale bug so (which was more even serious than this) and it was a bad decision to try to include this one for the rc given the limited resources available and the limited time I had after losing a day. The merge confusion made it worse.

Note that I will be unavailable until mid to late January so don't hold up any redress for this on response from me.

sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21445 ensure pay_later processor is set so we do not get a fatal …
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants