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

(NFC) Enhance \Civi\Token\AbstractTokenSubscriber::prefetch() #13222

Closed
wants to merge 1 commit into from

Conversation

aydun
Copy link
Contributor

@aydun aydun commented Dec 4, 2018

Enhance \Civi\Token\AbstractTokenSubscriber::prefetch() to receive list of active tokens

Update CRM_Mailing_Tokens::prefetch() to match

Another extraction from #13174 and #12012

…e list of active tokens

Update CRM_Mailing_Tokens::prefetch() to match
@civibot
Copy link

civibot bot commented Dec 4, 2018

(Standard links)

@civibot civibot bot added the master label Dec 4, 2018
@eileenmcnaughton
Copy link
Contributor

I agree this is the only core place where that function is overridden

I guess the only question is whether anything outside core overrides it - since the class is in the Civi dir it could be argued it's a formal interface & we support extending it - the alternative would be for $activeTokens to be a class property & it would be set prior to calling prefetch - which would mean no signature change

@totten do you have your universe tool keyed up to check that?

@seamuslee001
Copy link
Contributor

I have just checked Flexmailer and it doesn't use the prefetch function does implement other things i thin but not prefetch

@@ -72,7 +72,7 @@ public function checkActive(\Civi\Token\TokenProcessor $processor) {
* @return array
* @throws \Exception
*/
public function prefetch(\Civi\Token\Event\TokenValueEvent $e) {
public function prefetch(\Civi\Token\Event\TokenValueEvent $e, $activeTokens) {
Copy link
Member

Choose a reason for hiding this comment

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

Docblock? As someone browsing the code, it'd help to have an example of the data that will be passed in here.

@totten
Copy link
Member

totten commented Dec 5, 2018

I guess the only question is whether anything outside core overrides it (prefetch())

Good question.

@totten do you have your universe tool keyed up to check that?

My copy is little dated but nothing shows up. (I'm half way through updating and haven't found anything relevant in the updates... unless there's no more feedback from me within the hour, you can asume nothing else came up.)

I have just checked Flexmailer and it doesn't use the prefetch function does implement other things i thin but not prefetch

Correct. FlexMailer calls TokenProcessor and asks it to render a message... but otherwise it doesn't care how the tokens are populated or rendered, so it doesn't need to know anything about prefetch().

@aydun
Copy link
Contributor Author

aydun commented Dec 5, 2018

Thanks all. On reflection maybe making it a class property is better. I've made changes and pushed those to #13174 so I'll close this.

@aydun aydun closed this Dec 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants