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

Refs #3260975 replaces Deprecated call to drupal_set_message #689

Merged
merged 2 commits into from
Jan 30, 2022

Conversation

stesi561
Copy link
Collaborator

Overview

Replace deprecated call to drupal_set_message - as per https://www.drupal.org/project/webform_civicrm/issues/3260975

Before

As the function is now removed from 9.x fatal error occurs instead of displaying an error to the user.

After

Error message is passed through to the user.

@stesi561
Copy link
Collaborator Author

I guess ideally this would include test coverage. If @KarinG can point me in the right direction as to where this might be best added happy to work on this.

@KarinG
Copy link
Collaborator

KarinG commented Jan 27, 2022

Hi Luke! I've kicked off the tests. Thank you for catching this.

Re: tests -> we have live/functional tests for Dummy, iATS Payments, Pay later and Stripe (see tests/src/FunctionalJavascript/ dir). If you wanted to add a e.g. PayPal test (or another payment processor that uses IPNs) - start with copying one of the existing Payment Processor test .php files and then you can add a success scenario and a fail scenario. I'm happy to connect with you over GoTo or Zoom as well if you like - to help you get going.

However for this fix - since it's not data critical - I'm happy to merge it once it passes existing tests.

@KarinG
Copy link
Collaborator

KarinG commented Jan 27, 2022

Oh I think that's a real error in CiviCRM dev-master. I'll dig in and pull out the browser artifacts.

@KarinG
Copy link
Collaborator

KarinG commented Jan 27, 2022

image

image

Downloading Artifacts -> phpunit_browser_output

image

@KarinG
Copy link
Collaborator

KarinG commented Jan 27, 2022

This is the iATS portal ->

image

@stesi561
Copy link
Collaborator Author

I see the same error on all PRs in the last 5 days. Seems like it was working 8 days ago.

@KarinG
Copy link
Collaborator

KarinG commented Jan 27, 2022

I don't see any Failed Payments on the iATS side of things for dev-master (and the others all Settled).

@KarinG
Copy link
Collaborator

KarinG commented Jan 27, 2022

Yes - something has gone sideways in CiviCRM dev-master.
RC is still good 👍

@KarinG
Copy link
Collaborator

KarinG commented Jan 27, 2022

I've posted on Mattermost -> Product-Maintenance channel -> sometimes Seamus will know exactly which PR may have caused an issue :-) https://chat.civicrm.org/civicrm/pl/esi8wyr7hbr8pkier88k99emdc

@KarinG
Copy link
Collaborator

KarinG commented Jan 28, 2022

Ok reproduced by upgrading one of my live sites to dev-master ->

image

[validationFailures] => Array
    (
        [0] => Array
            (
                [key] => ownerStreet
                [message] => Street is required.
            )

        [1] => Array
            (
                [key] => ownerCity
                [message] => City is required.
            )

        [2] => Array
            (
                [key] => ownerZip
                [message] => Postal Code is required.
            )

    )

@KarinG
Copy link
Collaborator

KarinG commented Jan 28, 2022

Ok tracked it down to this PR in civicrm core -> it touches the billing fields -> civicrm/civicrm-core#21583

Reverting that PR -> all is well again. Talking to Dave about how to solve this issue in civicrm core.

@mattwire
Copy link
Collaborator

@KarinG I think this might be fixed by #691

drupal_set_message(ts('Payment approval failed with message: ') . $e->getMessage(),'error');
\Drupal::messenger()->addError(ts('Payment approval failed with message: %error ', [
'%error' => $e->getMessage(),
]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops! Fixed.

@KarinG
Copy link
Collaborator

KarinG commented Jan 29, 2022

Thanks for editing Luke. Re-running this against D9 (5.35 - RC).

@KarinG KarinG merged commit afe6d7a into colemanw:6.x Jan 30, 2022
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