-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Blade #1765
Conversation
Wouldn't this fit well as a soil module? And then sage or other themes can opt in to use blade templating (or timber which I'm using). |
I also like the idea of it being a soil module which allows for a user to choose between Timber or Blade. |
Since you are reconstructing the templates array, could it be made available elsewhere so for instance a header or sidebar could be retrieved based on the first match on the template hierarchy? |
this won't be a soil module, partly because soil is a paid plugin and partly because we don't think it fits in there @kalenjohnson has suggested we create a composer package for it within this repo, similar to how laravel does in all of these subfolders |
|
||
$template_name = basename(str_replace('.blade.php', '', $theme_template)); | ||
$html = $template_engine->make('partials/'.$template_name, []); | ||
var_dump($template_engine->exists('partials/'.$template_name)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops. ;)
return $template; | ||
} | ||
|
||
$container = Container::getInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Container
class used should probably be in the Roots namespace, and just extend the Illuminate Container, as to not conflict with some other code using the same Container class.
I think this would be sufficient..
<?php
namespace Roots\Sage;
use Illuminate\Container\Container as BaseContainer;
class Container extends BaseContainer {}
This would also only require changing the import to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can understand the reasoning on this, making sure everything that's used is in the same namespace. However, I was also considering that really, the Container class should be global and only one instance of, just like in a Laravel application. The IoC container is something that can be used anywhere and give you instances of anything you need.
Basically, I don't know if we necessarily want to make that Roots specific. I don't think it would necessarily be bad if other plugins called Illuminate\Container\Container
directly. On the other hand, perhaps we do move it out to it's own project along with the Blade service provider, and also add some things that are used in the global scope of WP, like $wpdb
, and add that to the container, it's a container that could be used across plugins and themes in general.
Also added It uses |
Thanks so much for your work on this! What do you think about creating some custom directives for the typical use cases such as https://laravel.com/docs/5.3/blade#extending-blade Some bad ones as an example, https://github.com/tormjens/wp-blade/blob/master/src/Blade.php#L321 |
Can't wait to try this out! Would it be possible to allow injecting data into the views? I see that the template method accepts data but it doesn't appear to be used in the render. Perhaps a filter could be used. |
Awesome work! I just love the blade engine and can't wait to start building stuff :) Did a quick test run and it seems like the Yes, I did make changes to the custom template. |
}); | ||
}, [ | ||
'index', '404', 'archive', 'author', 'category', 'tag', 'taxonomy', 'date', 'home', | ||
'front_page', 'page', 'paged', 'search', 'single', 'singular', 'attachment' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
front_page
should be corrected to frontpage
https://github.com/WordPress/WordPress/blob/4.7-branch/wp-includes/template.php#L29-L41
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can submit PR if you want credit for it. Otherwise I'll handle it later.
Blade templating. This PR is going to be dependent on WP 4.7, specifically this section of code. If you'd like to test, either use WP 4.7 or grab this code and add it to your installation. If you're using Bedrock, you can set the version requirement of
johnpbloch/wordpress
to4.7.x-dev
.Currently,
.blade.php
files are used, and they are being added to the list of files to load via the previously mentioned new hook.For plugins, the same issues I believe will apply that have been in Sage for the last few versions - plugins are able to load their own templates, but the same issues with calls to
get_header()
andget_footer()
will still show deprecation warnings. You'll want to add a Blade template according to the WP template you need to override.Looking for feedback and opinions on the current setup.
Ref #1699