-
Notifications
You must be signed in to change notification settings - Fork 21
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
[GTIN] Add Migration support for YOAST SEO #2677
base: add/support-core-gtin-field
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## add/support-core-gtin-field #2677 +/- ##
===============================================================
- Coverage 64.9% 64.9% -0.0%
- Complexity 4667 4671 +4
===============================================================
Files 478 478
Lines 19517 19527 +10
===============================================================
Hits 12674 12674
- Misses 6843 6853 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Thanks for providing the compatibility. I tested it and it does work as expected, however I think it would be nicer to keep the integration functionality encapsulated in the one class.
* @param WC_Product $product The product | ||
* @return string|null | ||
*/ | ||
protected function get_yoast_gtin( WC_Product $product ): ?string { |
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 structure does work, but it wouldn't be my preferred choice, as it requires the migration to know details about Yoast compatibility as well as having access to public methods.
While we only added support for Yoast in theory there could be other plugins which add support for their structure. Which is why each class in the Integrations folder was implemented in such a way that in theory it could live both in the plugin and outside the plugin (maintained by a 3PD).
I think it would be better to keep it that way and add any necessary hooks so we don't need to make parts of the Yoast integration classes public.
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.
So just add a filter in get_gtin
so we don't need to add anything in regards to Yoast here. Then in the class YoastWooCommerceSeo
we can hook into the filter for migration and if it has a value to return it can do so.
Changes proposed in this Pull Request:
Closes #2665
This PR adds support for the YOAST GTIN field in the GTIN migration feature
Screenshots:
Detailed test instructions:
wp wc g4wc gtin-migration start --debug
Additional details:
Changelog entry