-
Notifications
You must be signed in to change notification settings - Fork 874
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
Move generic site data into self contained objects. #21368
base: trunk
Are you sure you want to change the base?
Move generic site data into self contained objects. #21368
Conversation
Pull Request Test Coverage Report for Build 4dd641601cbdd7bf5701bd449c4dfe26ccbbaec5Details
💛 - Coveralls |
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.
admin/taxonomy/class-taxonomy.php
Outdated
*/ | ||
$repo = YoastSEO()->classes->get( Website_Information_Repository::class ); | ||
$repo->get_term_site_information()->set_term( get_term_by( 'id', $tag_id, $this::get_taxonomy() ) ); | ||
$script_data = array_merge_recursive( $repo->get_term_site_information()->get_site_information(), $script_data ); |
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.
Let's assign the $repo->get_term_site_information()
result to a variable, so as to not call it twice here and in the line above.
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 are we creating a repository that just returns instances of a couple of classes? We could just instantiate them in the metabox classes, without the use of a repository.
Unless I'm missing something...
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.
I added this because I wanted to follow the same as I did for all the other script data code, and I wanted the entry point for this to be in the application folder and not in the framework folder to more adhere to the Onion way.
'dismissedAlerts' => $dismissed_alerts, | ||
'currentPromotions' => $this->promotion_manager->get_current_promotions(), | ||
'webinarIntroBlockEditorUrl' => $this->short_link_helper->get( 'https://yoa.st/webinar-intro-block-editor' ), | ||
'blackFridayBlockEditorUrl' => ( $this->promotion_manager->is( 'black-friday-2023-checklist' ) ) ? $this->short_link_helper->get( 'https://yoa.st/black-friday-checklist' ) : '', |
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.
All these are post-specific and shouldn't be calculated on taxonomy loads.
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Only
Should be different between posts and terms.
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo]
, added test instructions for Shopify and attached theShopify
label to this PR.Documentation
Quality assurance
Innovation
innovation
label.Fixes #