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

Remove unused ProductCriterion.php file #143

Merged
merged 12 commits into from
Jul 15, 2022
Merged

Remove unused ProductCriterion.php file #143

merged 12 commits into from
Jul 15, 2022

Conversation

leemyongpakvn
Copy link
Contributor

@leemyongpakvn leemyongpakvn commented Jun 21, 2022 •
edited by kpodemski
Loading

Questions Answers
Description? ProductCriterion.php is not used anywhere. The correct and working version of it is ProductCommentCriterion.php
Type? improvement
BC breaks? no
Deprecations? yes
Fixed ticket? Fixes PrestaShop/PrestaShop#28855
How to test? Find 'ProductCriterion.php' phrase in the whole PrestaShop folder, it only appears in tests/phpstan/phpstan.neon

@leemyongpakvn
Remove unused ProductCriterion.php
76b15dd 
ProductCriterion.php is not used anywhere.
@leemyongpakvn
Delete ProductCriterion.php
53f4589
@leemyongpakvn
Replace deprecated CHARSET utf8 in install.sql
6577f2c 
deprecated CHARSET utf8 need to be replaced by utf8mb4
@leemyongpakvn leemyongpakvn changed the title Remove unused ProductCriterion.php file Remove unused file and Replace deprecated charset Jun 24, 2022
@leemyongpakvn
Copy link
Contributor Author

mysql.com declared that utf8 was deprecated at https://dev.mysql.com/doc/refman/8.0/en/charset-unicode-utf8mb3.html
Maybe a bot can replace CHARSET utf8 by CHARSET utf8mb4 in other modules' install.sql more effective.

@kpodemski
Copy link
Contributor

Hello @leemyongpakvn

Thank you for your PR. Could you remove changes related to the install.sql from this Pull Request? Let's keep it separate.

PS: I created an Issue for you regarding a not used file. Next time remember to do that as we have to validate the problems that you are trying to change beforehand, or have a written version of a report that something was to be improved, etc.

kpodemski
kpodemski requested changes Jun 25, 2022
View reviewed changes
Copy link
Contributor

@kpodemski kpodemski left a comment

Choose a reason for hiding this comment

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

This is not related to the missing file. Let's keep it separate.

install.sql Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
leemyongpakvn and others added 4 commits June 26, 2022 07:36
@leemyongpakvn @kpodemski
Update install.sql
b763630 
Co-authored-by: Krystian Podemski <kpodemski@users.noreply.github.com>
@leemyongpakvn @kpodemski
Update install.sql
be05559 
Co-authored-by: Krystian Podemski <kpodemski@users.noreply.github.com>
@leemyongpakvn @kpodemski
Update install.sql
0a3c447 
Co-authored-by: Krystian Podemski <kpodemski@users.noreply.github.com>
@leemyongpakvn @kpodemski
Update install.sql
09df320 
Co-authored-by: Krystian Podemski <kpodemski@users.noreply.github.com>
@leemyongpakvn leemyongpakvn changed the title Remove unused file and Replace deprecated charset Remove unused ProductCriterion.php file Jun 26, 2022
leemyongpakvn and others added 3 commits June 26, 2022 07:39
@leemyongpakvn @kpodemski
Update install.sql
5ef856a 
Co-authored-by: Krystian Podemski <kpodemski@users.noreply.github.com>
@leemyongpakvn @kpodemski
Update install.sql
a23f7ad 
Co-authored-by: Krystian Podemski <kpodemski@users.noreply.github.com>
@leemyongpakvn @kpodemski
Update install.sql
907df5c 
Co-authored-by: Krystian Podemski <kpodemski@users.noreply.github.com>
@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Jun 26, 2022 •
edited
Loading

@kpodemski OK. The PR is back to its original (5 days ago) title and 2 commits. I thought someone feel the PR is too small and not worth to review, so I tried to add 1 more commit and change PR title appropriately 2 days ago.
I just wonder if it is needed to create an small issue that I already solve myself with a small PR for a specific module.

ghost
ghost suggested changes Jun 26, 2022
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Small feedback

tests/phpstan/phpstan.neon Outdated Show resolved Hide resolved
install.sql Outdated Show resolved Hide resolved
leemyongpakvn and others added 2 commits June 26, 2022 14:28
@leemyongpakvn @Touxten
Update tests/phpstan/phpstan.neon
1459919 
Co-authored-by: Thomas Roux <contact@okom3pom.com>
@leemyongpakvn @Touxten
Update install.sql
329cd75 
Co-authored-by: Thomas Roux <contact@okom3pom.com>
This was referenced Jun 27, 2022
Closed
Closed
@matks
Copy link
Contributor

matks commented Jun 28, 2022

I just wonder if it is needed to create an small issue that I already solve myself with a small PR for a specific module.

Yes 😄 I know it looks useless to you, but it will be a great help for us. This issue will be used to register, track, label the problem, make sure it does not come back and also be an information carrier about the problem through ages. Someone in 3 or 4 years will maybe be very happy to be able to find it and find what he was looking for. This is extra information very useful for users.

@florine2623 florine2623 self-assigned this Jul 11, 2022
florine2623
florine2623 approved these changes Jul 15, 2022
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 @leemyongpakvn ,

Thanks for the PR!
File is gone. Tested main features of the module on 1.7.8x, 8.0.x, develop. Works as expected 👍

It QA ✅ !

@jolelievre jolelievre merged commit f511c1e into PrestaShop:dev Jul 15, 2022
@jolelievre
Copy link
Contributor

Thanks @leemyongpakvn and @florine2623

@leemyongpakvn leemyongpakvn deleted the patch-2 branch November 2, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@florine2623 florine2623 florine2623 approved these changes

@jolelievre jolelievre jolelievre approved these changes

@kpodemski kpodemski kpodemski approved these changes

Assignees

@florine2623 florine2623

Labels
QA ✔️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Not used file in the productcomments module
5 participants
@leemyongpakvn @kpodemski @matks @jolelievre @florine2623