fix seopro and check not empty email #13
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses multiple issues including SEO Pro fixes, email validation improvements, and UI updates. The PR aims to fix the SEO Pro library's AJAX detection and add validation to prevent empty or whitespace-only email addresses in forgotten password forms.
Changes:
- Added
detectAjax()call in seopro validate() method to properly detect AJAX requests - Enhanced email validation to check for empty/whitespace emails in both admin and catalog forgotten password controllers
- Updated branding from "ocStore" to "LiveStore" in installation header
- Fixed JavaScript variable handling in chart dashboard component
- Added dynamic form title text for blog article and category forms
- Minor code style improvements (spacing, indentation)
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| upload/system/library/seopro.php | Added detectAjax() call in validate() method to detect AJAX requests |
| upload/install/view/template/common/header.twig | Updated branding from ocStore to LiveStore |
| upload/catalog/controller/account/forgotten.php | Added trim() check to prevent empty/whitespace-only emails |
| upload/admin/view/template/extension/dashboard/chart_by_country_and_region_info.twig | Improved data handling with fallback value and code reuse |
| upload/admin/view/template/blog/article_form.twig | Changed static text_edit to dynamic text_form variable |
| upload/admin/controller/extension/dashboard/chart_by_country_and_region.php | Fixed code spacing consistency |
| upload/admin/controller/common/forgotten.php | Added trim() check to prevent empty/whitespace-only emails |
| upload/admin/controller/blog/category.php | Fixed indentation and added text_form variable initialization |
| upload/admin/controller/blog/article.php | Added text_form variable initialization for dynamic form titles |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| protected function validate() { | ||
| if (!isset($this->request->post['email'])) { | ||
| if (!isset($this->request->post['email']) || !trim($this->request->post['email'])) { |
There was a problem hiding this comment.
After checking that the email is not empty with trim(), the subsequent code still uses the untrimmed email value $this->request->post['email'] on lines 74, 79, and potentially line 21. This means that an email with only whitespace would fail the empty check, but downstream code would still receive a whitespace-only string. Consider trimming the email value once at the beginning of the validation method and storing it in a variable, or trim it before using it in database queries.
| if (!isset($this->request->post['email']) || !trim($this->request->post['email'])) { | ||
| $this->error['warning'] = $this->language->get('error_email'); | ||
| } elseif (!$this->model_user_user->getTotalUsersByEmail($this->request->post['email'])) { |
There was a problem hiding this comment.
After checking that the email is not empty with trim(), the subsequent code still uses the untrimmed email value $this->request->post['email'] on lines 21, 68. This means that an email with leading/trailing whitespace would pass the empty check, but downstream code would still receive the email with whitespace. Consider trimming the email value once at the beginning of the validation method and storing it in a variable, or trim it before using it in database queries.
| if (!isset($this->request->post['email']) || !trim($this->request->post['email'])) { | |
| $this->error['warning'] = $this->language->get('error_email'); | |
| } elseif (!$this->model_user_user->getTotalUsersByEmail($this->request->post['email'])) { | |
| $email = ''; | |
| if (isset($this->request->post['email'])) { | |
| $email = trim($this->request->post['email']); | |
| // Normalize the email so subsequent uses (including in index()) see the trimmed value | |
| $this->request->post['email'] = $email; | |
| } | |
| if ($email === '') { | |
| $this->error['warning'] = $this->language->get('error_email'); | |
| } elseif (!$this->model_user_user->getTotalUsersByEmail($email)) { |
| } | ||
|
|
||
| public function validate() { | ||
| $this->detectAjax(); |
There was a problem hiding this comment.
The detectAjax() method is called before $this->request is initialized. The method accesses $this->request->server on line 539, but $this->request is not set until line 37. This will cause a fatal error "Attempt to read property 'server' on null" when the constructor is executed. Either remove this call from line 29 or move it after the request object is initialized.
| dataType: 'json', | ||
| success: function(json) { | ||
| if(json.data.length > 1) { | ||
| data = json.data || 0; |
There was a problem hiding this comment.
The variable data is assigned using a fallback value of 0 (a number), but it's immediately used with .length property which is an array method. If json.data is undefined, data will be 0, and accessing 0.length will result in undefined (not an error, but may lead to unexpected behavior). Consider using an empty array [] as the fallback value instead to ensure consistent behavior with the .length checks.
| data = json.data || 0; | |
| data = json.data || []; |
| dataType: 'json', | ||
| success: function(json) { | ||
| if(json.data.length > 1) { | ||
| data = json.data || 0; |
There was a problem hiding this comment.
The variable data is declared without const, let, or var, making it an implicit global variable. This can lead to unintended side effects and conflicts with other code. Declare it with const or let to properly scope the variable.
| data = json.data || 0; | |
| const data = json.data || 0; |
1 participant
No description provided.