Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion classes/Visualizer/Module/Admin.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,6 @@ public function __construct( Visualizer_Plugin $plugin ) {
$this->_addFilter( 'media_view_strings', 'setupMediaViewStrings' );
$this->_addFilter( 'plugin_action_links', 'getPluginActionLinks', 10, 2 );
$this->_addFilter( 'plugin_row_meta', 'getPluginMetaLinks', 10, 2 );
$this->_addFilter( 'visualizer_logger_data', 'getLoggerData' );
$this->_addFilter( 'visualizer_feedback_review_trigger', 'feedbackReviewTrigger' );
$this->_addFilter( 'themeisle_sdk_blackfriday_data', 'add_black_friday_data' );

Expand Down
4 changes: 2 additions & 2 deletions classes/Visualizer/Module/Setup.php
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ public function getUsage( $data, $meta_keys = array() ) {
$lib = get_post_meta( $chart_id, Visualizer_Plugin::CF_CHART_LIBRARY, true );
$charts['library'][ $lib ] = isset( $charts['library'][ $lib ] ) ? $charts['library'][ $lib ] + 1 : 1;
$settings = get_post_meta( $chart_id, Visualizer_Plugin::CF_SETTINGS, true );
if ( array_key_exists( 'manual', $settings ) && ! empty( $settings['manual'] ) ) {
if ( is_array( $settings ) && ! empty( $settings['manual'] ) ) {
$charts['manual_config'] = $charts['manual_config'] + 1;
}

Expand All @@ -124,7 +124,7 @@ public function getUsage( $data, $meta_keys = array() ) {

if ( Visualizer_Module::is_pro() ) {
$permissions = get_post_meta( $chart_id, Visualizer_Pro::CF_PERMISSIONS, true );
if ( empty( $permissions ) ) {
if ( ! is_array( $permissions ) || empty( $permissions['permissions'] ) ) {

Copy link
Copy Markdown
Contributor Author

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.

Comment thread
Copilot marked this conversation as resolved.
Outdated
continue;
}
$permissions = $permissions['permissions'];
Expand Down
16 changes: 16 additions & 0 deletions tests/e2e/config/mu-plugins/plant-chart-settings.php
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,21 @@ function () {
},
)
);

// Runs the SDK usage logger on demand, so specs can verify it
// tolerates whatever chart meta they planted (issue #1359).
register_rest_route(
'visualizer-e2e/v1',
'/usage',
array(
'methods' => 'GET',
'permission_callback' => function () {
return current_user_can( 'manage_options' );
},
'callback' => function () {
return apply_filters( 'visualizer_logger_data', array() );
},
)
);
}
);
61 changes: 61 additions & 0 deletions tests/e2e/specs/usage-logger.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

/**
* 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 } ) => {
Comment on lines +1 to +23

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a4de841 — the spec now calls deleteAllCharts() in beforeAll like the other chart-counting specs.

// 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 );
} );
} );
76 changes: 76 additions & 0 deletions tests/test-usage-logger.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?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'] );
}

/**
* 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'] );
}
}
Loading