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

Implement a Token Provider interface #3820

Merged
merged 8 commits into from
Jul 7, 2020

Conversation

bogdan-litescu
Copy link
Contributor

@bogdan-litescu bogdan-litescu commented Jun 7, 2020

#3819.

The token replacement code is very exposed through public and protected members. My implementation leaves all of those in place to avoid breaking existing functionality. Therefore, all the core tokenization code still lives in the BaseCustomTokenReplace, BaseTokenReplace and TokenReplace classes.

If the provider is different than the CoreTokenProvider, it calls that provider instead. The CoreTokenProvider also has a simple implementation that calls the TokenReplace class. This is in case someone decides to use the provider instead of calling TokenReplace directly.

The changes I did also consolidate all parameters related to tokenization into a single TokenContext class that can be easily passed around.

Tested on DNN 9.6.1 with some simple scenarios and everything seems to work as before.

@dnfadmin
Copy link

dnfadmin commented Jun 7, 2020

CLA assistant check
All CLA requirements met.

@sleupold
Copy link
Contributor

sleupold commented Jun 7, 2020

did you test with modules implementing custom TR, like "DNN Form & List"?

@bogdan-litescu
Copy link
Contributor Author

did you test with modules implementing custom TR, like "DNN Form & List"?

I haven't tested Form and List yet. I tested the HTML module, which also uses a derived TR HtmlTokenReplace and the skin tokens, such as the language flags.

@sleupold
Copy link
Contributor

sleupold commented Jun 7, 2020

tokenReplace was originally developed as part of Form&List module, which still uses it more extensively than HTML, Skin or Newsletter module.

@bogdan-litescu
Copy link
Contributor Author

tokenReplace was originally developed as part of Form&List module, which still uses it more extensively than HTML, Skin or Newsletter module.

Thanks for the info! I've detected an issue and made adjustments.
Works beautifully now with Form and List as well.

@bdukes bdukes added this to the 9.7.0 milestone Jun 11, 2020
@bdukes
Copy link
Contributor

bdukes commented Jun 11, 2020

@bogdan-litescu this contribution looks great! There are some failing tests, though. I'm assuming you'll need to adjust how some things are created in the tests, maybe by creating mock implementations. Feel free to ask questions if you have trouble tracking the issues down. Thanks!

@bogdan-litescu
Copy link
Contributor Author

@bogdan-litescu this contribution looks great! There are some failing tests, though. I'm assuming you'll need to adjust how some things are created in the tests, maybe by creating mock implementations. Feel free to ask questions if you have trouble tracking the issues down. Thanks!

@bdukes Thanks for pointing this out. I registered the CoreTokenProvider in the ComponentFactory and that seems to do the trick.

@valadas valadas assigned valadas and unassigned valadas Jun 23, 2020
Copy link
Contributor

@valadas valadas left a comment

Choose a reason for hiding this comment

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

Looks good to me

@valadas valadas merged commit 58b46c1 into dnnsoftware:develop Jul 7, 2020
david-poindexter pushed a commit that referenced this pull request Jul 11, 2020
* Fix test config

* Add missing solution files

* Add <tokens> provider section for upgrades
@valadas
Copy link
Contributor

valadas commented Oct 13, 2021

It would be very nice to see some documentation or an example usage somewhere... I tried to use this and got into StackOverflow errors, not sure why...

@david-poindexter
Copy link
Contributor

It would be very nice to see some documentation or an example usage somewhere... I tried to use this and got into StackOverflow errors, not sure why...

Agreed @valadas

@bogdan-litescu is that something you'd be able to do?

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.

6 participants