Enhancement/#11541 - Decouple Google Tag Gateway health status from GTG settings #11881
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… and update related methods.
…ved health monitoring. Update methods to utilize health state data and enhance health check functionality.
…e management. Add new endpoints for gtg-health and gtg-health-checks. Update existing methods to return combined health data for backward compatibility.
…am and Mpath health status. Update related methods for consistency across ads, analytics-4, and tagmanager modules.
… status checks. Replace previous health checks with upstream and Mpath health checks.
…Google_Tag_Gateway_Controller.
…ust state management. Replace deprecated server requirement checks with upstream and Mpath health checks.
… and adjust test cases for upstream and Mpath health statuses.
…ed settings and incorporate new GTG health checks for upstream and Mpath statuses.
…t checks with new health checks for upstream and Mpath statuses across ads and analytics-4 modules.
…ream and Mpath statuses.
…nt new GTG health checks for upstream and Mpath statuses, removing deprecated settings.
… health check status wording.
…ecks and settings.
…for improved health checks.
…ons and remove deprecated properties.
…ag_Gateway_Health and update health check assertions.
…Gateway settings and health data.
|
Storybook is ready:
|
|
Build files for 8528386 are ready:
|
|
Size Change: +72 B (0%) Total Size: 2.23 MB ℹ️ View Unchanged
|
…or deprecation handling in is_google_tag_gateway_active test.
…g deprecated options to prevent issues during updates.
…health fields for pre-migration state simulation.
…s on isUpstreamHealthy and isMpathHealthy fields.
…ions. Initialize health data with default null values before migration.
aaemnnosttv
requested changes
Dec 19, 2025
Collaborator
aaemnnosttv
left a comment
aaemnnosttv
left a comment
There was a problem hiding this comment.
Thanks @hussain-t ! This is off to a good start and there is a lot to review here so there may be more but I thought I'd do a first pass and let you iterate on it to avoid being too much of a bottleneck.
Comment on lines
79
to
80
| $this->assertFalse( $this->options->has( Google_Tag_Gateway_Settings::OPTION ), 'GTG settings should not be created if they did not exist.' ); | ||
| $this->assertFalse( $this->options->has( Google_Tag_Gateway_Health::OPTION ), 'GTG health should not be created if settings did not exist.' ); |
Collaborator
There was a problem hiding this comment.
We have dedicated custom assertions for testing the existence of an option. See assertOption(Not)Exists
Comment on lines
93
to
95
| // Bypass sanitization to allow setting legacy health fields (which are stripped by the current class) | ||
| // in order to simulate the pre-migration state. | ||
| remove_all_filters( 'sanitize_option_' . Google_Tag_Gateway_Settings::OPTION ); |
Collaborator
There was a problem hiding this comment.
It might be cleaner to do this before the settings class is registered.
Comment on lines
141
to
142
| // Verify settings option was deleted. | ||
| $this->assertFalse( |
Collaborator
There was a problem hiding this comment.
Please use the dedicated assertions for option existence
| ); | ||
| } | ||
|
|
||
| public function test_migrate_preserves_db_version() { |
Collaborator
There was a problem hiding this comment.
This sounds like we expect the db version not to be set?
Collaborator
Author
There was a problem hiding this comment.
You're right - the name test_migrate_preserves_db_version is misleading. I've renamed it to test_migrate_sets_db_version to reflect what it's testing.
| * @since n.e.x.t | ||
| */ | ||
| protected function migrate_gtg_health_data() { | ||
| // Check if GTG settings exist using the Options::has() method. |
Collaborator
There was a problem hiding this comment.
Please save comments for adding content or explaining why we're doing something that may not be obvious. This comment simply restates the next line which is already clear.
Suggested change
| // Check if GTG settings exist using the Options::has() method. |
| } | ||
|
|
||
| // Get the existing GTG settings directly from options to preserve old health properties. | ||
| // Using $this->gtg_settings->get() would apply sanitization and strip out the old properties. |
Collaborator
There was a problem hiding this comment.
Are you sure? Sanitization is applied on write, not read.
Collaborator
Author
There was a problem hiding this comment.
You're right - my bad. That was added in an inappropriate place.
Comment on lines
135
to
148
| // Migrate health data to new health option. | ||
| // Start with default null values, then override with migrated values if they exist. | ||
| $health_data = array( | ||
| 'isUpstreamHealthy' => null, | ||
| 'isMpathHealthy' => null, | ||
| ); | ||
|
|
||
| if ( array_key_exists( 'isGTGHealthy', $gtg_settings ) ) { | ||
| $health_data['isUpstreamHealthy'] = $gtg_settings['isGTGHealthy']; | ||
| } | ||
|
|
||
| if ( array_key_exists( 'isScriptAccessEnabled', $gtg_settings ) ) { | ||
| $health_data['isMpathHealthy'] = $gtg_settings['isScriptAccessEnabled']; | ||
| } |
Collaborator
There was a problem hiding this comment.
I think this could be simplified into the definition of the array.
ie. newKey => $old[oldKey] ?? null
Collaborator
Author
There was a problem hiding this comment.
Great suggestion - thanks!
| $health_data['isMpathHealthy'] = $gtg_settings['isScriptAccessEnabled']; | ||
| } | ||
|
|
||
| // Save migrated health data. |
Collaborator
There was a problem hiding this comment.
Suggested change
| // Save migrated health data. |
| // This avoids issues where sanitization safeguards might preserve old keys during an update. | ||
| $this->options->delete( Google_Tag_Gateway_Settings::OPTION ); | ||
|
|
||
| if ( isset( $gtg_settings['isEnabled'] ) && true === $gtg_settings['isEnabled'] ) { |
Collaborator
There was a problem hiding this comment.
Suggested change
| if ( isset( $gtg_settings['isEnabled'] ) && true === $gtg_settings['isEnabled'] ) { | |
| if ( ! empty( $gtg_settings['isEnabled'] ) ) { |
…gacy data to avoid sanitization issues.
…implifying the code and removing unnecessary checks.
…ue instead of strict true.
…s to clarify the process of re-saving isEnabled option.
Collaborator
Author
|
Thanks for your kind review, @aaemnnosttv! Back to you for another review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist