Skip to content
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

sync: Resolve Phan issues related to Modules::get_module() #37274

Merged
merged 2 commits into from
May 8, 2024
Merged
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
Next Next commit
sync: Resolve Phan issues related to Modules::get_module()
The `Modules::get_module()` method returns a particular, generally known
subclass of `Automattic\Jetpack\Sync\Modules\Module` based on its
parameter. There's no way for Phan to know which subclass is returned
outside of either doing `instanceof` checks everywhere or `@phan-var`.
The latter is less intrusive to the existing code flow, so this does
that.

Also, let's take the opportunity to change the odd
`str_contains( get_class( $module ), 'Full_Sync_Immediately' )` checks
to a more standard `instanceof`.
anomiex committed May 7, 2024
commit 99823098f975a83e879d30690809fd6e88f6898d
16 changes: 6 additions & 10 deletions projects/packages/sync/.phan/baseline.php
Original file line number Diff line number Diff line change
@@ -16,7 +16,6 @@
// PhanUndeclaredProperty : 20+ occurrences
// PhanParamSignatureMismatch : 15+ occurrences
// PhanTypeMismatchArgumentProbablyReal : 15+ occurrences
// PhanUndeclaredMethod : 10+ occurrences
// PhanPluginSimplifyExpressionBool : 9 occurrences
// PhanPossiblyUndeclaredVariable : 8 occurrences
// PhanPluginDuplicateSwitchCaseLooseEquality : 6 occurrences
@@ -47,30 +46,28 @@
// PhanTypeMismatchPropertyProbablyReal : 1 occurrence
// PhanUndeclaredClassMethod : 1 occurrence
// PhanUndeclaredFunction : 1 occurrence
// PhanUndeclaredMethodInCallable : 1 occurrence

// Currently, file_suppressions and directory_suppressions are the only supported suppressions
'file_suppressions' => [
'src/class-actions.php' => ['PhanPluginSimplifyExpressionBool', 'PhanRedundantCondition', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentInternal', 'PhanTypeMismatchArgumentProbablyReal', 'PhanUndeclaredMethod'],
'src/class-actions.php' => ['PhanPluginSimplifyExpressionBool', 'PhanRedundantCondition', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentInternal', 'PhanTypeMismatchArgumentProbablyReal'],
'src/class-data-settings.php' => ['PhanPluginDuplicateConditionalNullCoalescing'],
'src/class-dedicated-sender.php' => ['PhanTypeInvalidLeftOperandOfNumericOp'],
'src/class-functions.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanRedundantCondition', 'PhanTypeMismatchReturnProbablyReal', 'PhanTypePossiblyInvalidDimOffset', 'PhanUndeclaredClassMethod'],
'src/class-listener.php' => ['PhanNonClassMethodCall', 'PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeArraySuspicious', 'PhanTypeExpectedObjectPropAccess', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentProbablyReal', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal'],
'src/class-lock.php' => ['PhanTypeMismatchReturn'],
'src/class-modules.php' => ['PhanUndeclaredMethodInCallable'],
'src/class-queue.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentNullableInternal'],
'src/class-replicastore.php' => ['PhanAccessMethodInternal', 'PhanParamSignatureMismatch', 'PhanPluginDuplicateSwitchCaseLooseEquality', 'PhanTypeArraySuspiciousNullable', 'PhanTypeMismatchArgument', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnNullable', 'PhanTypeSuspiciousStringExpression'],
'src/class-rest-endpoints.php' => ['PhanParamTooMany', 'PhanParamTooManyCallable', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentNullable', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal', 'PhanTypePossiblyInvalidDimOffset', 'PhanUndeclaredMethod'],
'src/class-rest-endpoints.php' => ['PhanParamTooMany', 'PhanParamTooManyCallable', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentNullable', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal', 'PhanTypePossiblyInvalidDimOffset'],
'src/class-rest-sender.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeMismatchArgumentProbablyReal'],
'src/class-sender.php' => ['PhanNonClassMethodCall', 'PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentProbablyReal', 'PhanTypeMismatchProperty', 'PhanTypeMismatchReturnProbablyReal', 'PhanUndeclaredMethod'],
'src/class-sender.php' => ['PhanNonClassMethodCall', 'PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentProbablyReal', 'PhanTypeMismatchProperty', 'PhanTypeMismatchReturnProbablyReal'],
'src/class-server.php' => ['PhanTypeMismatchDeclaredParam', 'PhanTypeMismatchReturnProbablyReal'],
'src/class-settings.php' => ['PhanNonClassMethodCall', 'PhanPossiblyUndeclaredVariable', 'PhanTypeMismatchArgumentProbablyReal'],
'src/class-utils.php' => ['PhanTypeExpectedObjectPropAccess'],
'src/modules/class-callables.php' => ['PhanParamSignatureMismatch', 'PhanParamTooMany', 'PhanTypeArraySuspicious', 'PhanTypeMismatchArgument', 'PhanTypeMismatchReturnProbablyReal'],
'src/modules/class-comments.php' => ['PhanParamSignatureMismatch', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal', 'PhanUndeclaredMethod'],
'src/modules/class-comments.php' => ['PhanParamSignatureMismatch', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal'],
'src/modules/class-constants.php' => ['PhanParamSignatureMismatch', 'PhanTypeMismatchReturnProbablyReal', 'PhanUndeclaredProperty'],
'src/modules/class-full-sync-immediately.php' => ['PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeMismatchReturn', 'PhanUndeclaredMethod'],
'src/modules/class-full-sync.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeComparisonFromArray', 'PhanUndeclaredMethod'],
'src/modules/class-full-sync-immediately.php' => ['PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeMismatchReturn'],
'src/modules/class-full-sync.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanPluginSimplifyExpressionBool', 'PhanPossiblyUndeclaredVariable', 'PhanTypeComparisonFromArray'],
'src/modules/class-import.php' => ['PhanTypeMismatchArgumentInternal'],
'src/modules/class-meta.php' => ['PhanParamSignatureMismatch', 'PhanTypeArraySuspicious', 'PhanTypeMismatchArgumentProbablyReal'],
'src/modules/class-module.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchReturnProbablyReal'],
@@ -85,7 +82,6 @@
'src/modules/class-users.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeMismatchArgumentInternal', 'PhanTypeMismatchDefault', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal'],
'src/modules/class-woocommerce-hpos-orders.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeMismatchReturn', 'PhanTypeMismatchReturnProbablyReal'],
'src/modules/class-woocommerce.php' => ['PhanTypeMismatchReturnProbablyReal'],
'src/replicastore/class-table-checksum-usermeta.php' => ['PhanUndeclaredMethod'],
'src/replicastore/class-table-checksum.php' => ['PhanPluginDuplicateConditionalNullCoalescing', 'PhanTypeMismatchPropertyDefault', 'PhanTypeMismatchPropertyProbablyReal'],
'tests/php/test-actions.php' => ['PhanTypeMismatchArgument', 'PhanTypeMismatchArgumentProbablyReal'],
'tests/php/test-dedicated-sender.php' => ['PhanDeprecatedFunction', 'PhanUndeclaredProperty'],
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: fixed
Comment: Add `@phan-var` for many calls to `Sync\Modules::get_module()` to specify which module subclass it returned. Should be no change to functionality.


8 changes: 6 additions & 2 deletions projects/packages/sync/src/class-actions.php
Original file line number Diff line number Diff line change
@@ -589,6 +589,7 @@ public static function do_initial_sync() {

// Don't start new sync if a full sync is in process.
$full_sync_module = Modules::get_module( 'full-sync' );
'@phan-var Modules\Full_Sync_Immediately|Modules\Full_Sync $full_sync_module';
if ( $full_sync_module && $full_sync_module->is_started() && ! $full_sync_module->is_finished() ) {
return false;
}
@@ -611,6 +612,7 @@ public static function do_initial_sync() {
*/
public static function do_only_first_initial_sync() {
$full_sync_module = Modules::get_module( 'full-sync' );
'@phan-var Modules\Full_Sync_Immediately|Modules\Full_Sync $full_sync_module';
if ( $full_sync_module && $full_sync_module->is_started() ) {
return false;
}
@@ -633,6 +635,7 @@ public static function do_full_sync( $modules = null ) {
}

$full_sync_module = Modules::get_module( 'full-sync' );
'@phan-var Modules\Full_Sync_Immediately|Modules\Full_Sync $full_sync_module';

if ( ! $full_sync_module ) {
return false;
@@ -1041,7 +1044,8 @@ public static function get_sync_status( $fields = null ) {
self::initialize_sender();

$sync_module = Modules::get_module( 'full-sync' );
$queue = self::$sender->get_sync_queue();
'@phan-var Modules\Full_Sync_Immediately|Modules\Full_Sync $sync_module';
$queue = self::$sender->get_sync_queue();

// _get_cron_array can be false
$cron_timestamps = ( _get_cron_array() ) ? array_keys( _get_cron_array() ) : array();
@@ -1091,7 +1095,7 @@ public static function get_sync_status( $fields = null ) {
);

// Verify $sync_module is not false.
if ( ( $sync_module ) && ! str_contains( get_class( $sync_module ), 'Full_Sync_Immediately' ) ) {
if ( $sync_module && ! $sync_module instanceof Modules\Full_Sync_Immediately ) {
$result['full_queue_size'] = $full_queue->size();
$result['full_queue_lag'] = $full_queue->lag();
}
3 changes: 3 additions & 0 deletions projects/packages/sync/src/class-modules.php
Original file line number Diff line number Diff line change
@@ -93,6 +93,8 @@ public static function set_defaults() {
* @return bool|\Automattic\Jetpack\Sync\Modules\Module
*/
public static function get_module( $module_name ) {
// @todo Better type hinting for Phan if https://github.com/phan/phan/issues/3842 gets fixed. Then clean up the `@phan-var` on all the callers.

foreach ( self::get_modules() as $module ) {
if ( $module->name() === $module_name ) {
return $module;
@@ -153,6 +155,7 @@ public static function load_module( $module_class ) {
public static function set_module_defaults( $module ) {
$module->set_defaults();
if ( method_exists( $module, 'set_late_default' ) ) {
// @phan-suppress-next-line PhanUndeclaredMethodInCallable -- https://github.com/phan/phan/issues/1204
add_action( 'init', array( $module, 'set_late_default' ), 90 );
}
return $module;
1 change: 1 addition & 0 deletions projects/packages/sync/src/class-rest-endpoints.php
Original file line number Diff line number Diff line change
@@ -698,6 +698,7 @@ public static function close( $request ) {
// Update Full Sync Status if queue is "full_sync".
if ( 'full_sync' === $queue_name ) {
$full_sync_module = Modules::get_module( 'full-sync' );
'@phan-var Modules\Full_Sync_Immediately|Modules\Full_Sync $full_sync_module';
$full_sync_module->update_sent_progress_action( $items );
}

7 changes: 5 additions & 2 deletions projects/packages/sync/src/class-sender.php
Original file line number Diff line number Diff line change
@@ -292,6 +292,7 @@ public function set_next_sync_time( $time, $queue_name ) {
*/
public function do_full_sync() {
$sync_module = Modules::get_module( 'full-sync' );
'@phan-var Modules\Full_Sync_Immediately|Modules\Full_Sync $sync_module';
if ( ! $sync_module ) {
return;
}
@@ -313,7 +314,7 @@ public function do_full_sync() {

$this->continue_full_sync_enqueue();
// immediate full sync sends data in continue_full_sync_enqueue.
if ( ! str_contains( get_class( $sync_module ), 'Full_Sync_Immediately' ) ) {
if ( ! $sync_module instanceof Modules\Full_Sync_Immediately ) {
return $this->do_sync_and_set_delays( $this->full_sync_queue );
} else {
$status = $sync_module->get_status();
@@ -342,7 +343,9 @@ private function continue_full_sync_enqueue() {
return false;
}

Modules::get_module( 'full-sync' )->continue_enqueuing();
$full_sync_module = Modules::get_module( 'full-sync' );
'@phan-var Modules\Full_Sync_Immediately|Modules\Full_Sync $full_sync_module';
$full_sync_module->continue_enqueuing();

$this->set_next_sync_time( time() + $this->get_enqueue_wait_time(), 'full-sync-enqueue' );
}
1 change: 1 addition & 0 deletions projects/packages/sync/src/modules/class-comments.php
Original file line number Diff line number Diff line change
@@ -240,6 +240,7 @@ public function only_allow_white_listed_comment_types( $args ) {
public function filter_blacklisted_post_types( $args ) {
$post_id = $args[0];
$posts_module = Modules::get_module( 'posts' );
'@phan-var Posts $posts_module';

if ( false !== $posts_module && ! $posts_module->is_post_type_allowed( $post_id ) ) {
return false;
Original file line number Diff line number Diff line change
@@ -87,7 +87,9 @@ public function start( $full_sync_config = null ) {
}

if ( isset( $full_sync_config['users'] ) && 'initial' === $full_sync_config['users'] ) {
$full_sync_config['users'] = Modules::get_module( 'users' )->get_initial_sync_user_config();
$users_module = Modules::get_module( 'users' );
'@phan-var Users $users_module';
$full_sync_config['users'] = $users_module->get_initial_sync_user_config();
}

$this->update_status(
1 change: 1 addition & 0 deletions projects/packages/sync/src/modules/class-full-sync.php
Original file line number Diff line number Diff line change
@@ -125,6 +125,7 @@ public function start( $module_configs = null ) {
}

if ( 'users' === $module_name && 'initial' === $module_config ) {
'@phan-var Users $module';
$module_config = $module->get_initial_sync_user_config();
}

2 changes: 1 addition & 1 deletion projects/packages/sync/src/modules/class-posts.php
Original file line number Diff line number Diff line change
@@ -233,7 +233,7 @@ public function init_before_send() {
add_filter( 'jetpack_sync_before_send_deleted_post_meta', array( $this, 'trim_post_meta' ) );
// Full sync.
$sync_module = Modules::get_module( 'full-sync' );
if ( $sync_module && str_contains( get_class( $sync_module ), 'Full_Sync_Immediately' ) ) {
if ( $sync_module instanceof Full_Sync_Immediately ) {
add_filter( 'jetpack_sync_before_send_jetpack_full_sync_posts', array( $this, 'add_term_relationships' ) );
} else {
add_filter( 'jetpack_sync_before_send_jetpack_full_sync_posts', array( $this, 'expand_posts_with_metadata_and_terms' ) );
Original file line number Diff line number Diff line change
@@ -159,6 +159,7 @@ public function calculate_checksum( $range_from = null, $range_to = null, $filte
*/
protected function expand_and_sanitize_user_meta( $user_object ) {
$user_module = Modules::get_module( 'users' );
'@phan-var \Automattic\Jetpack\Sync\Modules\Users $user_module';
// Expand User Objects based on Sync logic.
$user_object = $user_module->expand_user( $user_object );

Loading

Unchanged files with check annotations Beta

timeout,
}
)
.toBeTruthy();

Check failure on line 155 in projects/plugins/jetpack/tests/e2e/specs/sync/sync.test.js

GitHub Actions / Jetpack sync e2e tests

specs/sync/sync.test.js:49:2 › Sync › Normal Sync flow

1) specs/sync/sync.test.js:49:2 › Sync › Normal Sync flow › Assert post is synced ──────────────── Error: Sync queue should be empty [after post publish] expect(received).toBeTruthy() Received: false Call Log: - Timeout 30000ms exceeded while waiting on the predicate 153 | } 154 | ) > 155 | .toBeTruthy(); | ^ 156 | } 157 | } ); 158 | at assertSyncQueueIsEmpty (/home/runner/work/jetpack/jetpack/projects/plugins/jetpack/tests/e2e/specs/sync/sync.test.js:155:5) at /home/runner/work/jetpack/jetpack/projects/plugins/jetpack/tests/e2e/specs/sync/sync.test.js:57:10 at /home/runner/work/jetpack/jetpack/projects/plugins/jetpack/tests/e2e/specs/sync/sync.test.js:56:14
}
} );
*/
async click( selector, options = {} ) {
logger.action( `Clicking element '${ selector }'` );
await this.page.click( selector, options );

Check failure on line 183 in tools/e2e-commons/pages/page-actions.js

GitHub Actions / Social e2e tests

specs/connection.test.js:15:1 › Jetpack Social connection

1) specs/connection.test.js:15:1 › Jetpack Social connection › Can connect wordpress.com account to Jetpack Social Error: page.click: Target page, context or browser has been closed at ../../../../../tools/e2e-commons/pages/page-actions.js:183 181 | async click( selector, options = {} ) { 182 | logger.action( `Clicking element '${ selector }'` ); > 183 | await this.page.click( selector, options ); | ^ 184 | } 185 | 186 | /** at AuthorizePage.click (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/page-actions.js:183:19) at AuthorizePage.approve (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/wpcom/authorize.js:13:15) at AuthorizePage.approve (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/wpcom/authorize.js:20:23) at connect (/home/runner/work/jetpack/jetpack/projects/plugins/social/tests/e2e/flows/connection.js:10:2) at /home/runner/work/jetpack/jetpack/projects/plugins/social/tests/e2e/specs/connection.test.js:17:3 at /home/runner/work/jetpack/jetpack/projects/plugins/social/tests/e2e/specs/connection.test.js:16:2
}
/**
logger.action(
`Waiting for element '${ selector }' to be ${ state } [timeout: ${ timeout } ms]`
);
return await this.page.waitForSelector( selector, { state, timeout } );

Check failure on line 314 in tools/e2e-commons/pages/page-actions.js

GitHub Actions / Jetpack Boost - Page Cache e2e tests

specs/page-cache/page-cache.test.js:88:2 › Cache module › Page Cache meta information should show on the admin when the module is active

1) specs/page-cache/page-cache.test.js:88:2 › Cache module › Page Cache meta information should show on the admin when the module is active TimeoutError: page.waitForSelector: Timeout 180000ms exceeded. Call log: - waiting for locator('[data-testid="page-cache-meta"]') to be visible at ../../../../../tools/e2e-commons/pages/page-actions.js:314 312 | `Waiting for element '${ selector }' to be ${ state } [timeout: ${ timeout } ms]` 313 | ); > 314 | return await this.page.waitForSelector( selector, { state, timeout } ); | ^ 315 | } 316 | 317 | /** at JetpackBoostPage.waitForElementState (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/page-actions.js:314:26) at JetpackBoostPage.waitForElementToBeVisible (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/page-actions.js:264:21) at JetpackBoostPage.waitForPageCacheMetaInfoVisibility (/home/runner/work/jetpack/jetpack/projects/plugins/boost/tests/e2e/lib/pages/wp-admin/JetpackBoostPage.js:136:15) at /home/runner/work/jetpack/jetpack/projects/plugins/boost/tests/e2e/specs/page-cache/page-cache.test.js:97:27

Check failure on line 314 in tools/e2e-commons/pages/page-actions.js

GitHub Actions / Social e2e tests

specs/social-sidebar.test.js:17:1 › Jetpack Social sidebar

2) specs/social-sidebar.test.js:17:1 › Jetpack Social sidebar ──────────────────────────────────── TimeoutError: page.waitForSelector: Timeout 20000ms exceeded. Call log: - waiting for locator('#loginform') to be visible at ../../../../../tools/e2e-commons/pages/page-actions.js:314 312 | `Waiting for element '${ selector }' to be ${ state } [timeout: ${ timeout } ms]` 313 | ); > 314 | return await this.page.waitForSelector( selector, { state, timeout } ); | ^ 315 | } 316 | 317 | /** at WPLoginPage.waitForElementState (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/page-actions.js:314:26) at WPLoginPage.waitForElementToBeVisible (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/page-actions.js:264:21) at WPLoginPage.waitForPage (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/page-actions.js:63:16) at Function.init (/home/runner/work/jetpack/jetpack/tools/e2e-commons/pages/wp-page.js:18:3) at loginToWpSite (/home/runner/work/jetpack/jetpack/tools/e2e-commons/flows/log-in.js:25:10) at ensureUserIsLoggedIn (/home/runner/work/jetpack/jetpack/tools/e2e-commons/env/prerequisites.js:173:2) at buildPrerequisites (/home/runner/work/jetpack/jetpack/tools/e2e-commons/env/prerequisites.js:85:5) at Object.build (/home/runner/work/jetpack/jetpack/tools/e2e-commons/env/prerequisites.js:63:4) at /home/runner/work/jetpack/jetpack/projects/plugins/social/tests/e2e/specs/social-sidebar.test.js:8:2
}
/**
import { test, expect } from 'jetpack-e2e-commons/fixtures/base-test.js';

Check failure on line 1 in projects/plugins/social/tests/e2e/specs/connection.test.js

GitHub Actions / Social e2e tests

specs/connection.test.js:15:1 › Jetpack Social connection

1) specs/connection.test.js:15:1 › Jetpack Social connection › Can connect wordpress.com account to Jetpack Social Test timeout of 300000ms exceeded.
import { JetpackSocialPage } from '../pages/index.js';
import { connect } from '../flows/index.js';
import { prerequisitesBuilder } from 'jetpack-e2e-commons/env/prerequisites.js';