0018 - Add ADR about Horizontal Migration

[tegbessou]

Questions	Answers
Description?	Describe the idea for Horizontal Migration

Vote

Maintainer	Vote (✅ or ❌)
@atomiix	✅
@eternoendless	✅
@jolelievre	✅
@kpodemski	✅
@matks	✅
@matthieu-rolland	✅
@NeOMakinG
@Progi1984	❌
@PululuK
@sowbiba

[jolelievre]

Ok for me except a typo and a detail about public fields from legacy controller

[Progi1984]

I vote No.

I don't think it's a good idea to add a new migration. Why ? People, simply people.
The developer responsible of this ADR leaves in July. We have less maintainers since one year (some leave the Core Team, some leave the Maintainer Team). So less review, less management of this part. This makes it seem like we are spreading ourselves too thin with the few resources the project has. So it's why I reject this ADR.

[eternoendless]

I don't understand your reasoning @Progi1984. This represents less work to finish the symfony migration, not more work.

What's your suggestion? Should we abandon the migration?

[eternoendless]

I've added further clarification as to why I think this approach makes sense

[abramofranchetti]

I don't understand your reasoning @Progi1984. This represents less work to finish the symfony migration, not more work.

What's your suggestion? Should we abandon the migration?

I would prefer one migration per page, so that when a fully migrated page is released, the non-migrated ones remain as they were before. This gives more stability as of yet for developers and for the user who knows that when a page is migrated it is converted to the new synfony environment while a page that is not migrated is in the legacy system.

A horizontal migration, especially if carried on for a long time due to lack of programmers, could be a real mess.

Finding yourself working with a code that is partly migrated and partly not because the refactoring activity is missing does not benefit anyone and does not bring many benefits in my opinion. I am better off working when all the layers managing a given page are either all migrated to the new system or all legacy.
I don't understand the rush to do this horizontal migration, it was enough to proceed with the migration page by page until everything was migrated.

[kpodemski]

I lean more toward the "No".

I was skeptical about this from the beginning.

Why?

1. Community developers. Now it's confusing for developers to understand that we have an X/Y Symfony/legacy. Imagine explaining to them that we have a total mix here and there.

I don't believe in removing critical things for the ecosystem one by one, such as "in this version, we'll remove AdminController", "Now we'll remove HelperXYZ 🙄 It would be more frustrating than simply creating a new version of the module that works with the modern architecture.

Migration is for who? For the merchants? No. We can't possibly fix all the bugs simply because we remove AdminController. For developers? No. What's the advantage? Developers can wait. The feedback I hear is "We would like to have Twig in the Front office", "We would like to have a real REST API", not "We would like to have a "Symfony-like" version of all tabs in the back office.

Without going into the details - this strategy means more work for community developers to keep up to date with all the modifications they have in their modules. With the new strategy, it wasn't that hard to make the module compatible with a new Orders page, new Customers page, etc. It seems much more complicated.

2. Testing. What about QA? We'll remove HelperList and replace it with Grid one day. Ok. Does it mean that QA needs to go through 40+ pages in the Back office? And then go through them again when we replace a different core component?

If I'm not mistaken, in every stage of the migration, we need complete tests for the pages. Compare it to one thoroughly tested migration of the page.

3. Is the current process that slow? I don't think so. We had some pages successfully migrated by the community. The problem was:

• slow review process
• documentation on how to work with CQRS in PrestaShop, do's and don'ts, rules on how to approach such migration wasn't and is still not clear enough. Reading reviews of the new Product page, the team introduced some rules in the middle which are not documented. It's only "Let's keep it as X instead of Y because Z". Next person will not be aware of it.

Compared to the vertical approach, the horizontal approach could take slightly longer to complete. However, its main advantage is that we no longer have to wait for the whole migration project to be complete before being able to remove obsolete subsystems

Who would benefit from removing obsolete systems? In the second stage, you want to:

In the second stage, Smarty will be removed from controllers in favor of Twig. This will require adapting the existing Helpers to work with the twig and the new theme, as well as the complete removal of LegacyLayoutController.

What's the point of refactoring `Helper's if we're going to remove them in the next stage. You don't want migration to last the next four years, but these stages sound like a "1 year per stage" + massive amount of work that won't be needed.

What's the alternative? Focus on migrating pages one by one.

Let's take the Permissions configuration page as an example. Was it slow because of the developer? Was it slow because of the difficulties? No. It was slow because of us. Because we failed to deliver reviews, we could not provide proper feedback every time the author needed it.

A little history of it:
First PR - 02.2019. No specification was available.
Then there's a discussion and first "Wait for QA" in 03/2019
07/2019 - no clear way for QA to test everything
10/2019 - still no clear way for QA to test it
16/09/2020 - refacto by Pierre
...hundreds of rebases
05/2021 - another review series
07/2021 - still no idea how to test it
08/2021 - refacto to TS & SF 4
rebases, small reviews, rebases...
11/01/2022 - Waiting for QA
rebases
rebases
02/2022 - new reviews even though it was only being rebased, adding strict_types=1, etc. minor things
09/03/2022 - Waiting for QA again
rebases, PMs decisions, lack of specification, etc.
12/04/2022 - Merged without being fully tested

