Migrate from ObjectModel to Doctrine (final) #177

leemyongpakvn · 2023-07-24T08:09:16Z

Questions	Answers
Description?	Please check details in the related issue.
Type?	refacto
BC breaks?	yes
Deprecations?	yes
Fixed ticket?	Fixes PrestaShop/PrestaShop#33055
How to test?	Apply PR changes; Clear web browser cache & BO > Advanced Params > Performance > Clear Cache. Check to make sure module works as usual in BO and FO.

matks

Hi @leemyongpakvn and bravo for this PR. I suggested some little things 😉 .

Also I see you have a lot of commits with the same commit message can you squash them?

And I'm afraid there are git conflicts

src/Repository/ProductCommentCriterionRepository.php

src/Repository/ProductCommentRepository.php

… Doctrine

leemyongpakvn · 2023-08-04T14:54:35Z

@matks Rebased and applied your suggestion.

kpodemski · 2023-08-06T15:20:30Z

src/Repository/ProductCommentCriterionRepository.php

+    public function getCategories(int $id_criterion)
+    {
+        $sql = '
+			SELECT pccc.id_category, pccc.id_product_comment_criterion


Suggested change

SELECT pccc.id_category, pccc.id_product_comment_criterion

SELECT pccc.id_category, pccc.id_product_comment_criterion

there's something wrong with indentation, I'm surprised cs fixer didn't catch it, you should indent using spaces, also the indentation itself is wrong for the queries

Indentation and tabs are copied from existing ObjectModel code. I have just replaced tabs by spaces. Be free to change queries' indentation if you think it is needed.

We know php-cs-fixer and php-stan are not reliable in some cases.

kpodemski · 2023-08-06T15:21:03Z

src/Repository/ProductCommentCriterionRepository.php

+     */
+    public function getTypes()
+    {
+        $sfTranslator = SymfonyContainer::getInstance()->get('translator');


You could inject translator service in the construtor

I tried inserting @translator in common.yml but got this error

Fatal error: Uncaught Symfony\Component\DependencyInjection\Exception\ServiceNotFoundException: The service "product_comment_criterion_repository" has a dependency on a non-existent service "translator".

Then I discovered that ps_linklist uses @translator in its services, Symfony\Contracts\Translation\TranslatorInterface in its repository, and dropped support for both PS 1.7.8, PS 8.0 😢

With the upgrade to symfony 4, Symfony\Component\Translation\TranslatorInterface is transferred to Symfony\Contracts\Translation\TranslatorInterface. On the other hand, the translator service is still present, it has just changed the package.

Injecting translator service create error now so I'd prefer try to do it in the next minor version.

kpodemski · 2023-08-06T15:21:29Z

src/Repository/ProductCommentRepository.php

+ * @method ProductComment|null find($id, $lockMode = null, $lockVersion = null)
+ * @method ProductComment|null findOneBy(array $criteria, array $orderBy = null)
+ * @method ProductComment[] findAll()
+ * @method ProductComment[] findBy(array $criteria, array $orderBy = null, $limit = null, $offset = null)


I'm not a fun of such comments, what's the purpose of them?

They are generated automatically when you run Symfony make:entity in terminal . I think it is useful in case someone try to reinvent the wheel ;)

src/Repository/ProductCommentRepository.php

matks

Hi !

I have 1 good piece of news and 1 bad 😞

The good news: for me this PR is valid 👍 I can approve it

The bad: this PR is modifying ProductComments module Doctrine entities, right?

Last week we released GDPR 2.0.1 module. This release also also modify module Doctrine entities. And it started to create failures for people trying to upgrade PrestaShop 8.x. So the module was rollbacked.

Right now, the problem is under analyzis but the main culprit is that the way modules handle Doctrine schema updates migth break the shop upgrade process.

So ... if we put this PR #177 inside release 6.0.0 of Productcomments, the same thing might happen. It might start crashing people shop upgrades 😞 .

So I'm really really really sad 😭 to say this but I'm afraid we should wait for the resolution of this upgrade problem before merging this PR. Because of the Doctrine entities being changed.

kpodemski · 2023-08-22T09:52:53Z

@matks when I check the code inside the class which is inside Entity directory, I don't see anything that could trigger schema upgrade

leemyongpakvn · 2023-08-22T11:48:56Z

I see 2 good news from matks's bad news 😆
First, the community can avoid a possible crash that is still under analyzed.
Second, I can bump the first version 7 in native modules next month.

matks · 2023-08-22T12:38:18Z

Wait I might be wrong 😄

https://github.com/PrestaShop/productcomments/blob/c85529e874056c82f68ab7daae0104ea3d26db5d/src/Entity/ProductCommentCriterion.php

@leemyongpakvn you added private properties in the Entity BUT you did not map them to SQL column so the Doctrine entities are NOT modified, right?

leemyongpakvn · 2023-08-22T13:21:42Z

@matks Correct, the ProductCommentCriterion Entity is MODIFIED, but 3 additional private properties is NOT mapped to SQL columns so as @kpodemski said

I don't see anything that could trigger schema upgrade

matks

Then let's ask QA team to test this PR 💪 we move to the next stage

PululuK

Small feedback :)

PululuK · 2023-08-22T20:28:20Z

src/Entity/ProductCommentCriterion.php

+    public function __construct()
+    {
+        $langIsoIds = Language::getIsoIds();
+        $langString = '';


Suggested change

$langString = '';

Remove unused variable :)

PululuK · 2023-08-22T20:33:24Z

src/Entity/ProductCommentCriterion.php

+    public function isValid()
+    {
+        $res = true;
+        foreach ($this->names as $key => $value) {
+            $res &= Validate::isGenericName($value);
+        }
+
+        return $res;
+    }


Suggested change

public function isValid()

{

$res = true;

foreach ($this->names as $key => $value) {

$res &= Validate::isGenericName($value);

}

return $res;

}

public function isValid()

{

foreach ($this->getNames() as $value) {

if(!Validate::isGenericName($value)) {

return false;

}

}

return true;

}

Performances improvement :)

$this->getNames() returns $this->names in fact, so I think we simply need removing unused $key

@leemyongpakvn You don't need to loop all the names... just have a false for the condition to return false

@PululuK You're right. Applied :)

leemyongpakvn · 2023-08-23T01:46:25Z

@matks In need of re-approve after removing unused variables (small tweak, no logic change ;)

hibatallahAouadni · 2023-08-23T09:33:08Z

@PululuK @M0rgan01 @kpodemski we need second approval on this PR 🙏

kpodemski · 2023-08-23T09:41:23Z

@hibatallahAouadni modules don't require two approvals, it's ok 👍🏻

hibatallahAouadni

Hello @leemyongpakvn

Thanks for your PR 🚀
All module's functionalities works like expected ✅
@matks about the upgrade pb, do you want me test the upgrade with this PR? to check if there will be problems?
Waiting for your feedback.

Thanks!

hibatallahAouadni

As discussed with @kpodemski no need to test the upgrade 😉

leemyongpakvn requested a review from SD1982 July 24, 2023 10:49

leemyongpakvn closed this Jul 24, 2023

leemyongpakvn reopened this Jul 24, 2023

leemyongpakvn requested a review from SharakPL July 24, 2023 13:47

SharakPL requested review from a team and removed request for SharakPL July 24, 2023 14:02

matks requested changes Aug 4, 2023

View reviewed changes

ps-jarvis added the waiting for author label Aug 4, 2023

leemyongpakvn mentioned this pull request Aug 4, 2023

Release 6.0 #180

Merged

leemyongpakvn added 12 commits August 4, 2023 19:36

Doctrine ServiceEntityRepository need registry argument

bbc5315

migrate const, getter, setter, isser from ObjectModel to Doctrine Entity

28c0d62

migrate from ObjectModel to Doctrine ServiceEntityRepository

dea9aea

migrate from ObjectModel to Doctrine ServiceEntityRepository

0473149

mark migrated functions from v4.0.0 and v6.0.0

065f4ea

ignoreErros for outdated phpstan-doctrine

06289d4

as suggested by @matks avoid magic number and single letter variable

0e02c4f

remove indirect Repository use, remove ObjectModel include, switch to…

8e39ee6

… Doctrine

compatible fetch for both doctrine 2.7 and 3.6

42aeafe

cs-fix

3f9daf2

missing param name change

6a16a01

as suggested by @matks: no need to typecast back and forth

da835f5

leemyongpakvn force-pushed the migrate-from-objectmodel-to-doctrinedbal-final branch from 776c07b to da835f5 Compare August 4, 2023 14:47

kpodemski reviewed Aug 6, 2023

View reviewed changes

