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.

Sign up for GitHub

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

Jump to bottom

CO: improvement : Performance String Modifier #34458

Merged
merged 6 commits into from Nov 20, 2023
Merged

CO: improvement : Performance String Modifier #34458

merged 6 commits into from Nov 20, 2023

Conversation

Shoprunners
Copy link
Contributor

@Shoprunners Shoprunners commented Nov 3, 2023 •
edited by M0rgan01

Questions Answers
Branch? develop
Description? This improvements makes reuse of the StringModifier. Why is this an issue? I.e. if a product has many combinations, for each combination a new StringModifier with the whole tail will be created in the initContent method of the frontend ProductController, which costs a good time. There is no need for it and we can just reuse it.
Type? improvement
Category? CO
BC breaks? no
Deprecations? no
How to test? Profile a product with 2000 combinations and look at the initContent time.
UI Tests https://github.com/M0rgan01/ga.tests.ui.pr/actions/runs/6747522063
Fixed issue or discussion? Fixes #34453
Sponsor company Shoprunners

@Shoprunners Shoprunners requested a review from a team as a code owner November 3, 2023 10:39
@prestonBot prestonBot added develop Branch Improvement Type: Improvement labels Nov 3, 2023
@Shoprunners Shoprunners mentioned this pull request Nov 3, 2023
Closed
2 tasks
FabienPapet
FabienPapet previously approved these changes Nov 3, 2023
View reviewed changes
Copy link
Member

@FabienPapet FabienPapet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement

@M0rgan01 M0rgan01 added this to the 9.0.0 milestone Nov 3, 2023
@M0rgan01
Copy link
Contributor

M0rgan01 commented Nov 3, 2023

@Shoprunners Nice work, only one fix is ​​missing on CS Fixer, and it is approved

@Shoprunners
Copy link
Contributor Author

@Shoprunners Nice work, only one fix is ​​missing on CS Fixer, and it is approved

Thanks,
but I don't see the error, I can't find any spaces or missing brackets etc.. Any idea? The error message is not very usefull.

kpodemski
kpodemski reviewed Nov 3, 2023
View reviewed changes
classes/Tools.php Outdated Show resolved Hide resolved
FabienPapet
FabienPapet previously approved these changes Nov 3, 2023
View reviewed changes
FabienPapet
FabienPapet previously approved these changes Nov 3, 2023
View reviewed changes
M0rgan01
M0rgan01 previously approved these changes Nov 3, 2023
View reviewed changes
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Nov 3, 2023
@M0rgan01 M0rgan01 added the Waiting for dev Status: action required, waiting for tech feedback label Nov 3, 2023
jolelievre
jolelievre previously approved these changes Nov 6, 2023
View reviewed changes
boherm
boherm previously approved these changes Nov 6, 2023
View reviewed changes
@MatShir
Copy link
Contributor

MatShir commented Nov 13, 2023

We could cherry-pick for the 8.1.x or change the branch. what do you think @Shoprunners ?

@Shoprunners
Copy link
Contributor Author

We could cherry-pick for the 8.1.x or change the branch. what do you think @Shoprunners ?

Sure, I don't see any reason why not. But I am not good in rebasing etc.

@M0rgan01 M0rgan01 changed the base branch from develop to 8.1.x November 15, 2023 16:19
@M0rgan01 M0rgan01 dismissed stale reviews from boherm, jolelievre, FabienPapet, and themself November 15, 2023 16:19

The base branch was changed.

@M0rgan01 M0rgan01 requested a review from a team as a code owner November 15, 2023 16:19
@M0rgan01 M0rgan01 changed the base branch from 8.1.x to develop November 15, 2023 16:22
@M0rgan01
Copy link
Contributor

@Shoprunners I can do it if you want, but I need publishing rights to your fork

Otherwise, create a new branch (patch-16v2) starting from 8.1.x, cherry-pick the commits from the patch-16 branch and force-push targeting the patch-16 branch. It would then be necessary to change the destination branch of the PR to 8.1.x

@MatShir
Copy link
Contributor

MatShir commented Nov 17, 2023

@Shoprunners up !! You have the checkbox on the bottom right to allow maintainer to edit ;)

@Shoprunners
Copy link
Contributor Author

@Shoprunners I can do it if you want, but I need publishing rights to your fork

Otherwise, create a new branch (patch-16v2) starting from 8.1.x, cherry-pick the commits from the patch-16 branch and force-push targeting the patch-16 branch. It would then be necessary to change the destination branch of the PR to 8.1.x

I added you as collaborateur to the fork. Hope that is what you needed?

@M0rgan01 M0rgan01 changed the base branch from develop to 8.1.x November 17, 2023 16:31
@M0rgan01
Copy link
Contributor

@Shoprunners @MatShir it's all good

@M0rgan01 M0rgan01 modified the milestones: 9.0.0, 8.1.3 Nov 17, 2023
@RosaBenouamer RosaBenouamer removed the Waiting for QA Status: action required, waiting for test feedback label Nov 20, 2023
@M0rgan01 M0rgan01 added QA ✔️ Status: check done, code approved and removed Waiting for dev Status: action required, waiting for tech feedback labels Nov 20, 2023
@M0rgan01 M0rgan01 merged commit 47d0489 into PrestaShop:8.1.x Nov 20, 2023
38 checks passed
@M0rgan01
Copy link
Contributor

thanks @Shoprunners

@Shoprunners Shoprunners deleted the patch-16 branch November 20, 2023 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kpodemski kpodemski kpodemski left review comments

@Hlavtox Hlavtox Hlavtox approved these changes

@ShaiMagal ShaiMagal ShaiMagal approved these changes

@M0rgan01 M0rgan01 M0rgan01 approved these changes

@FabienPapet FabienPapet FabienPapet left review comments

@jolelievre jolelievre jolelievre left review comments

@boherm boherm boherm left review comments

Assignees
No one assigned
Labels
develop Branch Improvement Type: Improvement QA ✔️ Status: check done, code approved
Projects
PR Dashboard
Archived in project
Milestone
8.1.3
Development

Successfully merging this pull request may close these issues.

FO: Slow loading of product page with many combinations