are you sure it is the strategy that is wrong? How can we be sure that the lack of specification for the pages won't slow down the new strategy too?

[abramofranchetti]

I lean more toward the "No".

I was skeptical about this from the beginning.

Why?

1. Community developers. Now it's confusing for developers to understand that we have an X/Y Symfony/legacy. Imagine explaining to them that we have a total mix here and there.

I don't believe in removing critical things for the ecosystem one by one, such as "in this version, we'll remove AdminController", "Now we'll remove HelperXYZ 🙄 It would be more frustrating than simply creating a new version of the module that works with the modern architecture.

Migration is for who? For the merchants? No. We can't possibly fix all the bugs simply because we remove AdminController. For developers? No. What's the advantage? Developers can wait. The feedback I hear is "We would like to have Twig in the Front office", "We would like to have a real REST API", not "We would like to have a "Symfony-like" version of all tabs in the back office.

Without going into the details - this strategy means more work for community developers to keep up to date with all the modifications they have in their modules. With the new strategy, it wasn't that hard to make the module compatible with a new Orders page, new Customers page, etc. It seems much more complicated.

2. Testing. What about QA? We'll remove HelperList and replace it with Grid one day. Ok. Does it mean that QA needs to go through 40+ pages in the Back office? And then go through them again when we replace a different core component?

If I'm not mistaken, in every stage of the migration, we need complete tests for the pages. Compare it to one thoroughly tested migration of the page.

3. Is the current process that slow? I don't think so. We had some pages successfully migrated by the community. The problem was:

• slow review process
• documentation on how to work with CQRS in PrestaShop, do's and don'ts, rules on how to approach such migration wasn't and is still not clear enough. Reading reviews of the new Product page, the team introduced some rules in the middle which are not documented. It's only "Let's keep it as X instead of Y because Z". Next person will not be aware of it.

Compared to the vertical approach, the horizontal approach could take slightly longer to complete. However, its main advantage is that we no longer have to wait for the whole migration project to be complete before being able to remove obsolete subsystems

Who would benefit from removing obsolete systems? In the second stage, you want to:

In the second stage, Smarty will be removed from controllers in favor of Twig. This will require adapting the existing Helpers to work with the twig and the new theme, as well as the complete removal of LegacyLayoutController.

What's the point of refactoring `Helper's if we're going to remove them in the next stage. You don't want migration to last the next four years, but these stages sound like a "1 year per stage" + massive amount of work that won't be needed.

What's the alternative? Focus on migrating pages one by one.

Let's take the Permissions configuration page as an example. Was it slow because of the developer? Was it slow because of the difficulties? No. It was slow because of us. Because we failed to deliver reviews, we could not provide proper feedback every time the author needed it.

A little history of it: First PR - 02.2019. No specification was available. Then there's a discussion and first "Wait for QA" in 03/2019 07/2019 - no clear way for QA to test everything 10/2019 - still no clear way for QA to test it 16/09/2020 - refacto by Pierre ...hundreds of rebases 05/2021 - another review series 07/2021 - still no idea how to test it 08/2021 - refacto to TS & SF 4 rebases, small reviews, rebases... 11/01/2022 - Waiting for QA rebases rebases 02/2022 - new reviews even though it was only being rebased, adding strict_types=1, etc. minor things 09/03/2022 - Waiting for QA again rebases, PMs decisions, lack of specification, etc. 12/04/2022 - Merged without being fully tested

are you sure it is the strategy that is wrong? How can we be sure that the lack of specification for the pages won't slow down the new strategy too?

I completely agree. I think page by page migration is more clear for developer and users that have to use prestashop.

[jolelievre]

A typo and a precision and it's ok for me

[kpodemski]

I had a chance to speak with @eternoendless to understand this concept better.

A couple of essential points that help me to change my vote are:

• it would be possible to merge migrated pages sooner and keep them disabled/behind the feature flag, which will allow developers working on the migration to avoid the frustration of having to rebase their PRs over and over again
• HelperList, HelperForm, and some other components are not going anywhere soon, and we'll remove them in the 4th stage - this will allow many modules from our ecosystem to work as expected. Of course, some developers will need to adapt to make sure their modules that modify some Core Controllers are working as expected, but it's the case for everything in the core
• we need to try, simply as that, we need this move, no alternative was proven to be better
• and one important point that I noticed too late:

We can still continue using the vertical approch for pages where the horizontal approach would not make tactial sense, either because they are too simple to benefit from the advantages of horizontal migration, or because they require extensive refactoring (eg the product page).

A few additional thoughts:

• complete, detailed documentation on how to migrate controller layers for every stage - this could help other community members and me help with the project
• as soon as possible QA team should have a chance to test migrated controller and check if automated tests are working as before and, if not, discover and document problems - too much manual testing for it will slow down the process
• we need to do what is in our power to have a feedback from community developers who work with more advanced customizations of controllers in the Back office - in the end, we're creating a tool for them to use
• it would be great to have a "migration-focus period" later in the future to have more people assigned to this project as a primary task - both migrating pages and reviewing the pages of others involved

I'm still not sure if this will help QA, this is why I would like to know their thoughts as soon as possible, but it doesn't block me from giving this a ✅

[abramofranchetti]

I had a chance to speak with @eternoendless to understand this concept better.

A couple of essential points that help me to change my vote are:

• it would be possible to merge migrated pages sooner and keep them disabled/behind the feature flag, which will allow developers working on the migration to avoid the frustration of having to rebase their PRs over and over again
• HelperList, HelperForm, and some other components are not going anywhere soon, and we'll remove them in the 4th stage - this will allow many modules from our ecosystem to work as expected. Of course, some developers will need to adapt to make sure their modules that modify some Core Controllers are working as expected, but it's the case for everything in the core
• we need to try, simply as that, we need this move, no alternative was proven to be better
• and one important point that I noticed too late:

We can still continue using the vertical approch for pages where the horizontal approach would not make tactial sense, either because they are too simple to benefit from the advantages of horizontal migration, or because they require extensive refactoring (eg the product page).

A few additional thoughts:

• complete, detailed documentation on how to migrate controller layers for every stage - this could help other community members and me help with the project
• as soon as possible QA team should have a chance to test migrated controller and check if automated tests are working as before and, if not, discover and document problems - too much manual testing for it will slow down the process
• we need to do what is in our power to have a feedback from community developers who work with more advanced customizations of controllers in the Back office - in the end, we're creating a tool for them to use
• it would be great to have a "migration-focus period" later in the future to have more people assigned to this project as a primary task - both migrating pages and reviewing the pages of others involved

I'm still not sure if this will help QA, this is why I would like to know their thoughts as soon as possible, but it doesn't block me from giving this a ✅

Yes, for me the fundamental points are 3

1. do not carry out this horizontal migration indefinitely but that it is done quickly, if it really has to be done.
2. migrate more complex pages vertically, so the job is done when you release the change
3. have documentation that accurately reports the state of things.

[MeKeyCool]

To be discussed in another place.

[eternoendless]

As I explained to Krystian earlier today, the spirit of this strategy change is to accelerate the migration, which is required to keep the ecosystem interesting for developers.

Of course it's preferrable to migrate pages vertically. But doing that will take us too long before we reach the minimum level of a Symfony-based architecture. We cannot migrate pages vertically faster because we don't have enough people to do the specification, development, and review. The gaps in functional knowledge make it even harder to refactor, improve, and test because we don't always know what the expected behavior is. The only way I see to accelerate the migration is to focus on the migration itself so that we can deliver the Symfony features (the critical part at least) as quickly as possible, and defer the refactoring to a second stage so that we can have more time to catch up on the knowledge part.

It's like salary. Would you rather work for a whole year and collect all your money on December 31st, or be paid monthly? It's about receiving part of the value as soon as possible.

This is why phase 1 consists in migrating the page without refactoring it extensively – only the controller and the routing is changed. Dividing the migration perimeter into smaller stages means that, where a vertical PR changes 80 files (controller, commands, queries, handlers, form types, form theme, template, grid...), a horizontal migration stage 1 PR should include maybe 2 or 3 files (the controller, the router, and maybe some service configuration files). Smaller PRs are easier to review and verify. In phase 1, the view doesn't change one bit: it's exactly the same, driven by the original helpers on the same Smarty theme.

This should also reduce the temptation to "fix" any UX or historical functional issue that already existed within the page, one of the main reasons for scope "telescoping" in the past, which made some pages take years to review and validate. This should reduce back-and-forths with product and QA.

Initially, I even intended us to simply copy-paste the content of legacy controllers inside Symfony controllers. But after some exploration, we realized that it wasn't possible because the structure of legacy controllers is very complex, override-oriented, tightly coupled to helpers, and globally too hard to make compatible with the Symfony router. Keeping it exactly the same would have required us to create a different kind of Symfony controller – the opposite of my intention. Horizontally migrated controllers are not a different kind of controller, they are the same controllers as the vertical ones. The only difference is that they include a couple of traits that allow the use of Smarty.

One more thing: currently, migrated pages are not usually activated by default. I think we should industrialize feature flags so that we can easily merge incomplete or unstable pages behind feature flags and only activate them once we are confident they are stable. This should also streamline the QA process amd reduce the time to merge (and the risk of rebase conflicts). But that proposal needs to be discussed with QA and subject to another ADR.

[matthieu-rolland]

I vote "yes" 👍

Thank you all for sharing your reasoning and counter-points, and @matks for answering my questions

[NeOMakinG]

Blank vote for me, I'm trusting backend developers if they feel like it will be easier for them to maintain/improve this side of the project

[PululuK]

‎

[kpodemski]

@sowbiba any comment from you?

Acknowledged

[matks]

Hello, I think everyone was able to provide his opinion and vote

We have 6 YES, 3 Blank and 1 NO so the majority has said YES. Consequently this ADR is accepted. Thank you everybody.

Also QA team submitted feedback about it and after discussing together they agreed to it through the person of @sarahdib