-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Update Module.php #29695
Update Module.php #29695
Conversation
I was seeing the error in my local for this "," comma: PHP Parse error: syntax error, unexpected ')' in setup\src\Magento\Setup\Module.php on line 82
Hi @tejasklevu. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@tejasklevu
@magento run all tests |
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.
@tejasklevu
From first look - looks good, but actually trailing coma in function calls was implemented in php 7.3 that minumum supported version in magento 2.4.x, so not sure why are you getting this issue.
Could you add your used php version?
Could you sign Adobe CLA?
Hi @ihor-sviziev i checked again found my PHP CLI version was PHP 7.2.X and that created this issue. "The comma after the last array element is optional and can be omitted. This is usually done for single-line arrays, i.e. array(1, 2) is preferred over array(1, 2, ). For multi-line arrays on the other hand the trailing comma is commonly used, as it allows easier addition of new elements at the end." So as a code improvement, this pull request can be accepted? |
Hi @tejasklevu,
I prefer to reject this PR as issue isn't reproducing on the supported php versions and basically this change isn't doing any improvement. |
Hi @tejasklevu, thank you for your contribution! |
A comma in the array when it is ending
Description (*)
I tried to run normal Upgrade command on CLI and got the error as below:
PHP Parse error: syntax error, unexpected ')' in setup\src\Magento\Setup\Module.php on line 82
After seeing the code, there was unnecessary comma present in the code that I removed in my pull request. This fixed my issue as well.
Manual testing scenarios (*)
Contribution checklist (*)
Fixes #29756