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

Abstract template layer into its own dedicated module #11322

Open
19 of 23 tasks
GuySartorelli opened this issue Aug 5, 2024 · 13 comments
Open
19 of 23 tasks

Abstract template layer into its own dedicated module #11322

GuySartorelli opened this issue Aug 5, 2024 · 13 comments

Comments

@GuySartorelli
Copy link
Member

GuySartorelli commented Aug 5, 2024

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:

  • improved separation between the view and model layers
  • improved architecture for the view layer
  • opens the door to further decouple the CMS from the template layer over time (e.g. could move all templates out into an optional 'admin-theme', and allow projects to introduce their own full 'admin-theme' similar to how the Drupal backend works)
  • ability to swap out the rendering engine easily in the future, or per project, if that's desired

Related issues

Check if these can be sensibly resolved as part of this work

Notes

  • Check everywhere that uses ThemeResourceLoader::findTemplate() and SSViewer::get_templates_by_class() if those end up being moved or significantly refactored
  • Don't forget to check what's going on with ViewableData_Customised and ViewableData_Debugger - at a minimum they need the same renaming treatment that ViewableData is getting.
  • ContentNegotiator has some weird regex related to the output of <% base_tag %>

Acceptance criteria

  • Code related to rendering .ss templates is moved into a new repository called silverstripe/silverstripe-template-renderer
    • To the extent that it's feasible, commit history for that code is copied into the new repository
    • The new module is a direct dependency of silverstripe/framework since that contains .ss templates.
  • SSViewer remains in framework, and has the following responsibilities:
    • It delegates template rendering (i.e. you call process() on the SSViewer instance, which instantiates a template renderer and asks for the rendered HTML)
    • It keeps track of themes
    • It injects js/css from the Requirements API into the rendered HTML result
    • It wraps the rendered HTML (string) in DBHtmlText
  • The content for the base tag and functionality for rewriting anchor links are either both retained in SSViewer or both moved to the new module
  • There is an interface in framework that defines the minimum API required for SSViewer to abstractly interact with a template rendering engine
  • If feasible with minimal upgrade pain, redefine how template candidates are structured (currently an esoteric associative array)
  • In PHP for core and supported modules, templates are never referenced with the .ss extension
  • A new ViewLayerData class is created.
    • All values must be wrapped in this class before being passed to the template renderer.
    • This class is responsible for the following:
      • Casting values to the appropriate DBField instance (plus arrays to either ArrayList or ArrayData)
      • Determining whether a value "exists" (most likely delegating to ModelData::exists() where that has been implemented)
  • ViewableData is renamed to ModelData, since it really represents a "Model" in the MVC sense.
  • Documentation is updated as appropriate
    • Any references to the API are updated
    • There's docs indicating how to replace the template renderer (and clearly highlighting the drawbacks of doing so)
    • As appropriate, distinguish between the model layer and the view layer in the docs
  • Overall test coverage is not reduced
  • There is a clear upgrade path

Explicitly excluded

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

TBD

CMS 6

After merging, please reassign to Guy so they can add the new repo to packagist - it can't be added until composer.json is added.

@emteknetnz
Copy link
Member

emteknetnz commented Aug 5, 2024

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

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Aug 5, 2024

With this approach, ALL templates for a project must be in the same template syntax.
So if you're using the ss template renderer for your project, ALL templates must be in ss syntax.
If you've implemented a twig template renderer that follows the abstract API, ALL templates in your project (including ones usually provided by framework, admin, cms, etc) must be in twig syntax.

This is as per the description in https://github.com/silverstripeltd/product-issues/issues/875:

This would mean people would be free to replace the rendering engine with one of their choice if they really want to - with the caveat that they then need to convert and maintain maintain all templates from supported and third-party modules in their template syntax of choice. It gives people freedom, but doesn't give the CMS Squad responsibility for those choices.
...
after all of the above, we can completely replace the template engine in a project (with the caveat that all ss templates would also need to be replaced in that project)

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 LeftAndMain could use .ss templates and everything else (i.e. the frontend) could use .whatever templates. But the abstraction itself has to happen first.

@emteknetnz
Copy link
Member

emteknetnz commented Aug 5, 2024

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?

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Aug 5, 2024

Yeah isolating the CMS templates away from everything else seems like it should be done as part of this card

I would prefer it be a separate card. This is going to be a big change as it is.

it's absolutely crucial for this to have any real world usage

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.

if you have website project running twig, you now CANNOT use this module

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.

@emteknetnz
Copy link
Member

emteknetnz commented Aug 5, 2024

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.

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:

templates/Layout/Page.ss (PageController)
  ...
  $ElementalArea 
  templates/DNADesign/Elemental/Models/ElementalArea.ss <<< third-party module (elemental)
    ...
    <% loop $ElementControllers %>
      $Me
      templates/DNADesign/Elemental/Layout/ElementHolder.ss
      ...
	$Element
        templates/SilverStripe/ElementalBannerBlock/Block/BannerBlock.ss <<< third-party module (banner-block)
           ...
           $File, $Title, etc

If you were running Twig for your site, presumably in this case you'd need to
a) Replace all the elemental templates with twig templates
b) Replace the banner-block template with a twig template?

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?

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Aug 5, 2024

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

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.

@michalkleiner
Copy link
Contributor

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.

@jakxnz
Copy link
Contributor

jakxnz commented Aug 21, 2024

👍🏻 this scope sounds great to me

@GuySartorelli
Copy link
Member Author

@michalkleiner

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.

@GuySartorelli
Copy link
Member Author

Reopening - still got the last step of migrating to its own module

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants