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

[SF Migration] Get rid of legacy layout for twig-rendered pages #12925

Open
matks opened this issue Mar 14, 2019 · 9 comments

Comments

@matks
Copy link
Contributor

commented Mar 14, 2019

Overview

Currently, migrated pages rendered by Twig do rely on a Smarty layout:

This was probably done in order to benefit of all the features implemented into the legacy layout: hooking capabilities, menu navbar, header toolbar ... however it comes with a performance cost.

We have started to diagnose the features provided by this smarty legacy layout in order to remove it. This means we need to re-implement this features into the new twig layout and keep previous behaviors working, especially hooking capabilities.

Features list

  • Page head (meta, css files, js files, many js variables)
  • Breadcrumbs
  • Filters in breadcrumbs
  • Toolbar title
  • Hooks ⚠️
  • Page header toolbar (with help link)
  • Employee logout action listener
  • Employee cookie expiration check
  • Shop context selection
  • Header rendering (logo, search bar, quick access, etc.)
  • Navigation menu
  • LiteDisplaying mode
  • Context initialization (shop, country, currency)
@matks

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Thanks @rokaszygmantas for the exploration 👍

@matks matks added the migration label Mar 14, 2019
@matks matks added this to To do in Symfony Migration via automation Mar 14, 2019
@matks

This comment has been minimized.

Copy link
Contributor Author

commented Mar 14, 2019

Important: although the previous BO theme (a.k.a "default" theme) and the new BO theme ("new-theme") are being rendered using the same php code for controllers, there are 2 different smarty layouts. So we can remove the new layout without breaking the not-migrated-yet pages.

The layout to target is https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/themes/new-theme/template/layout.tpl

@sarjon

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

How about replacing sidebar menu building with https://symfony.com/doc/master/bundles/KnpMenuBundle/index.html in new layout?

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2019

How about replacing sidebar menu building with https://symfony.com/doc/master/bundles/KnpMenuBundle/index.html in new layout?

That's an interesting idea, but we need to maintain compatibility with previous theme features, especially modules hooking and tab management.

@sarjon

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

but we need to maintain compatibility with previous theme features, especially modules hooking and tab management.

Yep, it would still build menu from database Tabs (both modules and core).

@sarjon

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Some further notes:

  1. There is a bunch of global JS variables in legacy layout that both core and modules depend on.
  2. Custom CSS & JS can be added to controller using legacy $controller->addCSS() and $controller->addJS() methods.
  3. There is addJqueryPlugin() method in legacy controller that both modules and core can use to add jQuery plugins to page.
  4. Legacy layout uses Media to add custom JS variables (and more) to template. A lot of modules depend on it.
  5. It is a common use case for modules to register actionAdminControllerSetMedia and inside hook use $this->context->controller->addJS() and other methods (points 2. and 3.) to add custom assets, this will probably won't work after migrating layout.
  6. more is yet to come

This is how I see steps, that would be taken to migrate legacy layout:

  1. Take empty layout page (this thing https://prnt.sc/oihe7k) and split it into components (top bar, header, side navigation, footer & etc).
  2. Migrate one component by one to brand new Twig layout without carrying to much about backward-compatibility.
  3. After layout is migrated to Twig, add as much backward-compatibility as possible.
  4. Refactor Shop Context initialization (see AdminController::initShopContext())
  5. Deal with other problems that will come up during layout migration.

Even if we were to implement new layout, I don't think we will be able to switch migrated pages to it. To be honest, after investigating legacy code, I don't think that layout should be changed to Twig in 1.7 branch, as it potentially could introduce a lot of breaking changes.

@matks what are your thoughts on this? How would you suggest migrating layout?

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

Even if we were to implement new layout, I don't think we will be able to switch migrated pages to it. To be honest, after investigating legacy code, I don't think that layout should be changed to Twig in 1.7 branch, as it potentially could introduce a lot of breaking changes.

@matks what are your thoughts on this? How would you suggest migrating layout?

I was afraid of this answer :/

There's no way to trick modules into thinking they can still use functions like addCss(), addJs(), and use former hooks, but actually it calls the new code instead ?

I talked with a PS module developer today, he told me most modules he knows heavily rely on the hook actionAdminControllerSetMedia.

I see 2 possibilities then:

  1. We stick to the plan and migrate the layout, and we write a very accurate list of all the breaking changes that this brings in, classifying changes in
  • things we can fix by providing a bridge or a fake legacy function that actually does the right job
  • things that require a minor change from BO module developers (like having an twig equivalent available)
  • things that require a big change in BO modules
  1. Turn the issue the other way around: right now, even if it's not pretty, this legacy smarty layout is functional. But it makes the BO very slow because it boots both Symfony and the legacy framework and both Smarty and Twig. This is a major pain point. What if we can reduce this pain point by optimizing a lot this layout rendering, and keep the "get rid of legacy layout" for PrestaShop 8 ?
@sarjon

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

There's no way to trick modules into thinking they can still use functions like addCss(), addJs(), and use former hooks, but actually it calls the new code instead ?

I think there could be, but I cannot guarantee it 100% percent, we would need to migrate and try to implement that.

We stick to the plan and migrate the layout, and we write a very accurate list of all the breaking changes that this brings in

It's an option.

What if we can reduce this pain point by optimizing a lot this layout rendering, and keep the "get rid of legacy layout" for PrestaShop 8 ?

Hmmm, it's a very vague idea to "optimize rendering", Twig is fast on it's own, legacy pages are also fast when on Smarty, I don't think there is problem with tools, it's the problem with mixing them, so I don't think we can optimize that so see major difference (if any) as it would be when using single templating engine.

@matks

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

There's no way to trick modules into thinking they can still use functions like addCss(), addJs(), and use former hooks, but actually it calls the new code instead ?

Can we try with a POC ?

What if we can reduce this pain point by optimizing a lot this layout rendering, and keep the "get rid of legacy layout" for PrestaShop 8 ?

Hmmm, it's a very vague idea to "optimize rendering", Twig is fast on it's own, legacy pages are also fast when on Smarty, I don't think there is problem with tools, it's the problem with mixing them, so I don't think we can optimize that so see major difference (if any) as it would be when using single templating engine.

One idea was that currently, if I'm not wrong, we have

  • request arrives on Symfony
  • Symfony prepares the Response, starts rendering Twig template
  • in Twig template we call get_legacy_layout() that actually calls legacy framework because it calls a legacy controller
  • legacy controller fetches the data and renders the Smarty layout
  • get_legacy_layout() returns the response, Twig can finish the rendering
  • full Response is returned

If we could get Symfony to call Smarty instead we could avoid booting the legacy framework. So, keeping Smarty in the loop but getting rid of the legacy framework boot + the legacy controller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Symfony Migration
  
Backlog
2 participants
You can’t perform that action at this time.