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

[REF] Move getCustomFieldTokens to the tokens class, annotate more deprecations #21660

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

eileenmcnaughton
Copy link
Contributor

Overview

Move getCustomFieldTokens to the tokens class, annotate more deprecations

This deprecates (quietly) another function on CRM_Utils_Token. It's a pretty small function -
the sort you'd just copy & paste if you wanted to use outside of core but
I didn't add noise.

Before

CRM_Utils_Token being called for a 3 line function, only used in token classes, to get custom tokens

After

Duplicated onto the class, deprecated (still called from the TokenTrait but from a function that is itself a candidate for removal - once we've checked universe. The only class that uses the trait has good test cover)

Technical Details

The only core place that still calls this is the tokenTrait

  • which is 'used' by activity tokens - and I haven't started to think about
    whether that class actually needs the _N_ handling that it uses the tokenTrait
    to possibly support (my recollection is that functionality is not enabled)

Anyway - I'm leaving that question for later because I'm not sure if the token
trait is used outside of core or how our final interface will look

Comments

@civibot
Copy link

civibot bot commented Sep 29, 2021

(Standard links)

@civibot civibot bot added the master label Sep 29, 2021
@eileenmcnaughton
Copy link
Contributor Author

Test fail due to 2 changes being recently merged - all tests failing

CRM_Member_Form_Task_PDFLetterTest::testMembershipTokenReplacementInPDF
Non-conformant hook_civicrm_alterMailParams(..., messageTemplate): Unrecognized keys: schema
Array

@eileenmcnaughton
Copy link
Contributor Author

test this please

@eileenmcnaughton eileenmcnaughton changed the title Move getCustomFieldTokens to the tokens class, annotate more deprecations [REF] Move getCustomFieldTokens to the tokens class, annotate more deprecations Sep 29, 2021
@colemanw
Copy link
Member

colemanw commented Oct 3, 2021

@eileenmcnaughton needs rebase

@eileenmcnaughton
Copy link
Contributor Author

@colemanw rebased - although I think it might be in a race with #21713 to see which one makes the other stale :-)

@colemanw
Copy link
Member

colemanw commented Oct 3, 2021

@eileenmcnaughton checkstyle strikes again

@eileenmcnaughton
Copy link
Contributor Author

@colemanw hopefully this time!

@colemanw
Copy link
Member

colemanw commented Oct 4, 2021

@eileenmcnaughton just a few failures...

Test Result (437 failures / +436)

@eileenmcnaughton
Copy link
Contributor Author

@colemanw ug - would be easier if #21713 were merged but will fix that up in this PR anyway

@eileenmcnaughton eileenmcnaughton force-pushed the dep branch 2 times, most recently from 61d25b2 to c7587de Compare October 7, 2021 03:23
@eileenmcnaughton
Copy link
Contributor Author

@colemanw ok should be easy now since only the trait class calls that function now

This deprecates (quietly) another function on CRM_Utils_Token. It's a pretty small function -
the sort you'd just copy & paste if you wanted to use outside of core but
I didn't add noise. The only core place that calls this is the tokenTrait
- which is 'used' by activity tokens - but only for the handling of _N_
which I think was actually not supposed to have been included in the merge.

Anyway - I'm leaving that question for later because I'm not sure if the token
trait is used outside of core or how our final interface will look
@eileenmcnaughton
Copy link
Contributor Author

@colemanw this time maybe.... (rebased again)

@eileenmcnaughton eileenmcnaughton merged commit 8bfe9b2 into civicrm:master Oct 7, 2021
@eileenmcnaughton eileenmcnaughton deleted the dep branch October 7, 2021 22:01
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.

3 participants