adds new style token processing

Project60 / org.project60.sepa

SEPA direct debit integration with civicrm

19 stars 46 forks source link

adds new style token processing #677

Closed AlainBenbassat closed 6 months ago

AlainBenbassat commented 11 months ago

I noticed that the Most_Recent_SEPA_Mandate tokens are not replaced anymore in version 5.58.1, because the hooks hook_civicrm_tokens and hook_civicrm_tokenValues are deprecated.

This PR adds the new of processing tokens, while retaining the legacy way.

See also: https://docs.civicrm.org/dev/en/latest/framework/token/#defining-tokens for more information.

My attempt in this PR is to:

only touch the token code
make new token processing and legacy processing as similar as possible
convert array to [] in the files I changed
move as much code out of sepa.php
- 3 methods were added to the BAO SEPAMandate
- a new helper class SepaTokens was added for the new way of handling tokens
- a new helper class SepaTokensDeprecated handles the legacy token processing (it has the same methods as SepaTokens)

bjendres commented 11 months ago

@jensschuppe requested your review on: https://github.com/Project60/org.project60.sepa/pull/677 adds new style token processing.

@jensschuppe I'm currently pretty swamped, any chance you could do that?

jensschuppe commented 11 months ago

@jensschuppe requested your review on: #677 adds new style token processing.

@jensschuppe I'm currently pretty swamped, any chance you could do that?

I'm quite in the same swamp, actually - sorry!

Anyone feel free to review/test!

I think the tests failed because of lab.civicrm.org being down yesterday. Could you start a re-test, @bjendres?

bjendres commented 11 months ago

Could you start a re-test, @bjendres?

Unfortunately it still fails, see https://app.circleci.com/pipelines/github/Project60/org.project60.sepa/373

I get a lot of these errors

Error: Class 'SepaTokensDeprecated' not found

@AlainBenbassat could you have another look?

AlainBenbassat commented 11 months ago

@bjendres @jensschuppe I fixed a bug. Automatic tests pass now. However, functional testing is still required to make sure everything works fine, both with the old way of processing tokens as well as the new method.

bjendres commented 11 months ago

However, functional testing is still required to make sure everything works fine, both with the old way of processing tokens as well as the new method.

Good to know, thanks. Let me know when you're done.

kainuk commented 7 months ago

Hi @AlainBenbassat, this PR would be very for one of my customers. I had to make one change to make it work for my use case. Also, I think the old-skool token processing is not needed any more because this extension is supported from version 5.43. The new token processing was introduced in 5.28. If you grant me access to the branch I can add my change, remove old-skool token processing and see it holds up in the test sets.

kainuk commented 6 months ago

I have created a new PR with the code of @AlainBenbassat and the suggestion of @bjendres . You can find it here https://github.com/Project60/org.project60.sepa/pull/693

kainuk commented 6 months ago

I have tested the PR on version 5.43. The old-skool triggers are needed for this version. I have also tested it on 5.65.1.

AlainBenbassat commented 6 months ago

I will close this PR as Kainuk's pull request includes my changes.