-
Notifications
You must be signed in to change notification settings - Fork 821
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
Abstract template layer into its own dedicated module #11322
Comments
Main question for me is just how limited this is, particularly given that there's a bunch of pjax requests made in the cms that use .ss templates as part of the server rendered HTML, which presumably aren't getting changed any time soon. Using twig as an example and let's pretend there's a working twig renderer for Silverstripe, let say that a 3rd party module uses twig templates, or a website project uses twig templates. Will the CMS still be functional, or is there now suddenly a need for the module / project to override ALL core .ss templates with equivalent .twig templates? Also the basically the same question, this time with one third-party module using .ss templates and another third-party module using twig templates in the same project? If all .ss templates need to be replaced, then while this does clean up the code, it seems like it has nearly zero practical real-world usage |
With this approach, ALL templates for a project must be in the same template syntax. This is as per the description in https://github.com/silverstripeltd/product-issues/issues/875:
If we wanted to, we could do some follow-on work to allow defining different rendering engines for different controllers, or something like that, so |
Yeah isolating the CMS templates away from everything else seems like it should be done as part of this card, it's absolutely crucial for this to have any real world usage So just to be clear ... something like this - https://github.com/silverstripe/silverstripe-elemental-bannerblock/blob/3/templates/SilverStripe/ElementalBannerBlock/Block/BannerBlock.ss - if you have website project running twig, you now CANNOT use this module because the template would be nested on a PageController which uses twig rather than .ss? ... unless you re-implement BannerBlock.twig in your website codebase? |
I would prefer it be a separate card. This is going to be a big change as it is.
If by real world usage you mean someone implementing a different template engine with the abstracted API, I agree. I'm not sure that's a primary driver for this work, though Jackson can confirm either way.
No, you can use the module. You just need to supply a template in the syntax of your choice. Even changing template engine per controller, or however else we want to make that distinction, there will be cases like this where you still have to reimplement some templates. In this particular case you would have to implement a controller specifically for that elemental block and declare that it uses ss templates, assuming you declared that the main elemental controller uses twig. But imagine this block uses an include that is shared across some other things, and you've replaced those other things (and therefore also the include) with twig templates. Now you have a discrepancy where there's two versions of that include. You have to replace both if you make changes and want them in sync. |
I'm thinking we'd probably want some sort of backwards compatible logic where if there's controllers (or just namespaces) that don't have any "config" defined that it would just fallback to using .ss templates With this particular banner block example, it's "nested" in PageController:
If you were running Twig for your site, presumably in this case you'd need to Seems like for a "regular" site, where frontend content is basically just pages (SiteTree) + elemental blocks, that you need to be running purely .ss or twig (or something else), though not a combination? Basically it's breaks a lot of the "plug and play" of installing silverstripe modules? This wouldn't be an issue for an XHR style site (e.g. statically generated site) where little bits of content comes in from individual controller endpoints Do I have this correct? |
Regardless of how we do that, that's a separate card that would follow this one. There's a lot to decide about how we'd do that, including deciding if it even would be per controller or a simple CMS vs front-end separation, or some other way to separate it. Or frankly if we want to do it at all. This card is just about the initial abstraction. We can worry about follow-on work in follow-on cards. |
I understand the benefits of doing the work, but those are, obviously, benefits of doing the work. Are there real world problems that we need to or must solve here? Could the linked issues this would supposedly solve be addressed differently, without this work? I'm definitely not against progressing the CMS in these areas, however I still somehow feel like we are pulling random big impact changes out of a hat without a longterm strategy. Maybe there is one, maybe it just wasn't shared, not sure. Do we have a vision or a roadmap where we want the CMS to be in 5 years? E.g. full React, no graphql, modularised, fully decoupled, replaceable components using Symfony components etc.. Some goal, target, which we can relate all the work to, with some purpose and meaning. Maybe this is not the best issue to be raising this here (not an RFC), but it felt related. |
👍🏻 this scope sounds great to me |
This is part of a wider overall but very vague direction to make Silverstripe CMS architecture cleaner, less coupled, and easier to pull apart without the whole tower collapsing. I don't think anyone currently has a specific, well articulated long-term vision or strategy for Silverstripe CMS such as could populate a clear roadmap where we can forecast exactly when various features will drop. At the same time, we have scoped some items like this that help set us (Silverstripe CMS maintainers) and the community up for success. Another example of work like this that has been scoped is various issues related to refactoring LeftAndMain/CMSMain (see silverstripe/silverstripe-cms#2933 and the various issues that have linked to it). Yes to some degree these are "random big impact changes" - though we are keenly aware that all API breaking changes have the potential to cause upgrade pain, and are intentionally implementing the changes in such a way to minimise that pain. |
https://github.com/silverstripeltd/product-issues/issues/875 (private repo) outlines a POC for abstracting the template layer into its own module. This issue is about taking that POC and implementing it properly for CMS 6.
This will provide the following benefits:
Related issues
Check if these can be sensibly resolved as part of this work
$scope->locally()
)ViewableData
#11262ViewableData::Me()
out ofViewableData
and into the template layer #11261Notes
ThemeResourceLoader::findTemplate()
andSSViewer::get_templates_by_class()
if those end up being moved or significantly refactoredViewableData_Customised
andViewableData_Debugger
- at a minimum they need the same renaming treatment thatViewableData
is getting.ContentNegotiator
has some weird regex related to the output of<% base_tag %>
Acceptance criteria
.ss
templates is moved into a new repository calledsilverstripe/silverstripe-template-renderer
silverstripe/framework
since that contains.ss
templates.SSViewer
remains in framework, and has the following responsibilities:process()
on theSSViewer
instance, which instantiates a template renderer and asks for the rendered HTML)Requirements
API into the rendered HTML resultDBHtmlText
SSViewer
or both moved to the new moduleSSViewer
to abstractly interact with a template rendering engine.ss
extensionViewLayerData
class is created.DBField
instance (plus arrays to eitherArrayList
orArrayData
)ModelData::exists()
where that has been implemented)ViewableData
is renamed toModelData
, since it really represents a "Model" in the MVC sense.Explicitly excluded
ViewableData
per https://github.com/silverstripeltd/product-issues/issues/875#issuecomment-2249358959 (can easily do as a follow-on card afterward)Strong-typing PRs
Adding some strong typing to some existing API before I start the refactor so I have more confidence in the types of the values I'm dealing with and passing through to the template layer.
Kitchen sink CI run
Renaming PRs
CMS 5
CMS 6
kitchen sink CI
The CI failure is unrelated to these PRs and was caused by a fluent PR being merged before the code it relies on.
Refactor PRs
CMS 5
CMS 6
Kitchen sink CI run: https://github.com/creative-commoners/recipe-kitchen-sink/actions/runs/11584927001
I've made an example of what a twig module may look like, including twig versions of the simple theme templates.
https://github.com/GuySartorelli/silverstripe-twig
The readme notes some ways template creators will need to workaround the way the abstraction works - notably having to use
getRawDataValue()
for conditional checks.If you want to try it out to test the abstraction, follow the instructions in the readme to set it as the renderer for
PageController
Migration PRs
CMS 5
CMS 6
Kitchen sink CI
The above PRs should be merged before any others, since CI won't be happy on the others until the new repo is in packagist, and it can't be added to packagist until composer.json is added.
Supported modules PR is needed for GHA to generate a matrix so we can actually test the module itself.
The text was updated successfully, but these errors were encountered: