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

Move FrontController::updateQueryString to Tools to make reusable #33779

Merged
merged 6 commits into from Sep 28, 2023
Merged

Move FrontController::updateQueryString to Tools to make reusable #33779

merged 6 commits into from Sep 28, 2023

Conversation

PululuK
Copy link
Member

@PululuK PululuK commented Aug 30, 2023 •
edited

Questions Answers
Branch? develop
Description? Move FrontController::updateQueryString to Tools and make reusable.
Type? refacto
Category? CO
BC breaks? no
Deprecations? no
How to test? Test green :)
Fixed issue or discussion? N/A
Related PRs N/A
Sponsor company Evolutive

@PululuK PululuK requested a review from a team as a code owner August 30, 2023 20:59
@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring labels Aug 30, 2023
FabienPapet
FabienPapet requested changes Aug 30, 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.

Hey there ! 👋
I don't think moving code from FrontController to Tools helps the migration but I'm not against it.

If you want to do so, I think you should deprecate the old function as well, so it can be removed in next versions. As well as updating all the code using this function inside the project.

@PululuK
Copy link
Member Author

PululuK commented Aug 30, 2023

Hey there ! 👋 I don't think moving code from FrontController to Tools helps the migration but I'm not against it.

If you want to do so, I think you should deprecate the old function as well, so it can be removed in next versions. As well as updating all the code using this function inside the project.

is a protected method, We need realy to add deprecated flag ?

@FabienPapet
Copy link
Member

FabienPapet commented Aug 30, 2023 •
edited

Absolutely, because a child class can use it. This is covered by the BC promise. Only the private methods don't need it :)

@PululuK
Copy link
Member Author

PululuK commented Aug 30, 2023

Oh yeah ! Done

kpodemski
kpodemski previously approved these changes Aug 31, 2023
View reviewed changes
@FabienPapet
Copy link
Member

Some usages still need to be replaced, and then all good for me

M0rgan01
M0rgan01 previously approved these changes Sep 13, 2023
View reviewed changes
@ps-jarvis ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Sep 13, 2023
@FabienPapet FabienPapet added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 13, 2023
FabienPapet
FabienPapet requested changes Sep 13, 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.

Requesting changes because of the bot

@kpodemski
Copy link
Contributor

kpodemski commented Sep 14, 2023 •
edited

@FabienPapet, where do you see other usages in the core? unless you meant the Faceted Search module? (which should not block the PR)

@FabienPapet
Copy link
Member

image

@PululuK PululuK dismissed stale reviews from M0rgan01 and kpodemski via 2334d65 September 18, 2023 20:42
@PululuK PululuK removed the Waiting for author Status: action required, waiting for author feedback label Sep 19, 2023
boherm
boherm reviewed Sep 20, 2023
View reviewed changes
Copy link
Member

@boherm boherm left a comment

Choose a reason for hiding this comment

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

Ok for me! But just a little comment :)

classes/controller/FrontController.php Show resolved Hide resolved
matthieu-rolland
matthieu-rolland approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@matthieu-rolland matthieu-rolland left a comment

Choose a reason for hiding this comment

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

Do you want to apply @boherm 's feedback or should we merge ?

@boherm
Copy link
Member

boherm commented Sep 21, 2023

@matthieu-rolland I'm not blocking this PR, so we can merge as is. ;)

@M0rgan01 M0rgan01 added Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback labels Sep 25, 2023
@prestonBot
Copy link
Collaborator

QA approved, well done! Message to the maintainers: do not forget to milestone it before the merge.

@M0rgan01
Copy link
Contributor

After some checks, it's good for me :) Thanks

@M0rgan01 M0rgan01 merged commit f2103d8 into PrestaShop:develop Sep 28, 2023
18 checks passed
@PululuK PululuK deleted the modev-updateQueryString-to-Tools branch September 28, 2023 15:18
@matks
Copy link
Contributor

matks commented Oct 16, 2023

@M0rgan01 don't forget the milestone

@matks matks added this to the 9.0.0 milestone Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FabienPapet FabienPapet FabienPapet approved these changes

@matthieu-rolland matthieu-rolland matthieu-rolland approved these changes

@matks matks matks approved these changes

@boherm boherm boherm approved these changes

@M0rgan01 M0rgan01 M0rgan01 approved these changes

@kpodemski kpodemski Awaiting requested review from kpodemski

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

Successfully merging this pull request may close these issues.

None yet
9 participants
@PululuK @FabienPapet @kpodemski @boherm @prestonBot @M0rgan01 @matks @matthieu-rolland @ps-jarvis