Replace jQueryAJAX by FetchAPI #189

leemyongpakvn · 2023-10-13T02:43:01Z

Questions	Answers
Description?	old jQuery AJAX need to be replaced by modern and native FetchAPI. More details in the related discussion.
Type?	improvement
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes PrestaShop/PrestaShop/discussions/34239
How to test?	Apply PR changes then check to make sure that Review's Upvote / Downvote and Report Abuse functions work as usual in both FO and BO. (Don't forget to use a clean dev branch, Browser clear cache, BO clear cache before testing ;)

matks · 2023-10-13T15:32:23Z

controllers/front/ReportComment.php

@@ -47,7 +47,9 @@ public function display()
            return false;
        }

-        $id_product_comment = (int) Tools::getValue('id_product_comment');
+        $content = trim(file_get_contents('php://input'));


Hello, if you insert this code, does that men Tools::getValue() is not capable of handling one specific usecase? Then what is this usecase?

First, when replace jQuery Ajax by Fetch API in list-comment.js, we need replace Tools::getValue by file_get_contents('php://input') in ReportComment.php accordingly, because Fetch Post is sent as application/json not application/x-www-form-urlencoded nor multipart/form-data
Second, according to from-Legacy-to-Future-architecture's Homogenize the FO & BO architecture, Tools is an ancient legacy-based component, so in this case Tools removing just double the benefit of this PR. I really replace two legacy library usage by two native functions 😆

Thank you for the explanations.

