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

Feature for turning off template path requirement #2722

Closed
wants to merge 2 commits into from

Conversation

hcldan
Copy link

@hcldan hcldan commented Feb 6, 2024

Fixes #1792

@SergioBenitez
Copy link
Member

Cargo features cannot be negative because they must be additive. A correct implementation would be to introduce a positive with_filesystem feature (or similar) that is purely additive.

Still, I'm not sure the general notion behind this PR makes sense. If you remove the discovery aspect of the dyn_templates crate, all that's left is the managing of the template instance, and a responder that knows how to use said instance. Thus, if this general functionality is desired, I would suggest a PR that instead allows someone to pass in a pre-configured instance of the engine. If the engine is preconfigured, then there's no need to search the FS. And, it doesn't require any feature shenanigans.

@hcldan
Copy link
Author

hcldan commented Feb 6, 2024

I agree the feature flag is probably not the best way to do this... but I'm new to this crate.

I would suggest a PR that instead allows someone to pass in a pre-configured instance of the engine.

I'm not sure that's enough. The Context struct has watchers for the path that it attaches and that's agnostic of the engine.

@SergioBenitez
Copy link
Member

I'm not sure that's enough. The Context struct has watchers for the path that it attaches and that's agnostic of the engine.

I don't mean to suggest that's all the work that needs to be done, but rather that is the seed of the work. What falls from that decision needs to be handled.

@hcldan
Copy link
Author

hcldan commented Feb 7, 2024

I'm starting to wonder if the better course of action here is to just rip out templating from rocket completely.

Is there any special benefit beyond something like:

use std::include_str;

use rocket::log::error_;
use rocket::http::{ContentType, Status};
use rocket::request::Request;
use rocket::response::{Responder, Result};
use rocket::Response;
use tera::{Context, Tera};

// This function may be wonky, but is only for illustration purposes of the code below it.
fn rocket() -> Rocket {
    rocket::custom(rocket_conf)
    .mount("/", routes![root_path_redirect, login])
    .manage(engine()?)
}

pub struct Login(Context);
impl<'r> Responder<'r, 'r> for Login {
    fn respond_to(self, request: &'r Request<'_>) -> Result<'r> {
        let engine = match request.rocket().state::<Tera>() {
            Some(e) => e,
            None => { // should never happen, see `engine() below`
                error_!("Template engine not initialized!");
                return Result::Err(Status::InternalServerError)
            }
        };

        let rendered = match engine.render("login.html", &self.0) {
            Ok(html) => html,
            Err(err) => {
                error_!("Template render failed: {err:?}");
                return Result::Err(Status::InternalServerError)
            },
        };

        Response::build_from(rendered.respond_to(request)?)
        .header(ContentType::HTML)
        .ok()
    }
}

pub fn engine() -> anyhow::Result<Tera> {
    let mut engine = Tera::default();
    engine.add_raw_template("login.html", include_str!("../../templates/login.html"))?;

    Ok(engine)
}

#[get("/login")]
pub fn login() -> Login {
    let mut context = Context::new();
    // TODO: Fill the context
    Login(context)
}

I'm not sure if it's a bad idea or not to shove the engine in the State. It appears to work.
I feel like I have way more control over if I want to config the engine with a directory or not.
And I also feel like I probably wouldn't be using multiple template engines.

@hcldan
Copy link
Author

hcldan commented Feb 7, 2024

Sure, I could generalize it to be kinda like your Template struct with a responder...
But that's pretty simple. All the other stuff inside of rocket seems to introduce behavior that I want to avoid.

Maybe it would be better to just show people they can "Bring Your Own Engine" like the above.

@SergioBenitez
Copy link
Member

No, that's totally valid. Rocket's templating support brings quite a few things to the table:

  • A responder with a sentinel, so you can't forget to configure the templating engine.
  • Discovery of templates in the file system with support for multiple engines simultaneously.
  • Auto-reloading of templates in debug mode.
  • Correct and safe handling of template escaping.
  • Automatic content type handling based of file extensions.
  • Engine agnostic rendering interface, including a context! macro.
  • Great formatting for error messages from templating engines.

Perhaps some other things? In short, it's a production-ready, safe, secure, and reliable version of what you've written above.

@hcldan
Copy link
Author

hcldan commented Feb 7, 2024

I'll close this PR then, as most of my needs are met with the above minimal code.

Correct and safe handling of template escaping.

Can you say more about this? Shouldn't the engine be doing that?

@hcldan hcldan closed this Feb 7, 2024
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.

Template::custom but without configured template directory
2 participants