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

[10.x] Markdown engine #49455

Closed
wants to merge 11 commits into from
Closed

[10.x] Markdown engine #49455

wants to merge 11 commits into from

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Dec 20, 2023

This introduces a new view engine for rendering markdown files. It allows you to add .md files to your views/ directory and render them like any other view:

// given a `resources/views/terms/privacy.md` file, this will work:
return view('terms.privacy');

If you want to wrap markdown in a layout or manipulate it in some way, you can register a callback:

View::getEngineResolver()
  ->resolve('markdown')
  ->renderMarkdownUsing(function($markdown) {
    return Blade::render('<x-layout>{{ $markdown }}</x-layout>', compact('markdown'));
  });

(This callback receives the rendered HTML, the CommonMark RenderedContentInterface instance, the data passed to the view, and the path of the markdown file being rendered.)

Personally, I would love to add a renderMarkdownUsing method to the View\Factory that handled the engine resolution for you, but I wanted to keep this PR as focussed as possible.

By default, the markdown engine will use the GithubFlavoredMarkdownConverter (which is the default in existing Laravel markdown implementations), but you can bind any League\CommonMark\ConverterInterface interface to the container to make that the default:

// in your service provider's `register` method:
$this->app->bind(ConverterInterface::class, function() {
  return new CommonMarkConverter();
});

@GrahamCampbell
Copy link
Member

This is a breaking change to put into Laravel 10, and indeed breaks my package which registered a markdown renderer.

@inxilpro
Copy link
Contributor Author

and indeed breaks my package which registered a markdown renderer

Hm, I just looked at your package and won’t your engine registration just override the default? I’m on my phone so I can’t really check, but it sure seems like your package would just replace this…

@driesvints
Copy link
Member

@GrahamCampbell this isn't a breaking change. But if it breaks your package we could maybe re-consider it for Laravel v11. Let's see what @taylorotwell says.

@inxilpro
Copy link
Contributor Author

if it breaks your package we could maybe re-consider it for Laravel v11

FWIW I just created a new Laravel project, swapped in this fork, installed graham-campbell/markdown, and everything was fine. I was able to adjust the markdown.extensions config and see those changes reflected in a test view without any issues (so it was clearly using the package's implementation over the framework's). I'm curious what breaks, specifically… I can't find anything.

@taylorotwell
Copy link
Member

I'm not sure I want to take on any opinions on what a Markdown view engine should look like. I would rather leave it to packages. For example, I could imagine people might want to use Blade syntax within Markdown to potentially echo variables or use control structures, which could easily be done now via something like:

return Str::markdown((string) view('some.view-that-contains-markdown', $data));

@inxilpro
Copy link
Contributor Author

I'm not sure I want to take on any opinions on what a Markdown view engine should look like.

@taylorotwell I get it. That said, my one take would be: this was intentionally implemented very minimally and with zero opinions. In all of the recent Laravel projects I've worked on, I've needed markdown views. I know the Laravel docs implement this. I know the Verbs and Livewire docs also both implement this. It feels to me that a simple, but extensible markdown view engine would be great for many people.

I also definitely planned on PR'ing .blade.md support, which I absolutely think people would want to reach for. But I wanted to leave that for a second pass…

@GrahamCampbell
Copy link
Member

I think it's best left outside the framework core tbh. May people have opinionated ways to do this.

@GrahamCampbell
Copy link
Member

.blade.md already exists within my package, btw. ;)

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.

4 participants