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

Replace jQueryAJAX by FetchAPI #189

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Replace jQueryAJAX by FetchAPI #189

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d10632a
replace jQueryAJAX by FetchAPI
leemyongpakvn Oct 13, 2023
c72a8c5
lint fix
leemyongpakvn Oct 13, 2023
1a6a64a
use application/x-www-form-urlencoded to avoid changes in PHP code
leemyongpakvn Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion controllers/front/ReportComment.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@leemyongpakvn leemyongpakvn Oct 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

$decoded = json_decode($content, true);
$id_product_comment = (int) $decoded['id_product_comment'];

/** @var EntityManagerInterface $entityManager */
$entityManager = $this->container->get('doctrine.orm.entity_manager');
Expand Down
6 changes: 4 additions & 2 deletions controllers/front/UpdateCommentUsefulness.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,10 @@ public function display()
return false;
}

$id_product_comment = (int) Tools::getValue('id_product_comment');
$usefulness = (bool) Tools::getValue('usefulness');
$content = trim(file_get_contents('php://input'));
$decoded = json_decode($content, true);
$id_product_comment = (int) $decoded['id_product_comment'];
$usefulness = (bool) $decoded['usefulness'];

/** @var EntityManagerInterface $entityManager */
$entityManager = $this->container->get('doctrine.orm.entity_manager');
Expand Down
56 changes: 39 additions & 17 deletions views/js/list-comments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,18 @@ jQuery(document).ready(function () {
commentsList.append($comment);
}

function updateCommentUsefulness($comment, commentId, usefulness) {
$.post(updateCommentUsefulnessUrl, {id_product_comment: commentId, usefulness: usefulness}, function(jsonData){
if (jsonData) {
async function updateCommentUsefulness($comment, commentId, usefulness) {
try {
const response = await fetch(updateCommentUsefulnessUrl, {
method: "POST",
headers: {
"Content-Type": "application/json",
},
body: JSON.stringify({id_product_comment: commentId, usefulness: usefulness}),
});

if (response.status === 200) {
const jsonData = await response.json();
if (jsonData.success) {
$('.useful-review-value', $comment).html(jsonData.usefulness);
$('.not-useful-review-value', $comment).html(jsonData.total_usefulness - jsonData.usefulness);
Expand All @@ -193,9 +202,9 @@ jQuery(document).ready(function () {
} else {
showUpdatePostCommentErrorModal(productCommentUpdatePostErrorMessage);
}
}).fail(function() {
showUpdatePostCommentErrorModal(productCommentUpdatePostErrorMessage);
});
} catch (error) {
showUpdatePostCommentErrorModal(error);
}
}

function confirmCommentAbuse(commentId) {
Expand All @@ -204,20 +213,33 @@ jQuery(document).ready(function () {
if (!confirm) {
return;
}
$.post(reportCommentUrl, {id_product_comment: commentId}, function(jsonData){
if (jsonData) {
if (jsonData.success) {
reportCommentPostedModal.modal('show');
} else {
showReportCommentErrorModal(jsonData.error);
}
confirmCommentAbuseFetch(commentId);
})
}

async function confirmCommentAbuseFetch(commentId) {
try {
const response = await fetch(reportCommentUrl, {
method: "POST",
headers: {
"Content-Type": "application/json",
},
body: JSON.stringify({id_product_comment: commentId}),
});

if (response.status === 200) {
const jsonData = await response.json();
if (jsonData.success) {
reportCommentPostedModal.modal('show');
} else {
showReportCommentErrorModal(productCommentAbuseReportErrorMessage);
showReportCommentErrorModal(jsonData.error);
}
}).fail(function() {
} else {
showReportCommentErrorModal(productCommentAbuseReportErrorMessage);
});
})
}
} catch (error) {
showReportCommentErrorModal(error);
}
}

if (totalPages <= 1)
Expand Down