0024 - Refactoring of PrestaShop context by jolelievre · Pull Request #36 · PrestaShop/ADR

jolelievre · 2023-09-11T10:21:42Z

Questions	Answers
Description?	The ADR contains almost nothing but was necessary to get a reference in this POC PR PrestaShop/PrestaShop#33223
Type?	improvement
BC breaks?	no
Deprecations?	no
Fixed ticket?	~
Sponsor company	~
How to test?	~

tleon

just a little typo i think

tleon · 2024-02-23T14:39:31Z

+  - an automatic listener will be in charge of looping through all the builders that implement `LegacyContextBuilderInterface` and build the legacy context automatically, as long as the required data has been provided, it will ultimately completely replace the other means of building the legacy context
+  - the builder is in charge of fetching any data from DB or any advance building operations However it is not capable ok detecting itself the required arguments to build the context (like Entity IDs), this responsibility is left to the listener
+- a Symfony listener per sub context, it is responsible for getting the data required and inject it inside the `SubContextBuilder`, nothing more they are not responsible for triggering the actual build
+  - the data initialized bu the listener must be kept to its minimum, like a Locale code, or an Entity ID All the advanced fetching is left to the builder, the listener is also responsible for defining the default fallback when they are needed


Suggested change

- the data initialized bu the listener must be kept to its minimum, like a Locale code, or an Entity ID All the advanced fetching is left to the builder, the listener is also responsible for defining the default fallback when they are needed

- the data initialized by the listener must be kept to its minimum, like a Locale code, or an Entity ID All the advanced fetching is left to the builder, the listener is also responsible for defining the default fallback when they are needed

yep, thanks

matks

Good enough for me. I provided some suggestions.

Another suggestion: this is supposed to be an ADR, A is Architecture. I feel like you went very deep into the implementation for example

  - an automatic listener will be in charge of looping through all the builders that implement `LegacyContextBuilderInterface` and build the legacy context automatically, as long as the required data has been provided, it will ultimately completely replace the other means of building the legacy context

I think these are "details of implementation" and are not 100% architecture. They could change in the future while the architecture concept of having SubContext and SubContextBuilder would stay.

matks · 2024-02-26T10:49:10Z

+- a `SubContextBuilder`, it provides getters/setters to specify the parameters required to build the `SubContext` class and a `build` method that returns the `SubContext` instance
+  - any context builder can also implement the `PrestaShop\PrestaShop\Core\Context\LegacyContextBuilderInterface` interface, it requires implementing `buildLegacyContext` that will be used to initialize the legacy Context for backward compatibility, of course the data used to build the legacy context must be synced with the one used for the modern Context service
+  - an automatic listener will be in charge of looping through all the builders that implement `LegacyContextBuilderInterface` and build the legacy context automatically, as long as the required data has been provided, it will ultimately completely replace the other means of building the legacy context
+  - the builder is in charge of fetching any data from DB or any advance building operations However it is not capable ok detecting itself the required arguments to build the context (like Entity IDs), this responsibility is left to the listener


Suggested change

- the builder is in charge of fetching any data from DB or any advance building operations However it is not capable ok detecting itself the required arguments to build the context (like Entity IDs), this responsibility is left to the listener

- the builder is in charge of fetching any data from DB or any advance building operations However it is not capable of detecting itself the required arguments to build the context (like Entity IDs), this responsibility is left to the listener

matks · 2024-02-26T10:50:00Z

+
+Based on these three elements we can then use the Symfony DI in our favor, the `SubContext` is a service, so it will be easily injectable any place we need it, the `SubContextBuilder` is the factory that allows to build this service.
+
+The `SubContext` service **MUST** be a **lazy** service for several reasons:


I suggest provide a reference to what you mean by "lazy" 😄
https://developer.mozilla.org/en-US/docs/Web/Performance/Lazy_loading

Example: https://github.com/PrestaShop/PrestaShop/blob/develop/src/Adapter/Presenter/Product/ProductLazyArray.php

(not a greatest one, as many things are loaded in the constructor anyway)

Good idea to add a link but we're talking about Symfony services so we should use https://symfony.com/doc/current/service_container/lazy_services.html instead

matks · 2024-02-26T10:51:22Z

+In legacy code of PrestaShop, but sadly in a lot of recent code as well, we rely heavily on the `Context` [class/singleton](https://github.com/PrestaShop/PrestaShop/blob/8.1.0/classes/Context.php#L316)
+This legacy context, although convenient at times, has many flaws:
+- it's completely mutable, and lots of bugs in PrestaShop are related to values in this context being changed by unexpected code
+- the way it's built is not very well known mostly because it's fragmented in many different places in the current code


Suggestion: provide some more infos about how it's being built (from example some values come from the http request)

Hmm, it's in opposition with your other comment stating that we go too deep in the implementation 🤔 (even though I don't fully agree ith that either) Especially since here I clearly say that we don't really know how it's built precisely, how can we detail something we don't know?

M0rgan01 · 2024-02-26T14:21:49Z

+- Shop context
+- Language context
+- Currency context
+- Country context
+- Employee context
+- API Client context


the API part is new right? it is indeed part of the context, but it does not come from a split

Yep you're right it's a new one I will add a mention about this

jolelievre changed the title ~~Refactoring of PrestaShop context~~ 0024 - Refactoring of PrestaShop context Sep 13, 2023

jolelievre force-pushed the context-refacto branch from 04fa3ce to d966b85 Compare February 16, 2024 12:03

jolelievre marked this pull request as ready for review February 16, 2024 12:03

tleon approved these changes Feb 23, 2024

View reviewed changes

ps-jarvis added the Waiting for QA label Feb 23, 2024

jolelievre removed the Waiting for QA label Feb 23, 2024

jolelievre force-pushed the context-refacto branch from d966b85 to b3c86ab Compare February 23, 2024 15:19

matks approved these changes Feb 26, 2024

View reviewed changes

matthieu-rolland approved these changes Feb 26, 2024

View reviewed changes

M0rgan01 reviewed Feb 26, 2024

View reviewed changes

M0rgan01 approved these changes Feb 26, 2024

View reviewed changes

Context refactoring new architecture, replacement of the legacy context

cf00c33

jolelievre force-pushed the context-refacto branch from b3c86ab to cf00c33 Compare February 28, 2024 08:38

jolelievre merged commit 22e66ea into PrestaShop:master Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0024 - Refactoring of PrestaShop context#36

0024 - Refactoring of PrestaShop context#36
jolelievre merged 1 commit intoPrestaShop:masterfrom
jolelievre:context-refacto

jolelievre commented Sep 11, 2023

Uh oh!

tleon left a comment

Uh oh!

tleon Feb 23, 2024

Uh oh!

jolelievre Feb 23, 2024

Uh oh!

matks left a comment

Uh oh!

matks Feb 26, 2024

Uh oh!

matks Feb 26, 2024

Uh oh!

Hlavtox Feb 26, 2024 •

edited

Loading

Uh oh!

jolelievre Feb 28, 2024

Uh oh!

matks Feb 26, 2024

Uh oh!

jolelievre Feb 28, 2024

Uh oh!

M0rgan01 Feb 26, 2024

Uh oh!

jolelievre Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

	- the data initialized bu the listener must be kept to its minimum, like a Locale code, or an Entity ID All the advanced fetching is left to the builder, the listener is also responsible for defining the default fallback when they are needed
	- the data initialized by the listener must be kept to its minimum, like a Locale code, or an Entity ID All the advanced fetching is left to the builder, the listener is also responsible for defining the default fallback when they are needed

	- the builder is in charge of fetching any data from DB or any advance building operations However it is not capable ok detecting itself the required arguments to build the context (like Entity IDs), this responsibility is left to the listener
	- the builder is in charge of fetching any data from DB or any advance building operations However it is not capable of detecting itself the required arguments to build the context (like Entity IDs), this responsibility is left to the listener


		Based on these three elements we can then use the Symfony DI in our favor, the `SubContext` is a service, so it will be easily injectable any place we need it, the `SubContextBuilder` is the factory that allows to build this service.

		The `SubContext` service MUST be a lazy service for several reasons:

Conversation

jolelievre commented Sep 11, 2023

Uh oh!

tleon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hlavtox Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Hlavtox Feb 26, 2024 •

edited

Loading