-
Notifications
You must be signed in to change notification settings - Fork 30
fix: keep usage logger alive when chart settings meta is not an array #1360
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
Open
lucadobrescu
wants to merge
2
commits into
development
Choose a base branch
from
fix/1359-usage-logger-string-settings
base: development
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' ); | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| const { deleteAllCharts } = require( '../utils/common' ); | ||
|
|
||
| /** | ||
| * Regression tests for https://github.com/Codeinwp/visualizer/issues/1359 | ||
| * | ||
| * A published chart whose `visualizer-settings` meta is a string (instead of | ||
| * the sanitized settings array) crashed `Visualizer_Module_Setup::getUsage()` | ||
| * with a PHP 8 TypeError, aborting the whole SDK usage collection request. | ||
| * The logger must tolerate such charts and still report the others. | ||
| */ | ||
| test.describe( 'Usage logger', () => { | ||
| let corruptedId; | ||
| let manualId; | ||
|
|
||
| test.beforeAll( async ( { requestUtils } ) => { | ||
| // The assertions below count charts, so start from a clean library. | ||
| await deleteAllCharts( requestUtils ); | ||
|
|
||
| // A chart whose settings meta is a corrupted string value. | ||
| const corrupted = await requestUtils.rest( { | ||
| method: 'POST', | ||
| path: '/wp/v2/visualizer', | ||
| data: { title: 'Corrupted settings chart', status: 'publish' }, | ||
| } ); | ||
| corruptedId = corrupted.id; | ||
| await requestUtils.rest( { | ||
| method: 'POST', | ||
| path: `/visualizer-e2e/v1/chart-settings/${ corruptedId }`, | ||
| data: { settings: 'corrupted string settings' }, | ||
| } ); | ||
|
|
||
| // A healthy chart with a manual configuration, which must still be counted. | ||
| const manual = await requestUtils.rest( { | ||
| method: 'POST', | ||
| path: '/wp/v2/visualizer', | ||
| data: { title: 'Manual config chart', status: 'publish' }, | ||
| } ); | ||
| manualId = manual.id; | ||
| await requestUtils.rest( { | ||
| method: 'POST', | ||
| path: `/visualizer-e2e/v1/chart-settings/${ manualId }`, | ||
| data: { settings: { manual: '{"colors": ["#000"]}' } }, | ||
| } ); | ||
| } ); | ||
|
|
||
| test.afterAll( async ( { requestUtils } ) => { | ||
| for ( const id of [ corruptedId, manualId ] ) { | ||
| if ( id ) { | ||
| await requestUtils.rest( { method: 'DELETE', path: `/wp/v2/visualizer/${ id }`, params: { force: true } } ); | ||
| } | ||
| } | ||
| } ); | ||
|
|
||
| test( 'survives a chart whose settings meta is a string', async ( { requestUtils } ) => { | ||
| // Before the fix this request died with a TypeError (HTTP 500). | ||
| const usage = await requestUtils.rest( { method: 'GET', path: '/visualizer-e2e/v1/usage' } ); | ||
|
|
||
| expect( usage.manual_config ).toBe( 1 ); | ||
| expect( Object.values( usage.types ).reduce( ( a, b ) => a + b, 0 ) ).toBe( 2 ); | ||
| } ); | ||
| } ); |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| <?php | ||
| /** | ||
| * WordPress unit test plugin. | ||
| * | ||
| * @package visualizer | ||
| * @subpackage Tests | ||
| * @license http://opensource.org/licenses/gpl-2.0.php GNU Public License | ||
| */ | ||
|
|
||
| /** | ||
| * Test the usage logger fed to the Themeisle SDK. | ||
| * | ||
| * Regression tests for https://github.com/Codeinwp/visualizer/issues/1359 — | ||
| * a published chart whose `visualizer-settings` meta is a string crashed | ||
| * `Visualizer_Module_Setup::getUsage()` with a TypeError on PHP 8, aborting | ||
| * scheduled usage collection. | ||
| */ | ||
| class Test_Visualizer_Usage_Logger extends WP_UnitTestCase { | ||
|
|
||
| /** | ||
| * Create a published chart with the given settings meta value. | ||
| * | ||
| * @param mixed $settings The value stored in the visualizer-settings meta. | ||
| * @return int The chart id. | ||
| */ | ||
| private function create_chart( $settings ) { | ||
| $chart_id = self::factory()->post->create( | ||
| array( | ||
| 'post_type' => Visualizer_Plugin::CPT_VISUALIZER, | ||
| 'post_status' => 'publish', | ||
| 'post_content' => wp_slash( serialize( array( array( 'Label' ), array( 'Value' ) ) ) ), | ||
| ) | ||
| ); | ||
| update_post_meta( $chart_id, Visualizer_Plugin::CF_CHART_TYPE, 'line' ); | ||
| update_post_meta( $chart_id, Visualizer_Plugin::CF_SETTINGS, $settings ); | ||
| return $chart_id; | ||
| } | ||
|
|
||
| /** | ||
| * A chart whose settings meta is a string must not abort usage collection. | ||
| */ | ||
| public function test_string_settings_meta_does_not_crash_logger() { | ||
| $this->create_chart( 'corrupted string settings' ); | ||
|
|
||
| $usage = apply_filters( 'visualizer_logger_data', array() ); | ||
|
|
||
| $this->assertIsArray( $usage ); | ||
| $this->assertSame( 0, $usage['manual_config'] ); | ||
| } | ||
|
|
||
| /** | ||
| * A chart with no settings meta at all must not abort usage collection. | ||
| */ | ||
| public function test_missing_settings_meta_does_not_crash_logger() { | ||
| $chart_id = $this->create_chart( array() ); | ||
| delete_post_meta( $chart_id, Visualizer_Plugin::CF_SETTINGS ); | ||
|
|
||
| $usage = apply_filters( 'visualizer_logger_data', array() ); | ||
|
|
||
| $this->assertIsArray( $usage ); | ||
| $this->assertSame( 0, $usage['manual_config'] ); | ||
| } | ||
|
|
||
| /** | ||
| * On pro, a permission entry that should be an array but is a string | ||
| * must not abort usage collection with a count() TypeError. | ||
| */ | ||
| public function test_malformed_permissions_meta_does_not_crash_logger() { | ||
| // The stub stays defined for the rest of the PHPUnit process. That only | ||
| // affects code gating on class_exists( 'Visualizer_Pro' ) — the legacy | ||
| // license fallback in proFeaturesEnabled() — which no test exercises. | ||
| if ( ! class_exists( 'Visualizer_Pro' ) ) { | ||
| eval( 'class Visualizer_Pro { const CF_PERMISSIONS = "visualizer-permissions"; }' ); | ||
| } | ||
|
|
||
| $chart_id = $this->create_chart( array() ); | ||
| update_post_meta( | ||
| $chart_id, | ||
| Visualizer_Pro::CF_PERMISSIONS, | ||
| array( 'permissions' => array( 'edit-specific' => 'administrator' ) ) | ||
| ); | ||
|
|
||
| add_filter( 'visualizer_is_pro', '__return_true' ); | ||
| $usage = apply_filters( 'visualizer_logger_data', array() ); | ||
| remove_filter( 'visualizer_is_pro', '__return_true' ); | ||
|
|
||
| $this->assertIsArray( $usage ); | ||
| $this->assertSame( 0, $usage['permissions'] ); | ||
| } | ||
|
|
||
| /** | ||
| * Valid array settings still count manual configurations. | ||
| */ | ||
| public function test_manual_config_still_counted_for_array_settings() { | ||
| $this->create_chart( array( 'manual' => '{"colors": ["#000"]}' ) ); | ||
| $this->create_chart( 'corrupted string settings' ); | ||
|
|
||
| $usage = apply_filters( 'visualizer_logger_data', array() ); | ||
|
|
||
| $this->assertSame( 1, $usage['manual_config'] ); | ||
| $this->assertSame( 2, $usage['types']['line'] ); | ||
| } | ||
| } |
Oops, something went wrong.
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.
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.
Confirmed and fixed in a4de841 — the scenario reproduces exactly as described (TypeError at the count() call). Guarded the per-key value with is_array() and added a regression test that forces the pro branch via the visualizer_is_pro filter with a stubbed Visualizer_Pro class.