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

lib/Rex/CLI.pm: display a warning when/if Rex::Template is loaded. #1512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

monsieurp
Copy link

@monsieurp monsieurp commented Oct 14, 2021

Fixes #1475.

This PR is an attempt to fix #1475.

Checklist

  • based on top of latest source code
  • changelog entry included: we might want to add one.
  • tests pass in CI: let's see.
  • git history is clean
  • git commit messages are well-written

@monsieurp monsieurp force-pushed the 1475_rex-built-in-template-modules branch from 690a313 to da8538c Compare October 14, 2021 21:59
@monsieurp monsieurp force-pushed the 1475_rex-built-in-template-modules branch from d709f35 to 10f1ecc Compare October 14, 2021 22:20
Copy link
Member

@ferki ferki left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution as well, @monsieurp!

Hmm, quite a few core modules depend on Rex::Template so I'm afraid we can't simply deprecate it like that. I'd say we have to make sure the internal template-processing keeps working as before (and without deprecation warnings if not used directly).

We might need to move the discussion and design part back to the original #1475 issue. As briefly mentioned there, my preferred way would be to see if we could migrate the internal custom logic of Rex::Template to something like Text::Template::Simple (which seems close enough syntax-wise), and then perhaps making Rex::Template::NG to be an alias of Rex::Template (with a deprecation notice upon import).

What do you think about that?

Comment on lines -33 to -34
use Data::Dumper;

Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why Data::Dumper is also removed in this context about Rex::Template::* deprecation?

Copy link
Author

Choose a reason for hiding this comment

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

This module is unused in CLI.pm.

# Check whether Rex/Template.pm
# or Rex/Template/NG.pm modules are loaded. If there are,
# they should be present in the %INC hash.
sub check_template_modules {
Copy link
Member

Choose a reason for hiding this comment

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

We have Rex::deprecated to mark things deprecated in Rex core in a uniform way. That might come in handy here too. Or perhaps during import time of Rex::Template and Rex::Template::NG.

Comment on lines +472 to +477
Rex::Logger::info(
"Please consider migrating to a more modern template engine.", "warn" );
Rex::Logger::info(
"Text::Template::Simple, Template::Toolkit, etc. As usual, CPAN is you friend.",
"warn"
);
Copy link
Member

Choose a reason for hiding this comment

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

What do you think about pointing to Rex::Template::TT as a readily available and recommended external module, and point to the docs about how to set one's own custom templating functions:

Copy link
Author

Choose a reason for hiding this comment

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

I can do that indeed.

lib/Rex/CLI.pm Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate built-in template engines
2 participants