-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Make product SKU not null #32532
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
Make product SKU not null #32532
Conversation
Hi @ihor-sviziev. 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 |
@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.
Hi @ihor-sviziev, the change make total sense for me, however, it could be not backward compatible as
The following is a list of prohibited DB Schema changes:
Modifying field type, default value, or property
As we are changing the type from basically ?string
to string
Do you know somebody who we can ask for guidance?
Alternatively, we may target it to the 2.5-develop
branch.
@sidolov @sivaschenko what do you think about this PR? Can we accept it? |
I agree that there is no need for tests as product SKU piece should be covered many-many times and it doesn't make sense to inject the wrong value into DB to trigger the error |
Hi @sidolov, thank you for the review. |
Hi @hws47a, thank you for the review. |
Hi @ihor-sviziev, thank you for your contribution! |
Description (*)
Fixes issue when we weren't able to create product with null SKU through magento, but still could make it in DB. That's a mistake for sure that we should fix.
I think test coverage isn't needed for this case, as Magento itself worked fine and you weren't able to create such a product from it.
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)