-
Notifications
You must be signed in to change notification settings - Fork 11
Address PR #57 review comments: fix SQL injection, simplify CI #58
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.
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
Conversation
Co-authored-by: claudio-ferraro <[email protected]>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot I've added |
I attempted to run the linters after the repository was allowlisted, but encountered SSL connection timeouts when downloading packages from However, I validated the changes using PHP's built-in syntax checker:
The changes in commit 86486b9 are syntactically correct. The full linting (PHPStan + PHPCS) will run automatically via GitHub Actions when the PR is merged or when the CI workflow is triggered. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
| <img src="https://contrib.rocks/image?repo=justbetter/magento2-akeneo-bundle" alt="Contributors" /> | ||
| </a> | ||
|
|
||
| <p align="right">(<a href="#readme-top">back to top</a>)</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove all the back-to-top links? I'd like to keep this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Restored the "back to top" links in commit bb98adc. The README now keeps all navigation links while fixing the typos.
Helper/Import/Product.php
Outdated
| class Product extends BaseProduct | ||
| { | ||
| /** | ||
| * Overwritten to set values for store view 0 when attributes are scopable/localizable and required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is unnecessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the comment in commit bb98adc.
|
@copilot I have reviewed your changes and requested changes |
…omment Co-authored-by: claudio-ferraro <[email protected]>
* Remove unnecessary plugins * Refactor Job plugins to Services and Observers * Refactor codebase and add analyse actions * Refactor codebase | PHPStan lvl 7 * Update readme * Add documentation, community files, copyright headers and code quality tools * Modify FEATURES documentation * Fix PHP 8.4 compatibility and add missing license to composer.json * Refactor copyright headers * Fix feedback * Address PR #57 review comments: fix SQL injection, simplify CI (#58) * Initial plan * Address PR review comments: fix typos, security issues, and code quality Co-authored-by: claudio-ferraro <[email protected]> * Address review feedback: keep back-to-top links, remove unnecessary comment Co-authored-by: claudio-ferraro <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: claudio-ferraro <[email protected]> * Add comment * Fix exclude families plugin by updating method name to getFamiliesToImport * Remove unused config options and update documentation --------- Co-authored-by: Copilot <[email protected]>
Addresses unresolved review comments from PR #57 covering security vulnerabilities, code quality improvements, and documentation fixes.
Security Fixes
SQL Injection in SetTaxClassId.php - User-controlled values were concatenated directly into SQL queries:
strtotime() error handling - Added fallback for potential
falsereturn value in RunSlackMessage.phpCode Quality
Documentation
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.