src/Repository/ProductCommentRepository.php Outdated Show resolved Hide resolved

kpodemski reviewed Aug 6, 2023

View reviewed changes

src/Repository/ProductCommentRepository.php Outdated Show resolved Hide resolved

leemyongpakvn added 2 commits August 7, 2023 09:33

replace old tabs by spaces

a4e6640

remove plan-for-future Doctrine Cache

c85529e

matks reviewed Aug 22, 2023

View reviewed changes

matks approved these changes Aug 22, 2023

View reviewed changes

ps-jarvis added the waiting for QA label Aug 22, 2023

matks removed the waiting for author label Aug 22, 2023

PululuK reviewed Aug 22, 2023

View reviewed changes

remove unused variables

cab74fe

leemyongpakvn added 2 commits August 23, 2023 15:03

better loop :)

f27b7ed

CS fix

8ebec5e

leemyongpakvn closed this Aug 23, 2023

leemyongpakvn reopened this Aug 23, 2023

leemyongpakvn closed this Aug 23, 2023

leemyongpakvn reopened this Aug 23, 2023

hibatallahAouadni self-assigned this Aug 23, 2023

hibatallahAouadni reviewed Aug 23, 2023

View reviewed changes

hibatallahAouadni added waiting for dev and removed waiting for QA labels Aug 23, 2023

PululuK approved these changes Aug 23, 2023

View reviewed changes

hibatallahAouadni added QA ✔️ and removed waiting for dev labels Aug 23, 2023

hibatallahAouadni approved these changes Aug 23, 2023

View reviewed changes

M0rgan01 merged commit 43c6fb1 into PrestaShop:dev Aug 23, 2023
21 of 24 checks passed

MatShir added this to the 6.0.0 milestone Aug 23, 2023

leemyongpakvn deleted the migrate-from-objectmodel-to-doctrinedbal-final branch January 20, 2024 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate from ObjectModel to Doctrine (final) #177

Migrate from ObjectModel to Doctrine (final) #177

leemyongpakvn commented Jul 24, 2023 •

edited

Loading

matks left a comment

leemyongpakvn commented Aug 4, 2023

kpodemski Aug 6, 2023

leemyongpakvn Aug 7, 2023

leemyongpakvn Aug 7, 2023

kpodemski Aug 6, 2023

leemyongpakvn Aug 7, 2023

M0rgan01 Aug 22, 2023

leemyongpakvn Aug 22, 2023 •

edited

Loading

kpodemski Aug 6, 2023

leemyongpakvn Aug 7, 2023

matks left a comment

kpodemski commented Aug 22, 2023

leemyongpakvn commented Aug 22, 2023

matks commented Aug 22, 2023

leemyongpakvn commented Aug 22, 2023

matks left a comment

PululuK left a comment

PululuK Aug 22, 2023

PululuK Aug 22, 2023

PululuK Aug 22, 2023

PululuK Aug 22, 2023

leemyongpakvn Aug 23, 2023

PululuK Aug 23, 2023

leemyongpakvn Aug 23, 2023

leemyongpakvn commented Aug 23, 2023

hibatallahAouadni commented Aug 23, 2023

kpodemski commented Aug 23, 2023

hibatallahAouadni left a comment

hibatallahAouadni left a comment

	SELECT pccc.id_category, pccc.id_product_comment_criterion
	SELECT pccc.id_category, pccc.id_product_comment_criterion

Migrate from ObjectModel to Doctrine (final) #177

Migrate from ObjectModel to Doctrine (final) #177

Conversation

leemyongpakvn commented Jul 24, 2023 • edited Loading

matks left a comment

Choose a reason for hiding this comment

leemyongpakvn commented Aug 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leemyongpakvn Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

kpodemski commented Aug 22, 2023

leemyongpakvn commented Aug 22, 2023

matks commented Aug 22, 2023

leemyongpakvn commented Aug 22, 2023

matks left a comment

Choose a reason for hiding this comment

PululuK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leemyongpakvn commented Aug 23, 2023

hibatallahAouadni commented Aug 23, 2023

kpodemski commented Aug 23, 2023

hibatallahAouadni left a comment

Choose a reason for hiding this comment

hibatallahAouadni left a comment

Choose a reason for hiding this comment

leemyongpakvn commented Jul 24, 2023 •

edited

Loading

leemyongpakvn Aug 22, 2023 •

edited

Loading