Mmmm 🤔 so that means that every module developer who wants to use Fetch will have to do the same. So we can expect a lot of $content = trim(file_get_contents('php://input')); that appears... until Symfony can be used on the front (because I'm quite sure Symfony Request object can handle this very well)

In my opinion this is way overkill, what's the advantage of using fetch with a JSON body content?

Replacing $.post by fetch is a good call, but you can use fetch to perform a regular POST request with form vars instead of JSON body, it will simplify both the JS and PHP code

I just follow this manual https://developer.mozilla.org/en-US/docs/Web/API/Fetch_API/Using_Fetch . And I think we are in a Listing context not a Form context here.

That's not the point, request with form vars are performed on many APIs regardless of what they are requesting, or the context of the request The FormData here is actually the JS native way to send POST parameters, nothing more, and POST variables are more convenient to use in PHP on the BO side

Requests based on JSON bodies are relevant for REST APIs for example, or in JS ecosystem because most framework already handle them natively. We are also using a JSON bdy in our new API under development, but because we are in a Symfony framework and also relying on APIPlatform which handles the parsing under the hood

But for native PHP in a legacy controller it's not really among the usual practices Most of the time simple API endpoints in PHP rely on $_POST variables, or at least all our current legacy controllers do

We have no easy tool to get a JSON body request on legacy controller, that's why you had to "hard code" the fetching and parsing based on php://input But I'm pretty sure the next developer who will see this won't even understand what's it used for 🤷‍♂️

So in my opinion, but I'm not the only one to decide, the PR should only focus on refactoring the JS side, as your PR title suggests, without impacting the server side controller So instead of sending a JSON body it should send POST variables and my suggestion simply shows how it's done in vanilla JS

Alright. If you are so worried, we can switch this PR to draft, and continue to discuss more in the related discussion. Wdyt @matks ?

As @jolelievre is thinking about module developer experience I quite approve his opinion 👍

OK. Let's postpone this risky PR. I will find another way.

leemyongpakvn · 2023-10-19T01:21:34Z

@AureRita This PR is approved technically by devs. So QA just need to check following How to test ? as an end-user.

leemyongpakvn · 2023-10-26T07:41:31Z

@matks , @matthieu-rolland & @jolelievre application/json is optional for Fetch API POST Content-Type, not required as I thought before. Using application/x-www-form-urlencoded can avoid changes in PHP code now. Please review again.

jolelievre · 2023-10-26T07:50:19Z

Thanks @leemyongpakvn

matks · 2023-10-27T08:45:56Z

I put back the "Waiting for QA" label 😉

florine2623

Hello @leemyongpakvn ,

I encountered an exception while testing your PR, only reproducible if I don't clear cache from PS and browser.

Steps to reproduce :

In BO, Enable upvotes / downvotes on reviews
In FO, log into your customer account
Choose a product > Add a comment
Report abuse
In BO > Approve the abusive comment
In FO, report abuse again
In BO > Delete the abusive comment
Exception is displayed
PrestaShop\Module\ProductComment\Repository\ProductCommentRepository::delete(): Argument #1 ($entity) must be of type PrestaShop\Module\ProductComment\Entity\ProductComment, null given, called in /Users/fHea/Desktop/ps800/modules/productcomments/productcomments.php on line 210

@PrestaShop/prestashop-core-developers what do you think ? Approved or not ?

leemyongpakvn · 2023-10-28T03:37:01Z

@florine2623 There are 2 different problems here:

This PR only affects Review's Upvote / Downvote and Report Abuse functions. It means we just need to make sure: logged-in user can Upvote / Downvote a comment (see count increased after voted) in FO; logged-in user can Report a comment as Abusive in FO and administrator can SEE that Report in BO; non logged in user can not do Upvote / Downvote nor Report Abusive in FO.
The exception you report is another problem, it is APPROVE / DELETE abusive comment by administrator in BO. I'm pretty sure that you can reproduce it without applying this PR change, just need to upgrade productcomments from 5.0.3 to 6.0.2, old doctrine cache will generate this exception after that.

matks · 2023-11-09T17:20:58Z

Hello @florine2623 the bug you have detected is not related to this Pull Request. I was able to reproduce it using PrestaShop develop branch and the current version of productcomments module, 6.0.2

I have submitted a bug report about it PrestaShop/PrestaShop#34512

This PR works as expected according to my tests 👍 and the problem you have encountered is not related to this PR. Do you think we can merge the PR?

florine2623

Hello @leemyongpakvn ,

Yes indeed, I have reproduced the issue without your PR as well ^^

It is QA ✅ !
Thanks!

leemyongpakvn added 2 commits October 13, 2023 09:03

replace jQueryAJAX by FetchAPI

d10632a

lint fix

c72a8c5

matks reviewed Oct 13, 2023

View reviewed changes

matks approved these changes Oct 14, 2023

View reviewed changes

ps-jarvis added the waiting for QA label Oct 14, 2023

matthieu-rolland approved these changes Oct 16, 2023

View reviewed changes

AureRita added the waiting for dev label Oct 18, 2023

AureRita removed the waiting for dev label Oct 19, 2023

leemyongpakvn marked this pull request as draft October 23, 2023 12:53

leemyongpakvn removed the waiting for QA label Oct 23, 2023

use application/x-www-form-urlencoded to avoid changes in PHP code

1a6a64a

leemyongpakvn marked this pull request as ready for review October 26, 2023 07:36

jolelievre approved these changes Oct 26, 2023

View reviewed changes

matks added the waiting for QA label Oct 27, 2023

florine2623 self-assigned this Oct 27, 2023

florine2623 reviewed Oct 27, 2023

View reviewed changes

florine2623 added the waiting for dev label Oct 27, 2023

florine2623 removed their assignment Oct 27, 2023

matks approved these changes Nov 9, 2023

View reviewed changes

matks removed the waiting for dev label Nov 9, 2023

boherm approved these changes Nov 13, 2023

View reviewed changes

florine2623 self-assigned this Nov 14, 2023

florine2623 approved these changes Nov 14, 2023

View reviewed changes

florine2623 removed the waiting for QA label Nov 14, 2023

florine2623 added the QA ✔️ label Nov 14, 2023

nicosomb added this to the 6.0.3 milestone Nov 14, 2023

nicosomb merged commit fae55dc into PrestaShop:dev Nov 14, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace jQueryAJAX by FetchAPI #189

Replace jQueryAJAX by FetchAPI #189

leemyongpakvn commented Oct 13, 2023 •

edited

Loading

matks Oct 13, 2023

leemyongpakvn Oct 14, 2023 •

edited

Loading

matks Oct 14, 2023

jolelievre Oct 18, 2023

leemyongpakvn Oct 19, 2023

jolelievre Oct 19, 2023

leemyongpakvn Oct 19, 2023

matks Oct 23, 2023

leemyongpakvn Oct 23, 2023

leemyongpakvn commented Oct 19, 2023

leemyongpakvn commented Oct 26, 2023

jolelievre commented Oct 26, 2023

matks commented Oct 27, 2023

florine2623 left a comment

leemyongpakvn commented Oct 28, 2023 •

edited

Loading

matks commented Nov 9, 2023

florine2623 left a comment

Replace jQueryAJAX by FetchAPI #189

Replace jQueryAJAX by FetchAPI #189

Conversation

leemyongpakvn commented Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

leemyongpakvn Oct 14, 2023 • edited Loading

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 Oct 19, 2023

leemyongpakvn commented Oct 26, 2023

jolelievre commented Oct 26, 2023

matks commented Oct 27, 2023

florine2623 left a comment

Choose a reason for hiding this comment

leemyongpakvn commented Oct 28, 2023 • edited Loading

matks commented Nov 9, 2023

florine2623 left a comment

Choose a reason for hiding this comment

leemyongpakvn commented Oct 13, 2023 •

edited

Loading

leemyongpakvn Oct 14, 2023 •

edited

Loading

leemyongpakvn commented Oct 28, 2023 •

edited

Loading