search

silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
722 stars 821 forks source link

Abstract template layer into its own dedicated module #11322

Closed GuySartorelli closed 3 weeks ago

GuySartorelli commented 3 months ago

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

Notes

Acceptance criteria

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

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.

emteknetnz commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

👍🏻 this scope sounds great to me

GuySartorelli commented 3 months ago

@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 https://github.com/silverstripe/silverstripe-cms/issues/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.

emteknetnz commented 2 months ago

@GuySartorelli PRs are draft and require rebasing

GuySartorelli commented 2 months ago

They didn't need rebasing when I put this in peer review lol. Rebased.

emteknetnz commented 1 month ago

@GuySartorelli Even though it's draft, I've done some initial peer review on the CMS 6 framework PR. I'm not sure if it will mean that we'll need to change the CMS 5 PRs, so I've held off merging those

GuySartorelli commented 1 month ago

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