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

Deleting files marked as "to be deleted in 1.8" #29284

Closed
wants to merge 1 commit into from
Closed

Deleting files marked as "to be deleted in 1.8" #29284

wants to merge 1 commit into from

Conversation

Sinepel
Copy link
Contributor

@Sinepel Sinepel commented Aug 9, 2022 •
edited

Questions Answers
Branch? develop
Description? Remove three files marked as "to be removed in 1.8" aka, 8.0
Type? refacto
Category? CO
BC breaks? yes
Deprecations? no
Fixed ticket? no
Related PRs no
How to test?
Possible impacts? If specific modules or developments of certain shops use these classes directly

BC Breaks

Removed class src/Adapter/ClassLang.php
Removed class src/PrestaShopBundle/Form/Admin/Type/TextEmptyType.php
Removed class src/PrestaShopBundle/Form/Admin/Type/TextareaEmptyType.php

@Sinepel Sinepel requested a review from a team as a code owner August 9, 2022 12:05
@prestonBot prestonBot added develop Branch Refactoring Type: Refactoring labels Aug 9, 2022
matks
matks approved these changes Aug 12, 2022
View reviewed changes
Copy link
Contributor

@matks matks left a comment

Choose a reason for hiding this comment

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

Woups these files went off the radar during the clean for 8.0 😅

At least they'll be gone for PS 8.1 👍

@matks matks added the BC break Type: Introduces a backwards-incompatible break label Aug 12, 2022
@matks
Copy link
Contributor

matks commented Aug 12, 2022

@Sinepel Even if this is expected we need to inform people "we removed these files" 😄 can you please add the BC break description?

@matks matks added the Needs documentation Needs an update of the developer documentation label Aug 12, 2022
@Progi1984
Copy link
Contributor

@Sinepel Why not target the 8.0.x branch ? I believe that we can't remove classes in minor version. It's not compatible with semver.

@Sinepel
Copy link
Contributor Author

Sinepel commented Aug 13, 2022

@matks It's OK !

@Sinepel
Copy link
Contributor Author

Sinepel commented Aug 13, 2022

@Progi1984 Considering it was a BC Breaks, I thought it was too late to include it at this stage in 8.0.0 :)

Should it be changed?

@Progi1984
Copy link
Contributor

Only @PrestaShop/prestashop-maintainers have the control on this decision.

@matks
Copy link
Contributor

matks commented Aug 17, 2022

Apologies @Sinepel I did not look closely at the branch

Considering it was a BC Breaks, I thought it was too late to include it at this stage in 8.0.0 :)

You're right.

I think it's too late for this one. Maybe we mark these files to be deleted in prestashop 9.0.0 instead?

@Sinepel
Copy link
Contributor Author

Sinepel commented Aug 22, 2022 •
edited

@matks

I think it's too late for this one. Maybe we mark these files to be deleted in prestashop 9.0.0 instead?

As you prefer, if it's better to do it that way, let's go.

I am waiting for your confirmation

Amoifr
Amoifr reviewed Aug 23, 2022
View reviewed changes
Copy link
Contributor

@Amoifr Amoifr left a comment

Choose a reason for hiding this comment

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

Thx, good catch

@FabienPapet FabienPapet added this to the 9.0.0 milestone Jan 16, 2023
@kpodemski kpodemski added Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback labels Feb 1, 2023
@kpodemski
Copy link
Contributor

Auto tests: https://github.com/kpodemski/ga.tests.ui.pr/actions/runs/4066725799

@nicosomb
Copy link
Contributor

@kpodemski : do you think that the failing tests are related to this PR? I don't think so. In my opinion, we can merge.

@lartist
Copy link
Contributor

lartist commented Apr 24, 2023

It seems like a rebase is needed: https://github.com/lartist/ga.tests.ui.pr/actions/runs/4788537360

@lartist lartist added Waiting for author Status: action required, waiting for author feedback and removed Waiting for QA Status: action required, waiting for test feedback Waiting for dev Status: action required, waiting for tech feedback labels Apr 26, 2023
@kpodemski kpodemski closed this May 12, 2023
@kpodemski kpodemski reopened this May 12, 2023
@kpodemski kpodemski requested a review from a team as a code owner May 12, 2023 15:23
@kpodemski kpodemski added the Waiting for rebase Status: action required, waiting for rebase label May 12, 2023
@Hlavtox Hlavtox added Waiting for QA Status: action required, waiting for test feedback and removed Waiting for author Status: action required, waiting for author feedback Waiting for rebase Status: action required, waiting for rebase labels May 12, 2023
@florine2623 florine2623 self-assigned this May 17, 2023
florine2623
florine2623 reviewed May 17, 2023
View reviewed changes
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @Sinepel ,

Even after the rebase, it seems like your PR breaks the automated tests. I have relaunched them multiple times.
https://github.com/florine2623/testing_pr/actions/runs/5001093966

Could you check what is wrong ?
Thanks!

@florine2623 florine2623 added the Waiting for author Status: action required, waiting for author feedback label May 17, 2023
@florine2623 florine2623 removed their assignment May 17, 2023
@florine2623 florine2623 removed the Waiting for QA Status: action required, waiting for test feedback label May 17, 2023
@kpodemski
Copy link
Contributor

Since we have had no news from you for more than 20 days, I'm closing this PR. Feel free to re-open it if you find the motivation to finish it. Don't hesitate to ping me if you have any questions or doubts.

@kpodemski kpodemski closed this Jun 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florine2623 florine2623 florine2623 left review comments

@Amoifr Amoifr Amoifr left review comments

@kpodemski kpodemski kpodemski approved these changes

@matks matks matks approved these changes

@Hlavtox Hlavtox Hlavtox approved these changes

Assignees
No one assigned
Labels
BC break Type: Introduces a backwards-incompatible break develop Branch Needs documentation Needs an update of the developer documentation Refactoring Type: Refactoring Waiting for author Status: action required, waiting for author feedback
Projects
PR Dashboard
Archived in project
Milestone
9.0.0
Development

Successfully merging this pull request may close these issues.

None yet