Fix/security fixures #568
Fix/security fixures #568
Conversation
Introduces whitelists of allowed columns for SQL ORDER BY clauses in helper classes to prevent invalid or unsafe column names. Also ensures order directions are validated and all parameters are sanitized before building SQL queries, improving security and robustness across Activity, Comment, Discussion_Board, File, Milestone, Project, Task, Task_List, and User helpers.
Introduces separate XML rulesets for accessibility, general, performance, plugin repository, and security checks under phpcs-xml/. Updates phpcs.xml.dist to reference the new modular rulesets and improves organization and documentation of coding standards for WordPress plugins.
Replaced direct filesystem and random functions with WP_Filesystem and wp_rand for better security and compatibility. Updated all date() usages to gmdate() to avoid timezone issues. Added phpcs:ignore comments for intentional global overrides in admin menu code. Improved output escaping and fixed minor issues flagged by WordPress coding standards. Enabled additional PHPCS rules for performance and general checks.
Introduces comprehensive WP-CLI commands for seeding and cleaning Project Manager data, including users, projects, task lists, tasks, and milestones. Improves robustness in Task and User helpers by handling empty data and preventing SQL errors. Updates Pusher action to correctly detect admin status. Adds '/phpcs-xml' to .distignore.
WalkthroughMigrates filesystem I/O to WP_Filesystem, converts local date() calls to gmdate() across modules, hardens SQL (prepared statements and ORDER BY whitelists with esc_sql), adds input sanitization, replaces cURL with WP HTTP API, adds CLI seeding/cleanup commands, introduces PHPCS rulesets and docs, and small UI/error-handling tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
core/Promotions/Offers.php (1)
174-191: Tighten initial POST validation indismiss_offer()The guard:
if ( empty( $_POST['nonce'] ) && ! isset( $_POST['pm_offer_key'] ) ) { return; }only returns when both the nonce is empty and the key is missing. If one is present and the other is not, you proceed and later read
$_POST['pm_offer_key'], which can trigger an undefined index notice and lead to updating the option with an empty value.Recommend short‑circuiting when either parameter is missing:
- if ( empty( $_POST['nonce'] ) && ! isset( $_POST['pm_offer_key'] ) ) { + if ( empty( $_POST['nonce'] ) || ! isset( $_POST['pm_offer_key'] ) ) { return; }src/Project/Controllers/Project_Controller.php (1)
394-424: Sanitizing$favouritechanged behavior for boolean inputsAfter
sanitize_text_field( $request->get_param( 'favourite' ) ),$favouriteis always a string. That makes the$favourite === truecheck unreachable and breaks clients that previously sent a JSON booleantrueinstead of the string'true'(they will now always hit the “remove from favorite” path).To support both string and boolean inputs while still sanitizing, consider using your existing
pm_is_true()helper and treating$favouriteas a boolean:- $project_id = intval( $request->get_param( 'id' ) ); - $favourite = sanitize_text_field( $request->get_param( 'favourite' ) ) ; + $project_id = intval( $request->get_param( 'id' ) ); + $favourite_in = $request->get_param( 'favourite' ); + $favourite = pm_is_true( $favourite_in ); @@ - if ( $favourite == 'true' || $favourite === true ) { + if ( $favourite ) { @@ - if ( $favourite == 'true' ) { + if ( $favourite ) {This preserves backward compatibility for existing boolean callers while keeping the control flow simple.
src/Task/Controllers/Task_Controller.php (2)
769-780: Inconsistent date function usage within the same filter logic.The
overduecase now usesgmdate()(line 769), buttodayandweekcases (lines 772-776) still usedate(). This creates inconsistency within the same filter logic.Additionally, mixing
gmdate()withcurrent_time('mysql')(which returns local WordPress time) may lead to incorrect comparisons. Consider either:
- Using
gmdate()consistently with a UTC time source, or- Using
date()consistently with local timeif( $due_date == 'overdue' ) { $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); $q->where( 'due_date', '<', $today ); } else if ( $due_date == 'today' ) { - $today = date('Y-m-d', strtotime( current_time('mysql') ) ); + $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); $q->where( 'due_date', $today ); } else if ( $due_date == 'week' ) { - $today = date('Y-m-d', strtotime( current_time('mysql') ) ); - $last = date('Y-m-d', strtotime( current_time('mysql') . '-1 week' ) ); + $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); + $last = gmdate( 'Y-m-d', strtotime( current_time('mysql') . '-1 week' ) ); $q->where( 'due_date', '>=', $last ); $q->where( 'due_date', '<=', $today ); }
809-821: Same inconsistency exists in the whereHas callback.Apply the same fix here as noted above for consistency.
if( $due_date == 'overdue' ) { $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); $q->where( 'due_date', '<', $today ); } else if ( $due_date == 'today' ) { - $today = date('Y-m-d', strtotime( current_time('mysql') ) ); + $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); $q->where( 'due_date', $today ); } else if ( $due_date == 'week' ) { - $today = date('Y-m-d', strtotime( current_time('mysql') ) ); - $last = date('Y-m-d', strtotime( current_time('mysql') . '-1 week' ) ); + $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); + $last = gmdate( 'Y-m-d', strtotime( current_time('mysql') . '-1 week' ) ); $q->where( 'due_date', '>=', $last ); $q->where( 'due_date', '<=', $today ); }src/Comment/Helper/Comment.php (1)
345-346: Incorrect table reference in ORDER BY clause.The
$tb_pjvariable referencespm_boards, but this is the Comment helper. The ORDER BY clause will reference columns from the wrong table. This should use$this->tb_commentorpm_commentsinstead.private function orderby() { global $wpdb; - $tb_pj = $wpdb->prefix . 'pm_boards'; + $tb_comment = $this->tb_comment; $odr_prms = isset( $this->query_params['orderby'] ) ? $this->query_params['orderby'] : false;And update line 392:
foreach ( $orders as $key => $value ) { - $order[] = $tb_pj . '.' . esc_sql($key) . ' ' . esc_sql($value); + $order[] = $tb_comment . '.' . esc_sql($key) . ' ' . esc_sql($value); }src/File/Helper/File.php (1)
280-281: Incorrect table reference in ORDER BY clause.The
$tb_pjvariable referencespm_boards, but this is the File helper. The ORDER BY clause will reference columns from the wrong table. This should use$this->tb_fileorpm_filesinstead.private function orderby() { global $wpdb; - $tb_pj = $wpdb->prefix . 'pm_boards'; + $tb_file = $this->tb_file; $odr_prms = isset( $this->query_params['orderby'] ) ? $this->query_params['orderby'] : false;And update line 326:
foreach ( $orders as $key => $value ) { - $order[] = $tb_pj . '.' . esc_sql($key) . ' ' . esc_sql($value); + $order[] = $tb_file . '.' . esc_sql($key) . ' ' . esc_sql($value); }core/Cli/Commands.php (1)
771-774: Incorrect use of$wpdb->prepare()for table names.The
TRUNCATE TABLE %spattern is fundamentally broken -$wpdb->prepare()wraps%sin quotes, producing invalid SQL likeTRUNCATE TABLE 'wp_pm_activities'. Table identifiers cannot be parameterized this way.Apply this diff to fix the issue:
foreach ($tables as $table) { - /* translators: %s: Table Name. */ - $wpdb->query( $wpdb->prepare( "TRUNCATE TABLE %s", $wpdb->prefix . $table ) ); + $table_name = $wpdb->prefix . $table; + // phpcs:ignore WordPress.DB.PreparedSQL.InterpolatedNotPrepared -- Table name is from hardcoded whitelist + $wpdb->query( "TRUNCATE TABLE `" . esc_sql( $table_name ) . "`" ); }src/Tools/Library/PM_Asana.php (1)
541-560: File upload implementation incompatible with WordPress HTTP API.The
addAttachmentToTaskmethod still constructsCURLFileobjects and passes them toaskAsana, butaskAsananow useswp_remote_requestwhich does not supportCURLFile. This will cause file uploads to fail.The WordPress HTTP API requires a different approach for file uploads. Consider using
WP_Http::request()with a proper multipart body or document that file uploads require additional handling.public function addAttachmentToTask($taskId, array $data = array()) { - $mimeType = array_key_exists('mimeType', $data) ? $data['mimeType'] : null; - $finalFilename = array_key_exists('finalFilename', $data) ? $data["finalFilename"] : null; - - if (class_exists('CURLFile', false)) { - $data['file'] = new CURLFile($data['file'], $data['mimeType'], $data['finalFilename']); - } else { - $data['file'] = "@{$data['file']}"; - - if (!is_null($finalFilename)) { - $data['file'] .= ';filename=' . $finalFilename; - } - if (!is_null($mimeType)) { - $data['file'] .= ';type=' . $mimeType; - } - } - - return $this->askAsana($this->tasksUrl . '/' . $taskId . '/attachments', $data, ASANA_METHOD_POST); + // File uploads are not currently supported with WordPress HTTP API + // This requires custom multipart handling + throw new \Exception('File attachments are not currently supported'); }
🧹 Nitpick comments (21)
src/Pusher/Src/Controllers/Pusher_Controller.php (1)
36-48: JSON handling for Pusher auth looks correct; consider consistent error response (optional).Using
$auth_response = $pusher->socket_auth( $channel_name, $socket_id );and then sending it with an explicitContent-Type: application/jsonheader is aligned withAuth::socket_auth()returning a JSON-encoded string, and the PHPCS ignore is justified in this context. This is a clear improvement over the previous direct echo without a content type.If you want to tighten things further (not required for this PR), you could:
- Return a JSON error body and
Content-Type: application/jsonin the 403 branch as well, for consistent response semantics from this endpoint.- Longer term, consider using
wp_send_json()/WP_REST_Responseinstead ofecho/exitto better align with REST patterns.core/Promotions/Offers.php (1)
48-51: Make fallback button label translatableThe fallback
'Get Now'string is user‑visible but not currently translatable.You could keep remote
action_titleas‑is while making the local fallback i18n‑ready:- $offer->btn_txt = ! empty( $promo_notice['action_title'] ) ? $promo_notice['action_title'] : 'Get Now'; + $offer->btn_txt = ! empty( $promo_notice['action_title'] ) + ? $promo_notice['action_title'] + : __( 'Get Now', 'wedevs-project-manager' );src/Tools/Helpers/ImportAsana.php (2)
478-478: Add spacing around function arguments for readability.Line 478 is missing a space after the comma in the string replacement call.
- $hostname = str_replace('http', '',get_site_url()); + $hostname = str_replace('http', '', get_site_url());
319-325: Prefer explicit isset() checks or null-safe operators for object properties.Directly accessing object properties like
$credentials->emailwithout validation can raise notices if properties don't exist. Consider using isset or the null-safe operator (?.) for clarity.- if($credentials->email){ + if(isset($credentials->email) && $credentials->email){ $email = sanitize_email( $credentials->email ); error_log($email); $user_id = $this->getOrCreateUserId($credentials->name, $email); } else { - $user_id = $this->getOrCreateUserId($credentials->name,$this->makeFakeEmail($credentials->name)); + $user_id = $this->getOrCreateUserId($credentials->name, $this->makeFakeEmail($credentials->name)); }phpcs-xml/plugin-check-performance.xml (1)
20-32: Misleading comments for rule purposes.The inline comments don't accurately describe the sniffs:
- Line 20: "Proper transient usage" but
WordPress.WP.CronIntervalchecks cron scheduling intervals, not transients.- Line 29: "Discourage slow string concatenation in loops" but
Squiz.PHP.DisallowMultipleAssignmentschecks for multiple variable assignments in a single statement (e.g.,$a = $b = 1), not string concatenation.- <!-- Proper transient usage --> + <!-- Check for excessively short cron intervals --> <rule ref="WordPress.WP.CronInterval"> <type>warning</type> </rule> <!-- =================================== --> <!-- PHP PERFORMANCE --> <!-- =================================== --> - <!-- Discourage slow string concatenation in loops --> + <!-- Discourage multiple variable assignments in a single statement --> <rule ref="Squiz.PHP.DisallowMultipleAssignments"> <type>warning</type> </rule>phpcs-xml/README.md (2)
7-14: Consider specifying a language for the code block.The directory structure code block could use a language identifier like
textorplaintextfor consistency with markdown linting standards.-``` +```text phpcs-xml/ ├── plugin-check-security.xml # Security-focused rules ...
116-119: Convert bare URL to markdown link for consistency.The Plugin Check plugin reference uses a bare URL, while other references (lines 123-127) use proper markdown link syntax.
These rulesets are based on the official WordPress Plugin Check plugin: -https://wordpress.org/plugins/plugin-check/ +[Plugin Check](https://wordpress.org/plugins/plugin-check/)phpcs.xml.dist (1)
34-41: Potential duplicate rule inclusions.The legacy rules section enables
WordPress.DB,WordPress.Security, andWordPress.CodeAnalysis, which overlap with rules already included viaplugin-check-security.xmlandplugin-check-general.xml. This may result in duplicate warnings for the same issues.Consider either:
- Removing the legacy rules section since the plugin-check rulesets cover these
- Adding exclusions in the plugin-check XML files to avoid overlap
If the plugin-check rulesets fully cover these, you could simplify:
<!-- =================================== --> <!-- LEGACY RULES (Kept for compatibility) --> <!-- =================================== --> <!-- WordPress Core Standards (optional, can be enabled) --> <!-- <rule ref="WordPress-Core"> <exclude name="Generic.WhiteSpace.DisallowSpaceIndent"/> </rule> --> - <!-- WordPress Database Rules --> - <rule ref="WordPress.DB"/> - - <!-- WordPress Security Rules --> - <rule ref="WordPress.Security"/> - - <!-- WordPress Code Analysis Rules --> - <rule ref="WordPress.CodeAnalysis"/> + <!-- Note: WordPress.DB, WordPress.Security, and WordPress.CodeAnalysis + are now included via plugin-check-security.xml and plugin-check-general.xml -->core/Upgrades/Upgrade_2_0.php (1)
1403-1426: Repeated$nowpattern: consider simplifying and/or using full UTC datetimeIn
gantt_upgrate,add_time_tracker, andinsert_client_roleyou now use:$now = gmdate( 'Y-m-d', strtotime( current_time( 'mysql' ) ) );and then persist
$nowintocreated_at/updated_at.
- This yields a UTC date (no time), which matches the pre‑existing
date('Y-m-d', ...)behavior except for the timezone shift.- The expression is a bit convoluted (WP local time string →
strtotime→ UTC date) and the timestamp columns still only get a date component.Not a blocker, but you might consider, in a follow‑up:
- Either storing a full UTC datetime (e.g.
gmdate( 'Y-m-d H:i:s' )orcurrent_time( 'mysql', true )) forcreated_at/updated_at, or- If you truly only care about the calendar date, simplifying to a more direct “today in UTC” expression and using it consistently across all three sites.
If you decide to adjust this, it’d be good to double‑check any code that compares these timestamps or orders by them so behavior stays consistent.
Also applies to: 1588-1613, 1914-1928
src/Tools/Helpers/ImportTrello.php (1)
429-441: Use URL parsing instead of string replaces for hostname
str_replace( 'http', '', get_site_url() )can mangle the host (e.g.,https://example.combecomessexample.com). Since you’re already touching this area, it’s safer to derive the host via URL parsing:- $hostname = str_replace('http', '',get_site_url()); - $hostname = str_replace('://', '', $hostname); + $hostname = parse_url( get_site_url(), PHP_URL_HOST ); + if ( empty( $hostname ) ) { + $hostname = 'localhost'; + }This keeps fake emails well‑formed regardless of scheme.
core/WP/Menu.php (1)
67-101: Remove unused$todayinmy_task_count()
$todayis computed withgmdate()but never used, and PHPMD is flagging it. You can safely drop this line to reduce noise:- $today = gmdate( 'Y-m-d', strtotime( current_time( 'mysql' ) ) );src/Pusher/Libs/action.php (1)
39-52: Safer detection ofis_admininPM_pusher_before_assignees()The new
isset( $_POST['is_admin'] ) && ! empty( intval( pm_clean( $_POST['is_admin'] ) ) )check avoids undefined index notices while still sanitizing and normalizing the flag to a boolean. You might later consider extracting this pattern into a small helper and reusing it where other functions still read$_POST['is_admin']directly.core/File_System/File_System.php (1)
68-73: Redundant require_once statement.The
file.phpis already included at line 5 of this file. This require_once is unnecessary.// Initialize WP_Filesystem global $wp_filesystem; if ( empty( $wp_filesystem ) ) { - require_once ABSPATH . 'wp-admin/includes/file.php'; WP_Filesystem(); }src/My_Task/Controllers/MyTask_Controller.php (1)
165-172: Pre-existing SQL interpolation concern.While not introduced in this PR, the direct interpolation of
$boards_id,$project_ids, and$user_idinto the query string (lines 169, 172, 284-287, 404-407) doesn't use prepared statements. These values come from trusted sources (database queries andget_current_user_id()), but parameterizing them would be more defensive.Consider addressing this in a follow-up to maintain consistency with the prepared statement improvements made elsewhere in this file.
Also applies to: 284-287
core/Cli/Commands.php (3)
603-612: Direct table name interpolation in SQL queries.While the table names are constructed from trusted sources (
$wpdb->prefix+ hardcoded strings), using direct string interpolation in SQL is inconsistent with the security-focused nature of this PR. The$full_table_namevariable is safely constructed, but consider usingesc_sql()for consistency with the PR's security hardening goals.Additionally, line 603 uses a raw query for
COUNT(*)without$wpdb->prepare(), though this is acceptable since no user input is involved.
88-89: Unused parameters flagged by static analysis.The
$argsparameter and$wpdbvariable are unused. While$argsis required by WP-CLI's callback signature, the$wpdbdeclaration on line 89 appears to be a leftover from removed code.- public function seed_all($args, $assoc_args) { - global $wpdb; + public function seed_all($args, $assoc_args) { // phpcs:ignore Generic.CodeAnalysis.UnusedFunctionParameter.FoundBeforeLastUsed
182-209: Potential collision in generated usernames.The
seed_usersmethod generates usernames usingwp_rand(1000, 9999), which provides only 9,000 unique values. When creating many users (e.g., 50+), collisions become likely, silently skipping user creation.Consider using a larger range or a unique suffix:
for ($i = 0; $i < $count; $i++) { - $username = 'testuser_' . wp_rand(1000, 9999); + $username = 'testuser_' . wp_rand(100000, 999999) . '_' . $i; $email = $username . '@testproject.local';src/Task/Helper/Task.php (1)
93-134: WP_Filesystem initialized but not used.The code initializes
WP_Filesystem(lines 94-99) but then uses native PHP stream functions (fopen,fputcsv,stream_get_contents) for the actual CSV output. The filesystem abstraction serves no purpose here.Either remove the unused
WP_Filesysteminitialization or refactor to use$wp_filesystem->put_contents()if the intent is to write to an actual file.public static function get_taskscsv( WP_REST_Request $request ) { - $self = self::getInstance(); $tasks = self::get_results( $request->get_params() ); - // Initialize WP_Filesystem - global $wp_filesystem; - if ( empty( $wp_filesystem ) ) { - require_once ABSPATH . 'wp-admin/includes/file.php'; - WP_Filesystem(); - } - header('Content-Type: text/csv; charset=utf-8'); header('Content-Disposition: attachment; filename=data.csv');src/Task_List/Controllers/Task_List_Controller.php (2)
95-96: Unused variable$is_archive.The
$is_archivevariable is assigned but never used in theindexmethod. Static analysis correctly flagged this.- $is_archive = filter_var($request->get_param('is_archive'), FILTER_VALIDATE_BOOLEAN);If the variable is intended for future use, consider adding a
// phpcs:ignorecomment with explanation.
248-248: Unused variable$with.The
$withvariable on line 248 is sanitized but never used - the raw parameter is passed toget_liston line 253 instead.public function show( WP_REST_Request $request ) { $project_id = intval( $request->get_param( 'project_id' ) ); $task_list_id = intval( $request->get_param( 'task_list_id' ) ); - $with = sanitize_text_field($request->get_param('with')); return $this->get_list( [ 'project_id' => $project_id, 'task_list_id' => $task_list_id, - 'with' => $request->get_param( 'with' ) + 'with' => sanitize_text_field($request->get_param( 'with' )) ] ); }src/Activity/Helper/Activity.php (1)
31-31: Consider removing unused property.The
$per_pageproperty is declared but never assigned or read in the file. If it's not intended for future use, consider removing it to keep the code clean.Apply this diff if the property is not needed:
- private $per_page;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
reports/security copy.csvis excluded by!**/*.csvreports/security.csvis excluded by!**/*.csv
📒 Files selected for processing (45)
.distignore(1 hunks)bootstrap/loaders.php(1 hunks)core/Cli/Commands.php(5 hunks)core/File_System/File_System.php(1 hunks)core/Notifications/Background_Emailer.php(1 hunks)core/Notifications/Notification.php(1 hunks)core/Pro/Menu.php(1 hunks)core/Promotions/Offers.php(1 hunks)core/Upgrades/Upgrade_2_0.php(4 hunks)core/Upgrades/Upgrade_2_2_1.php(1 hunks)core/WP/Menu.php(3 hunks)libs/functions.php(3 hunks)phpcs-xml/QUICK-REFERENCE.md(1 hunks)phpcs-xml/README.md(1 hunks)phpcs-xml/plugin-check-accessibility.xml(1 hunks)phpcs-xml/plugin-check-general.xml(1 hunks)phpcs-xml/plugin-check-performance.xml(1 hunks)phpcs-xml/plugin-check-plugin-repo.xml(1 hunks)phpcs-xml/plugin-check-security.xml(1 hunks)phpcs.xml.dist(1 hunks)src/Activity/Helper/Activity.php(3 hunks)src/Comment/Helper/Comment.php(2 hunks)src/Discussion_Board/Controllers/Discussion_Board_Controller.php(3 hunks)src/Discussion_Board/Helper/Discussion_Board.php(2 hunks)src/File/Controllers/File_Controller.php(2 hunks)src/File/Helper/File.php(2 hunks)src/Imports/Helpers/Trello_helper.php(1 hunks)src/Milestone/Controllers/Milestone_Controller.php(3 hunks)src/Milestone/Helper/Milestone.php(2 hunks)src/My_Task/Controllers/MyTask_Controller.php(6 hunks)src/Project/Controllers/Project_Controller.php(2 hunks)src/Project/Helper/Project.php(2 hunks)src/Project/Transformers/Project_Transformer.php(3 hunks)src/Pusher/Libs/action.php(2 hunks)src/Pusher/Src/Controllers/Pusher_Controller.php(1 hunks)src/Task/Controllers/Task_Controller.php(2 hunks)src/Task/Helper/Task.php(10 hunks)src/Task_List/Controllers/Task_List_Controller.php(8 hunks)src/Task_List/Helper/Task_List.php(14 hunks)src/Tools/Helpers/ImportActivecollab.php(1 hunks)src/Tools/Helpers/ImportAsana.php(1 hunks)src/Tools/Helpers/ImportTrello.php(1 hunks)src/Tools/Library/PM_Asana.php(2 hunks)src/User/Helper/User.php(3 hunks)src/User/Transformers/User_Transformer.php(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
src/Discussion_Board/Controllers/Discussion_Board_Controller.php (1)
src/Discussion_Board/Models/Discussion_Board.php (2)
files(39-41)Discussion_Board(14-64)
src/Pusher/Libs/action.php (1)
libs/functions.php (1)
pm_clean(969-975)
src/Comment/Helper/Comment.php (2)
src/User/Helper/User.php (1)
orderby(171-224)src/Discussion_Board/Helper/Discussion_Board.php (1)
orderby(636-690)
src/Pusher/Src/Controllers/Pusher_Controller.php (1)
src/Pusher/Core/Auth/Auth.php (1)
socket_auth(36-61)
src/User/Helper/User.php (8)
src/Task/Helper/Task.php (1)
orderby(1837-1899)src/Activity/Helper/Activity.php (1)
orderby(534-603)src/Task_List/Helper/Task_List.php (1)
orderby(921-978)src/Comment/Helper/Comment.php (1)
orderby(342-398)src/Discussion_Board/Helper/Discussion_Board.php (1)
orderby(636-690)src/File/Helper/File.php (1)
orderby(277-332)src/Milestone/Helper/Milestone.php (1)
orderby(532-588)src/Project/Helper/Project.php (1)
orderby(1301-1360)
src/My_Task/Controllers/MyTask_Controller.php (2)
src/Task/Helper/Task.php (2)
where_users(1279-1307)get_results(146-166)src/Activity/Helper/Activity.php (1)
get_results(48-67)
src/Tools/Helpers/ImportActivecollab.php (1)
libs/validation-rules.php (1)
src/Task_List/Helper/Task_List.php (7)
src/Task/Helper/Task.php (1)
orderby(1837-1899)src/Activity/Helper/Activity.php (1)
orderby(534-603)src/Comment/Helper/Comment.php (1)
orderby(342-398)src/Discussion_Board/Helper/Discussion_Board.php (1)
orderby(636-690)src/File/Helper/File.php (1)
orderby(277-332)src/Milestone/Helper/Milestone.php (1)
orderby(532-588)src/Project/Helper/Project.php (1)
orderby(1301-1360)
src/Task_List/Controllers/Task_List_Controller.php (5)
src/Task_List/Observers/Task_List_Observer.php (2)
status(58-72)title(32-38)libs/functions.php (1)
pm_update_meta(235-247)src/Common/Traits/Transformer_Manager.php (1)
get_response(11-27)src/Task_List/Transformers/List_Task_Transformer.php (1)
assignees(140-179)src/Task_List/Models/Task_List.php (1)
assignees(65-69)
core/Pro/Menu.php (1)
libs/functions.php (1)
pm_has_manage_capability(413-427)
src/Discussion_Board/Helper/Discussion_Board.php (8)
src/Task/Helper/Task.php (1)
orderby(1837-1899)src/User/Helper/User.php (1)
orderby(171-224)src/Activity/Helper/Activity.php (1)
orderby(534-603)src/Task_List/Helper/Task_List.php (1)
orderby(921-978)src/Comment/Helper/Comment.php (1)
orderby(342-398)src/File/Helper/File.php (1)
orderby(277-332)src/Milestone/Helper/Milestone.php (1)
orderby(532-588)src/Project/Helper/Project.php (1)
orderby(1301-1360)
core/WP/Menu.php (1)
libs/functions.php (3)
pm_user_can_access(435-448)pm_manager_cap_slug(1318-1320)pm_has_admin_capability(393-406)
🪛 LanguageTool
phpcs-xml/QUICK-REFERENCE.md
[style] ~71-~71: This adverb was used twice in the sentence. Consider removing one of them or replacing them with a synonym.
Context: ...ow summary only - --severity=7 - Show only errors with severity 7 or higher ## Ex...
(ADVERB_REPETITION_PREMIUM)
🪛 markdownlint-cli2 (0.18.1)
phpcs-xml/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
117-117: Bare URL used
(MD034, no-bare-urls)
🪛 PHPMD (2.15.0)
src/Activity/Helper/Activity.php
31-31: Avoid unused private fields such as '$per_page'. (undefined)
(UnusedPrivateField)
src/Task_List/Controllers/Task_List_Controller.php
95-95: Avoid unused local variables such as '$is_archive'. (undefined)
(UnusedLocalVariable)
248-248: Avoid unused local variables such as '$with'. (undefined)
(UnusedLocalVariable)
src/File/Controllers/File_Controller.php
112-112: Avoid unused local variables such as '$file_name'. (undefined)
(UnusedLocalVariable)
core/Cli/Commands.php
88-88: Avoid unused parameters such as '$args'. (undefined)
(UnusedFormalParameter)
89-89: Avoid unused local variables such as '$wpdb'. (undefined)
(UnusedLocalVariable)
517-517: Avoid unused parameters such as '$args'. (undefined)
(UnusedFormalParameter)
core/WP/Menu.php
68-68: Avoid unused local variables such as '$today'. (undefined)
(UnusedLocalVariable)
src/Task/Helper/Task.php
91-91: Avoid unused local variables such as '$self'. (undefined)
(UnusedLocalVariable)
117-117: Avoid unused local variables such as '$key'. (undefined)
(UnusedLocalVariable)
🔇 Additional comments (52)
core/Notifications/Background_Emailer.php (1)
48-48: Good security improvement.Escaping the callback filter value before including it in the error message is a solid defense-in-depth practice that prevents potential output issues if error messages are displayed in admin interfaces or logs.
core/Notifications/Notification.php (1)
110-112: Excellent fixes: namespace correction + escaping improvement.Two solid improvements here:
- Line 110: Using
\Exceptioncorrectly references the global Exception class rather than looking for a non-existent namespaced Exception.- Line 112: Escaping
current_filter()output aligns with the security hardening goals of this PR.src/Tools/Helpers/ImportActivecollab.php (1)
513-515: Email prefix update looks good, but depends on hostname fix.The change from
activecol_toactivecollab_improves clarity. The conditional domain logic (append.comif no dot present) is appropriate for generating fake emails. However, these lines will still produce invalid emails until the hostname extraction bug (lines 510-511) is resolved.src/Tools/Helpers/ImportAsana.php (2)
474-487: Debug output successfully removed from email generation function.The removal of the debug
echostatement frommakeFakeEmailis clean and preserves the hostname sanitization logic. The function correctly generates and sanitizes fake email addresses.However, note that this file still contains multiple
error_log()calls throughout (lines 135, 160, 222, 255, 263, 310–311, 321, 346, 381–382, 402, 417, 491–492). If the PR aims for complete debug cleanup, consider whether these should also be removed or evaluated for production use.
393-401: No changes required — the code is already secure.The
wpdb->insert()function automatically calls$wpdb->prepare()internally and handles data escaping without additional parameters. This is the standard WordPress pattern and is used consistently throughout the codebase. The data sources ($comment->text,$pm_task_id,$pm_project_id,$user_id) are properly escaped by the database layer.src/User/Transformers/User_Transformer.php (2)
298-299: Good: Proper UTC date handling in graph date range initialization and loop.The use of
gmdate()for converting GET parameters (lines 298-299), calculating relative dates (line 302), and incrementing dates in the loop (line 314) correctly normalizes dates to UTC format. This aligns with the PR's goal of UTC-based date handling.Also applies to: 302-302, 314-314
146-146: The gmdate() usage here is correct and requires no changes.The database stores dates in MySQL
TIMESTAMPfields, which automatically handle UTC conversion. When these values are retrieved, they are already in UTC format as strings, andgmdate('Y-m-d', strtotime($item['due_date']))correctly formats them. This pattern is used consistently throughout the codebase (Task_List_Controller.php, Task_Controller.php, Project_Transformer.php, etc.) and is the appropriate way to handle UTC-stored dates in WordPress.Likely an incorrect or invalid review comment.
.distignore (1)
51-51: LGTM!Correctly excludes the PHPCS configuration directory from distribution. Development tooling should not be shipped with the production plugin.
phpcs-xml/QUICK-REFERENCE.md (1)
1-124: Helpful developer documentation.The quick reference guide is well-structured with practical examples for running security audits, generating reports, and using git hooks. The static analysis hint about adverb repetition on line 71 is a false positive—the phrases "Show summary only" and "Show only errors" appear in separate list items with distinct meanings.
phpcs-xml/plugin-check-accessibility.xml (1)
1-30: LGTM!Accessibility ruleset appropriately uses warning severity for HTML-in-PHP detection sniffs, which can have false positives. The ruleset is disabled by default in
phpcs.xml.dist, allowing teams to opt-in when ready.phpcs-xml/plugin-check-general.xml (1)
1-61: LGTM!The ruleset is well-organized with appropriate severity levels.
GlobalVariablesOverridecorrectly uses error severity since overwriting WordPress globals can cause hard-to-debug issues.Note:
Generic.Functions.OpeningFunctionBraceKernighanRitchieenforces K&R brace style—ensure this aligns with the project's existing code style to avoid conflicts.phpcs.xml.dist (1)
63-64: Good addition for debugging.Adding
<arg value="s"/>to show sniff codes in the output is helpful for debugging and selectively excluding specific rules.phpcs-xml/plugin-check-security.xml (2)
1-204: Comprehensive security ruleset structure and organization—well-designed.The ruleset is logically organized into clear categories with inline documentation. Rules are appropriately configured with severity levels (6–7 for critical issues, warnings for non-blocking checks). The inclusion of
WordPress.DateTime.RestrictedFunctions(line 154) aligns with the PR's gmdate() migration goal. Overall architecture supports the PR's security hardening objectives.
1-204: The ruleset already covers HTTP API enforcement viaWordPress.WP.AlternativeFunctions.The
WordPress.WP.AlternativeFunctionsrule (lines 213–222) already flags cURL usage and recommends WordPress HTTP API functions (wp_remote_get, wp_remote_post, wp_remote_request, WP_Http). No additional rule is needed for basic HTTP API migration enforcement.core/Upgrades/Upgrade_2_0.php (1)
1205-1226: UTC normalization of invoice payment date — just confirm downstream expectationsSwitching the stored payment
datetogmdate( 'Y-m-d', strtotime( $payment_date ) )normalizes it to a UTC date, which matches the PR’s goal. For sites previously running in a non‑UTC timezone, this can shift the persisted calendar date by ±1 day compared to the olddate()‑based behavior. Please confirm anything that reads theinvoice_paymentmeta (filters, reports, UI) is either timezone‑agnostic or explicitly expects this field to be UTC‑normalized, not “blog local” date.core/WP/Menu.php (1)
21-53: PHPCS ignores around$submenuare appropriately scopedThe new
phpcs:ignoreannotations are narrowly targeting the intentional$submenuoverrides and explain the rationale, which is a good balance between standards and required behavior.core/Pro/Menu.php (1)
44-55: New Pro submenus are correctly capability‑gatedAdding Calendar unconditionally and Progress/Reports/Modules only when
pm_has_manage_capability()is true keeps advanced views restricted while leaving read‑only calendar access. PHPCS ignores are correctly scoped to the intentional$submenuoverrides.src/Project/Controllers/Project_Controller.php (2)
35-56: Sanitizing query params inindex()is a good hardening stepCasting
per_page/pageto int and sanitizingstatusandproject_transform, plus int‑castingcategory, all reduce risk from malformed or unexpected request data while keeping the rest of the logic unchanged.
164-185: Castingidinshow()improves robustnessUsing
intval( $request->get_param( 'id' ) )beforeProject::find()prevents non‑numeric IDs from leaking into the ORM, which is safer for both routing and DB access.libs/functions.php (3)
105-110: UTC‑based date formatting inpm_date_format()Switching to
gmdate()here makes this helper return UTC‑normalized dates, which is consistent with the rest of the PR’s move to UTC timing and avoids server‑locale surprises.
573-580: UTC timestamps inpm_log()Using
gmdate()for log timestamps standardizes logs to UTC, which is typically preferable for debugging across environments and aligns with other UTC changes in this PR.
124-139:pm_get_wp_roles()correctly useswp_roles()->get_names()as the recommended approachThe implementation follows WordPress core best practices by calling
wp_roles()->get_names()for WordPress 4.3+, which is the officially documented and recommended method for retrieving role labels. The guarded fallback tonew WP_Roles()properly handles older WordPress versions. The PHPCS ignore comment is appropriately narrowed in scope to only the fallback initialization, and is justified for backward compatibility.core/Upgrades/Upgrade_2_2_1.php (1)
24-32: Milestone achieve_date upgrade now normalizes to UTCConverting
$meta_value->datewithgmdate( 'Y-m-d H:i:s', strtotime( ... ) )makes the upgradedachieve_datemeta UTC‑based, matching the broader UTC standardization in this release without altering the surrounding upgrade logic.src/Pusher/Libs/action.php (1)
24-32: UTC normalization for due_date comparisonUsing
gmdate()for both$original_dueand$updated_duekeeps the comparison on a consistent UTC day boundary, which aligns with the rest of the date/time changes and avoids local‑timezone discrepancies when deciding whether to emit a due‑date update.core/File_System/File_System.php (1)
75-87: Good migration to WP_Filesystem API.The use of
$wp_filesystem->put_contents()and$wp_filesystem->size()properly replaces direct PHP file operations. Error handling for write failure is correctly implemented.src/Comment/Helper/Comment.php (1)
352-392: Whitelist-based column validation and escaping looks good.The implementation correctly:
- Defines a whitelist of allowed columns
- Normalizes direction to lowercase
- Validates column against whitelist with strict comparison
- Validates direction against allowed values
- Applies
esc_sql()to both column and directionThis pattern is consistent with other helpers in the codebase.
src/Milestone/Helper/Milestone.php (1)
542-582: Proper ORDER BY whitelist implementation.The implementation correctly validates columns against a whitelist that includes milestone-specific fields like
achieve_date, normalizes direction, and applies escaping. This is consistent with the pattern used across other helpers.src/Discussion_Board/Controllers/Discussion_Board_Controller.php (3)
108-118: Good input validation for file deletion.Proper validation that
files_to_deleteis an array before applyingintvalto each element, with a safe default to an empty array.
211-226: Proper sanitization of user IDs.Good pattern: validate array type, convert to integers, and filter out zero values before processing. This prevents invalid or malicious user IDs from being processed.
242-247: Consistent sanitization in detach_users.The same validation pattern is applied consistently for detaching users.
src/Discussion_Board/Helper/Discussion_Board.php (1)
646-684: Consistent ORDER BY whitelist implementation.The implementation follows the same secure pattern as other helpers, with proper column whitelisting, direction validation, and SQL escaping.
src/File/Helper/File.php (1)
287-320: Whitelist validation logic is correct.The column whitelist and validation logic are properly implemented for file-specific columns. Just needs the table reference fix noted above.
src/Project/Helper/Project.php (1)
1311-1354: Good security hardening for ORDER BY clause.The whitelist-based validation and
esc_sql()escaping properly prevent SQL injection via user-controlled orderby parameters. This is consistent with the same pattern applied across other helper files in this PR.src/Task_List/Helper/Task_List.php (2)
179-180: Spread operator usage for$wpdb->prepare()is correct.The change from
$wpdb->prepare($query, $query_data)to$wpdb->prepare($query, ...$query_data)properly unpacks the array into individual arguments. This pattern is applied consistently across multiple methods in this file.
931-972: Good whitelist-based ORDER BY validation.The implementation correctly validates columns against an allowed list and normalizes direction values. This aligns with the security hardening pattern applied across other helper classes.
bootstrap/loaders.php (1)
167-186: Proper migration to WP_Filesystem API.The implementation correctly:
- Initializes WP_Filesystem with the standard WordPress pattern
- Replaces all direct PHP file operations with their WP_Filesystem equivalents
- Uses
FS_CHMOD_FILEfor appropriate file permissions- Preserves error handling flow
src/My_Task/Controllers/MyTask_Controller.php (3)
28-28: Good change togmdate()for UTC consistency.Using
gmdate()ensures UTC-based date calculations, aligning with the broader date/time normalization effort in this PR.
177-295: Prepared statements properly parameterize date and meta_key values.The multisite query correctly uses placeholders for
$meta_keyand the four occurrences of$startdate. This prevents SQL injection via these parameters.
490-492: Correct prepared statement for user role query.The change properly parameterizes
$user_idwith%dplaceholder, ensuring type safety.src/Task/Helper/Task.php (3)
1849-1893: Good implementation of ORDER BY column whitelisting.The whitelist approach for allowed columns (lines 1850-1863) combined with direction validation (lines 1882-1885) and
esc_sql()escaping (line 1893) provides robust protection against SQL injection in dynamic ORDER BY clauses.
329-344: Good defensive handling for missing users.Returning a structured placeholder object with sensible defaults (like
'display_name' => 'Unknown User') when a user doesn't exist prevents null pointer exceptions downstream and provides graceful degradation.
161-163: Good null safety for single-query results.The early return of an empty array when no results exist prevents potential errors when accessing
$response['data'][0]on empty results.src/Tools/Library/PM_Asana.php (1)
1363-1390: Clean migration to WordPress HTTP API.The request argument assembly and method-specific body handling are well-structured. The use of
wp_remote_requestwith proper timeout, headers, and SSL verification settings follows WordPress best practices.src/Task_List/Controllers/Task_List_Controller.php (2)
609-699: Well-implemented parameterized query construction.The query building logic correctly:
- Sanitizes list IDs with
array_map('intval', ...)(line 609)- Builds placeholders dynamically (line 610)
- Accumulates filter values separately from the query string
- Uses
$wpdb->prepare()with spread operator for final execution (line 699)This is a solid security improvement.
543-547: Good input validation for orders parameter.Adding array type validation before processing
$ordersprevents type errors and provides a clear error response.src/Project/Transformers/Project_Transformer.php (2)
106-107: Potential timezone inconsistency.The code uses
current_time('mysql')(which returns WordPress local time) but formats it withgmdate()(which interprets timestamps as UTC). This combination doesn't actually convert to UTC -strtotime()on a datetime string assumes local timezone.If UTC consistency is the goal, consider:
-$today = gmdate( 'Y-m-d', strtotime( current_time( 'mysql' ) ) ); -$first_day = gmdate( 'Y-m-d', strtotime('-1 month') ); +$today = current_time( 'Y-m-d', true ); // true = GMT +$first_day = gmdate( 'Y-m-d', strtotime('-1 month', current_time('timestamp', true)) );Alternatively, if the local timezone is intentional for user-facing graphs, using
date()would be more semantically clear. Please verify the intended behavior.
117-141: Consistent gmdate usage in date iteration.The date grouping and iteration logic consistently uses
gmdate()for formatting, which ensures uniform date representation across the graph data.src/Milestone/Controllers/Milestone_Controller.php (1)
111-111: LGTM! UTC migration for achieve_date timestamps.The conversion from
date()togmdate()ensures consistent UTC-based storage of milestone achieve dates across all create and update operations. This aligns with the broader UTC standardization effort in the PR.Also applies to: 147-147, 198-198
src/User/Helper/User.php (2)
139-142: LGTM! Prevents SQL error from empty IN clause.The guard correctly prevents execution when the prepared ID list is empty, avoiding SQL syntax errors. This defensive check is consistent with similar improvements across other helpers in this PR.
181-219: LGTM! Essential security hardening for dynamic ORDER BY.The orderby enhancements introduce critical security improvements:
- Column name whitelist prevents SQL injection through orderby parameters
- Direction validation normalizes to 'asc'/'desc' with safe defaults
- esc_sql() escaping provides defense-in-depth
This implementation is consistent with the pattern applied across other helpers (Activity, Comment, File, Milestone, Project, Task, Task_List, Discussion_Board) in this PR.
src/Activity/Helper/Activity.php (2)
353-354: LGTM! UTC migration for date range calculations.The conversion from
date()togmdate()ensures consistent UTC-based date calculations for the BETWEEN query conditions. This aligns with the broader UTC standardization effort across the PR.
543-603: LGTM! Essential security hardening for dynamic ORDER BY with dual format support.The orderby enhancements introduce critical security improvements:
- Column name whitelist prevents SQL injection through orderby parameters
- Supports both string (
'col|dir,col|dir') and array (['col'=>'dir']) formats- Direction validation normalizes to 'asc'/'desc' with safe defaults
- Validates pair structure (skips malformed pairs)
- esc_sql() escaping provides defense-in-depth
This implementation is consistent with the security pattern applied across other helpers in this PR, with additional robustness for handling multiple input formats.
| <exclude-pattern>*/cherry-framework/*</exclude-pattern> | ||
| <exclude-pattern>*/titan-framework/*</exclude-pattern> | ||
| <exclude-pattern>*/plugin-update-checker/*</exclude-pattern> | ||
| <exclude-pattern>*/composer_directory/*</exclude-pattern> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if composer_directory exists as a directory
fd -t d "composer_directory" 2>/dev/null || echo "Directory not found"Repository: weDevsOfficial/wp-project-manager
Length of output: 59
🏁 Script executed:
# Search for references to composer_directory in the entire codebase
rg "composer_directory" --type-list | head -20
rg "composer_directory" 2>/dev/null || echo "No references found"Repository: weDevsOfficial/wp-project-manager
Length of output: 581
🏁 Script executed:
# Examine the phpcs config file to understand the exclude pattern context
cat -n phpcs-xml/plugin-check-plugin-repo.xmlRepository: weDevsOfficial/wp-project-manager
Length of output: 2825
🏁 Script executed:
# Check if there's a main phpcs config referenced
fd -t f "phpcs.xml*" -o "phpcs-ruleset*" 2>/dev/null | head -20Repository: weDevsOfficial/wp-project-manager
Length of output: 59
Remove or clarify the composer_directory exclude pattern.
The pattern */composer_directory/* does not correspond to any directory in the codebase and has no other references in the project. This appears to be dead code or a typo. Either remove it or document why it's needed; if the intent was to exclude Composer dependencies, use the standard */vendor/* pattern instead (which aligns with typical Composer setup).
🤖 Prompt for AI Agents
In phpcs-xml/plugin-check-plugin-repo.xml around line 47, the exclude-pattern
entry "*/composer_directory/*" does not match anything in the repo and should be
removed or clarified; either delete this dead/typo pattern or replace it with
the standard Composer exclusion "*/vendor/*" if the intent was to skip Composer
dependencies, and if you keep a nonstandard name add a comment explaining why
that specific path must be excluded.
| <!-- Plugins should be compatible with PHP 5.6 and higher (updated from 5.2) --> | ||
| <config name="testVersion" value="5.6-"/> | ||
|
|
||
| <!-- Check for PHP compatibility issues --> | ||
| <rule ref="PHPCompatibility"/> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -name "composer.json" -type fRepository: weDevsOfficial/wp-project-manager
Length of output: 307
🌐 Web query:
WordPress 6.7 minimum PHP version requirement
💡 Result:
WordPress 6.7 minimum PHP requirement: PHP 7.2.24 or newer. [1][2]
Sources:
- WordPress Hosting Handbook (server requirements table). [1]
- WordPress Trac / core tickets preparing 6.7 for PHP versions. [2]
🏁 Script executed:
find . -name "composer.json" -type fRepository: weDevsOfficial/wp-project-manager
Length of output: 92
🏁 Script executed:
cat composer.json 2>/dev/null | head -50Repository: weDevsOfficial/wp-project-manager
Length of output: 1586
🏁 Script executed:
# Also check if there's any documentation about minimum PHP version for this plugin checker
fd "README" -type f | head -5Repository: weDevsOfficial/wp-project-manager
Length of output: 247
🏁 Script executed:
cat phpcs-xml/plugin-check-plugin-repo.xmlRepository: weDevsOfficial/wp-project-manager
Length of output: 2405
🏁 Script executed:
# Search for any documentation or comments about PHP version targets
rg "testVersion|PHP.*7\.[0-9]|6\.[0-9]" phpcs-xml/ -A 2 -B 2Repository: weDevsOfficial/wp-project-manager
Length of output: 443
PHP 5.6 compatibility target is outdated; update to PHP 7.2 minimum.
PHP 5.6 reached end-of-life in December 2018. WordPress 6.7 (current) requires PHP 7.2.24 minimum. Update the testVersion to align with current WordPress requirements.
- <!-- Plugins should be compatible with PHP 5.6 and higher (updated from 5.2) -->
- <config name="testVersion" value="5.6-"/>
+ <!-- Plugins should be compatible with PHP 7.2 and higher (WordPress 6.7+ requirement) -->
+ <config name="testVersion" value="7.2-"/>The PHPCompatibility package is already properly installed in composer.json as a require-dev dependency.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <!-- Plugins should be compatible with PHP 5.6 and higher (updated from 5.2) --> | |
| <config name="testVersion" value="5.6-"/> | |
| <!-- Check for PHP compatibility issues --> | |
| <rule ref="PHPCompatibility"/> | |
| <!-- Plugins should be compatible with PHP 7.2 and higher (WordPress 6.7+ requirement) --> | |
| <config name="testVersion" value="7.2-"/> | |
| <!-- Check for PHP compatibility issues --> | |
| <rule ref="PHPCompatibility"/> |
🤖 Prompt for AI Agents
In phpcs-xml/plugin-check-plugin-repo.xml around lines 54 to 58, the testVersion
is set to "5.6-" which is outdated; change it to target PHP 7.2 minimum (e.g.
"7.2-") so the PHPCompatibility checks match current WordPress requirements,
updating the value of the config name="testVersion" to "7.2-" to enforce PHP
7.2+ compatibility.
| <rule ref="WordPress.WP.AlternativeFunctions"> | ||
| <type>error</type> | ||
| <exclude name="WordPress.WP.AlternativeFunctions.json_encode_json_encode"/> | ||
| <exclude name="WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents"/> | ||
| <exclude name="WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents"/> | ||
| </rule> |
There was a problem hiding this comment.
Remove exclusions for file_get_contents and file_put_contents—contradicts PR migration goals.
Lines 175-177 exclude warnings for file_get_contents and file_put_contents via WordPress.WP.AlternativeFunctions, yet the PR explicitly aims to migrate these direct PHP file operations to the WordPress Filesystem API (WP_Filesystem). Suppressing these warnings undermines the security hardening goals and would allow future code to bypass the intended standard.
Remove the exclusions so PHPCS enforces the migration to WP_Filesystem:
<rule ref="WordPress.WP.AlternativeFunctions">
<type>error</type>
- <exclude name="WordPress.WP.AlternativeFunctions.json_encode_json_encode"/>
- <exclude name="WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents"/>
- <exclude name="WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents"/>
+ <exclude name="WordPress.WP.AlternativeFunctions.json_encode_json_encode"/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <rule ref="WordPress.WP.AlternativeFunctions"> | |
| <type>error</type> | |
| <exclude name="WordPress.WP.AlternativeFunctions.json_encode_json_encode"/> | |
| <exclude name="WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents"/> | |
| <exclude name="WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents"/> | |
| </rule> | |
| <rule ref="WordPress.WP.AlternativeFunctions"> | |
| <type>error</type> | |
| <exclude name="WordPress.WP.AlternativeFunctions.json_encode_json_encode"/> | |
| </rule> |
🤖 Prompt for AI Agents
In phpcs-xml/plugin-check-security.xml around lines 173 to 178, the rule block
currently excludes file_get_contents and file_put_contents which prevents PHPCS
from flagging direct PHP file operations; remove the two <exclude> entries named
"WordPress.WP.AlternativeFunctions.file_get_contents_file_get_contents" and
"WordPress.WP.AlternativeFunctions.file_system_operations_file_put_contents" so
PHPCS will enforce migrating to WP_Filesystem, and ensure the resulting XML
remains well-formed (keep the json_encode exclusion if intended).
| die( esc_html__( 'file not found', 'wedevs-project-manager' ) ); | ||
| } | ||
|
|
||
| $file_name = basename( $path ); |
There was a problem hiding this comment.
Remove unused variable $file_name.
This variable is assigned but never used. Line 130 uses basename($path) directly instead.
- $file_name = basename( $path );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $file_name = basename( $path ); |
🧰 Tools
🪛 PHPMD (2.15.0)
112-112: Avoid unused local variables such as '$file_name'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
In src/File/Controllers/File_Controller.php around line 112, the variable
$file_name is assigned with basename($path) but never used later (the code calls
basename($path) directly at line 130); remove the unused assignment to eliminate
dead code — delete the line that sets $file_name = basename($path); so the code
uses basename($path) where needed without the unused variable.
| // Replace readfile with WP_Filesystem method | ||
| echo $wp_filesystem->get_contents( $path ); |
There was a problem hiding this comment.
Potential memory issue with large file downloads.
Replacing readfile() with echo $wp_filesystem->get_contents() loads the entire file into memory before outputting. For large files, this could exhaust PHP memory limits.
Consider using WP_Filesystem's direct file reading with output buffering or streaming, or keeping readfile() for the download functionality since it streams content directly.
- // Replace readfile with WP_Filesystem method
- echo $wp_filesystem->get_contents( $path );
+ // For downloads, readfile() is more memory-efficient as it streams content
+ // phpcs:ignore WordPress.WP.AlternativeFunctions.file_system_read_readfile
+ readfile( $path );🤖 Prompt for AI Agents
In src/File/Controllers/File_Controller.php around lines 132-133, replacing
readfile() with echo $wp_filesystem->get_contents($path) can exhaust PHP memory
for large files because it loads the whole file into memory; revert to a
streaming approach instead — either restore readfile($path) (preferred for
direct streaming) or open the file and stream in chunks (fopen +
while/fpassthru/streamed read) while preserving existing download headers and
ensuring output buffering is flushed and disabled before streaming.
| public function make_request($source,$querystring,$calback){ | ||
| // Get cURL resource | ||
| $curl = curl_init(); | ||
| // Set some options - we are passing in a useragent too here | ||
| curl_setopt_array($curl, [ | ||
| CURLOPT_RETURNTRANSFER => 1, | ||
| CURLOPT_URL => $this->remote_addr($source,$querystring) | ||
| // Use WordPress HTTP API instead of cURL | ||
| $url = $this->remote_addr($source,$querystring); | ||
|
|
||
| // Make the request using wp_remote_get | ||
| $response = wp_remote_get($url, [ | ||
| 'timeout' => 30, | ||
| 'sslverify' => true | ||
| ]); | ||
| // Send the request & save response to $resp | ||
| $resp = curl_exec($curl); | ||
| // Close request to clear up some resources | ||
| curl_close($curl); | ||
|
|
||
| // Check for errors | ||
| if (is_wp_error($response)) { | ||
| return $calback(json_encode(['error' => $response->get_error_message()])); | ||
| } | ||
|
|
||
| // Get the response body | ||
| $resp = wp_remote_retrieve_body($response); | ||
|
|
||
| return $calback($resp); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
WordPress wp_remote_get() best practices HTTP status code error handling
💡 Result:
Practical best practices for wp_remote_get() and HTTP status‑code handling
- Always check for transport errors first using is_wp_error(). If true, log/handle the WP_Error. [1]
- Then check the HTTP status code with wp_remote_retrieve_response_code(). Treat only 2xx (usually 200) as success; handle other codes explicitly. [1][2]
- 200: process body (wp_remote_retrieve_body()) and verify/parse (e.g., json_decode + JSON error checks). [1][2]
- 3xx: follow redirects (WP HTTP does by default) or handle location header if you expect specific redirect logic. [2]
- 4xx (client errors): don’t retry immediately; surface useful messages, back off, and validate request parameters/auth. 429 (rate limit): implement exponential backoff and honor Retry‑After header if present. [2][3]
- 5xx (server errors): retry with exponential backoff a limited number of times; escalate/alert if persistent. [2][3]
- Respect and read headers: use wp_remote_retrieve_header() for Retry‑After, ETag/Last‑Modified for conditional requests, and rate‑limit headers. Use conditional requests to reduce load. [2]
- Timeouts and redirection: set sensible args (timeout, redirection) and a clear user‑facing fallback for long/blocking requests. Avoid huge timeouts on front‑end requests. [1][3]
Example args: ['timeout'=>5, 'redirection'=>5, 'user-agent'=>..., 'headers'=>[...] ]. [1][3] - Use caching (transients or object cache) for API responses to avoid repeated network calls; refresh in background (cron or WP‑CLI) and use ETag/Last‑Modified to validate cache. Don’t cache enormous payloads. [3]
- Retries & backoff: implement capped retries with exponential backoff for intermittent network/server errors (5xx, timeouts). For 429, respect Retry‑After header if provided. [2][3]
- Security: never disable SSL verification in production; validate and sanitize all returned data before use. Use authentication headers securely (never embed secrets in client‑side code). [2][3]
- Non‑blocking / background: for user‑facing pages, use non‑blocking requests (blocking=>false) or async background jobs to avoid slowing page loads. [1][3]
- Limit response size and validate content-type: check Content‑Type and limit processing for very large responses to avoid memory issues. [1][7]
- Instrumentation: log status codes, latencies, errors, and retry attempts so you can monitor external API reliability and adjust timeouts/retry policies. [2][3]
Minimal example pattern
- Do is_wp_error() check → read response code → if 200 parse and validate body → else branch by code (429 backoff, 4xx show error, 5xx retry). Cache successful payloads and honor headers like ETag/Retry‑After. [1][2][3]
References
- wp_remote_get() reference and helpers (wp_remote_retrieve_body/response_code). [1]
- “Getting data from an external service” — WordPress Common APIs Handbook (status codes, headers, conditional requests). [2]
- Practical tips: caching, retries, timeouts and handling rate limits. [3]
Sources
[1] WordPress Developer Reference: wp_remote_get().
[2] WordPress Developer Handbook — Getting data from an external service.
[3] Best‑practice guides: caching & retry examples (transients/background refresh, rate‑limit handling).
🌐 Web query:
WordPress wp_remote_retrieve_response_code() documentation external API
💡 Result:
wp_remote_retrieve_response_code() — short summary, usage, and behavior
-
Purpose: returns the HTTP status code from a response array returned by WordPress HTTP functions (wp_remote_get, wp_remote_post, wp_remote_request). [1][2]
-
Signature: wp_remote_retrieve_response_code( array|WP_Error $response ) : int|string. Returns the numeric HTTP status code (e.g. 200, 404) or an empty string if the parameter is invalid or a WP_Error. [1]
-
Typical usage:
$response = wp_remote_get( 'https://api.example.com/endpoint' );
if ( is_wp_error( $response ) ) {
// handle transport error
} else {
$code = wp_remote_retrieve_response_code( $response );
if ( 200 === (int) $code ) {
$body = wp_remote_retrieve_body( $response );
// process body
} else {
// handle non-200 HTTP status
}
} [2][3] -
Implementation note: it reads $response['response']['code'] and returns '' when $response is a WP_Error or the expected array keys are missing. [1]
Sources:
[1] Function reference: wp_remote_retrieve_response_code() — Developer.WordPress.org.
[2] Making HTTP requests / Getting data from an external service — Developer.WordPress.org.
[3] wp_remote_get() function reference — Developer.WordPress.org.
Migrate to wp_remote_get() and add HTTP status code validation
Good move switching to WordPress's HTTP API for better SSL/proxy handling and cleaner error management via is_wp_error() and wp_remote_retrieve_body(). However, the implementation is incomplete: it handles transport errors but treats all HTTP responses (including 4xx/5xx) as successful. When consuming external APIs like Trello, non-2xx responses should be treated as errors.
Add HTTP status code validation after the error check:
// Check for errors
if (is_wp_error($response)) {
return $calback(json_encode(['error' => $response->get_error_message()]));
}
+ // Validate HTTP status code
+ $code = wp_remote_retrieve_response_code($response);
+ if ((int)$code < 200 || (int)$code >= 300) {
+ return $calback(json_encode(['error' => 'HTTP ' . $code]));
+ }
+
// Get the response body
$resp = wp_remote_retrieve_body($response);
return $calback($resp);This prevents silently treating Trello error responses (404, 429, 500, etc.) as valid data.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function make_request($source,$querystring,$calback){ | |
| // Get cURL resource | |
| $curl = curl_init(); | |
| // Set some options - we are passing in a useragent too here | |
| curl_setopt_array($curl, [ | |
| CURLOPT_RETURNTRANSFER => 1, | |
| CURLOPT_URL => $this->remote_addr($source,$querystring) | |
| // Use WordPress HTTP API instead of cURL | |
| $url = $this->remote_addr($source,$querystring); | |
| // Make the request using wp_remote_get | |
| $response = wp_remote_get($url, [ | |
| 'timeout' => 30, | |
| 'sslverify' => true | |
| ]); | |
| // Send the request & save response to $resp | |
| $resp = curl_exec($curl); | |
| // Close request to clear up some resources | |
| curl_close($curl); | |
| // Check for errors | |
| if (is_wp_error($response)) { | |
| return $calback(json_encode(['error' => $response->get_error_message()])); | |
| } | |
| // Get the response body | |
| $resp = wp_remote_retrieve_body($response); | |
| return $calback($resp); | |
| } | |
| public function make_request($source,$querystring,$calback){ | |
| // Use WordPress HTTP API instead of cURL | |
| $url = $this->remote_addr($source,$querystring); | |
| // Make the request using wp_remote_get | |
| $response = wp_remote_get($url, [ | |
| 'timeout' => 30, | |
| 'sslverify' => true | |
| ]); | |
| // Check for errors | |
| if (is_wp_error($response)) { | |
| return $calback(json_encode(['error' => $response->get_error_message()])); | |
| } | |
| // Validate HTTP status code | |
| $code = wp_remote_retrieve_response_code($response); | |
| if ((int)$code < 200 || (int)$code >= 300) { | |
| return $calback(json_encode(['error' => 'HTTP ' . $code])); | |
| } | |
| // Get the response body | |
| $resp = wp_remote_retrieve_body($response); | |
| return $calback($resp); | |
| } |
🤖 Prompt for AI Agents
In src/Imports/Helpers/Trello_helper.php around lines 36 to 55, after the
is_wp_error() check add HTTP status validation: retrieve the response code
(using wp_remote_retrieve_response_code or $response['response']['code']) and if
it is not a 2xx value, treat it as an error — call the provided $calback with a
JSON-encoded error that includes the HTTP status and the response body (from
wp_remote_retrieve_body) so non-2xx Trello responses (404/429/500, etc.) aren’t
treated as successful.
| $hostname = str_replace('http', '',get_site_url()); | ||
| $hostname = str_replace('://', '',$hostname); | ||
| echo $hostname; | ||
| $hostname = str_replace('://', '', $hostname); |
There was a problem hiding this comment.
Critical bug: Hostname extraction breaks for HTTPS URLs.
The current approach removes 'http' first (line 510), leaving 's://' for HTTPS sites, then removes '://' (line 511), resulting in 's:example.com'. This produces invalid email addresses like activecollab_user@s:example.com because the colon violates email format standards.
Apply this diff to correctly extract the hostname:
$mailuser = preg_replace('/[^A-Za-z0-9\-]/', '', $mailuser);
-$hostname = str_replace('http', '',get_site_url());
-$hostname = str_replace('://', '', $hostname);
+$hostname = parse_url(get_site_url(), PHP_URL_HOST);
if (strpos($hostname, ".")) {🤖 Prompt for AI Agents
In src/Tools/Helpers/ImportActivecollab.php around lines 510-511, the hostname
extraction currently strips 'http' then '://', which breaks HTTPS (leaving
's:example.com'); replace this logic by parsing the site URL to extract the host
component (e.g., use parse_url(get_site_url(), PHP_URL_HOST) or WP's
wp_parse_url) and fall back to get_site_url() or rtrim as needed; ensure the
final hostname contains only the host (no scheme, port, or colon) before being
used in email generation.
| echo '<pre>'; | ||
| echo 'WordPress HTTP API Error: ' . esc_html($response->get_error_message()); | ||
| echo '</pre>'; | ||
| echo '<br>Sent info:<br><pre>'; | ||
| print_r($data); | ||
| echo '</pre>'; | ||
| } |
There was a problem hiding this comment.
Debug output may expose sensitive data.
When $this->debug or $this->advDebug is enabled, print_r($data) outputs the request body which may contain sensitive API keys, tokens, or user data. Consider sanitizing or limiting debug output.
if ($this->debug || $this->advDebug) {
echo '<pre>';
echo 'WordPress HTTP API Error: ' . esc_html($response->get_error_message());
echo '</pre>';
- echo '<br>Sent info:<br><pre>';
- print_r($data);
- echo '</pre>';
+ if ($this->advDebug) {
+ echo '<br>Sent data type: ' . esc_html(gettype($data));
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| echo '<pre>'; | |
| echo 'WordPress HTTP API Error: ' . esc_html($response->get_error_message()); | |
| echo '</pre>'; | |
| echo '<br>Sent info:<br><pre>'; | |
| print_r($data); | |
| echo '</pre>'; | |
| } | |
| echo '<pre>'; | |
| echo 'WordPress HTTP API Error: ' . esc_html($response->get_error_message()); | |
| echo '</pre>'; | |
| if ($this->advDebug) { | |
| echo '<br>Sent data type: ' . esc_html(gettype($data)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/Tools/Library/PM_Asana.php around lines 1398-1404, the current debug path
echoes the raw $data which can expose sensitive tokens/keys; replace the raw
print_r with a sanitized representation: when $this->debug or $this->advDebug is
enabled, create a copy of $data and for any keys matching sensitive patterns
(e.g. token|key|secret|password|authorization|access_token) redact or mask their
values, truncate very long values, or alternatively only output keys and value
types/counts, then echo the sanitized output instead of the raw $data.
| public function includeMeta ( User $user ) { | ||
| return $this->item ('', function () use ( $user ) { | ||
| $today = date( 'Y-m-d', strtotime( current_time( 'mysql' ) ) ); | ||
| $today = gmdate( 'Y-m-d', strtotime( current_time( 'mysql' ) ) ); |
There was a problem hiding this comment.
Critical: Incorrect timezone handling with current_time() and gmdate().
The pattern gmdate( 'Y-m-d', strtotime( current_time( 'mysql' ) ) ) is problematic because current_time('mysql') returns local WordPress time, not UTC. When passed through strtotime() and gmdate(), this creates timezone confusion and produces incorrect dates for sites in non-UTC timezones, affecting task filtering and graph calculations.
Apply this diff to use proper UTC date handling:
-$today = gmdate( 'Y-m-d', strtotime( current_time( 'mysql' ) ) );
+$today = gmdate( 'Y-m-d' );For all four occurrences at lines 116, 162, 182, and 301. Alternatively, use:
$today = current_datetime()->setTimezone( new DateTimeZone( 'UTC' ) )->format( 'Y-m-d' );Also applies to: 162-162, 182-182, 301-301
🤖 Prompt for AI Agents
In src/User/Transformers/User_Transformer.php around lines 116, 162, 182 and
301, replace the incorrect timezone handling that uses gmdate('Y-m-d',
strtotime(current_time('mysql'))) with a proper UTC-aware DateTime approach;
specifically obtain the current WP datetime, set its timezone to UTC, and format
as 'Y-m-d' (e.g. current_datetime()->setTimezone(new
DateTimeZone('UTC'))->format('Y-m-d')), ensuring each of the four occurrences is
updated so dates are computed in UTC rather than converting local WP time via
strtotime/gmdate.
There was a problem hiding this comment.
this is the current_time function body do you think it will make problem ?
@CodeRabbit
/**
-
Retrieves the current time based on specified type.
-
- The 'mysql' type will return the time in the format for MySQL DATETIME field.
-
- The 'timestamp' or 'U' types will return the current timestamp or a sum of timestamp
-
and timezone offset, depending on
$gmt. -
- Other strings will be interpreted as PHP date formats (e.g. 'Y-m-d').
-
If
$gmtis a truthy value then both types will use GMT time, otherwise the -
output is adjusted with the GMT offset for the site.
-
@SInCE 1.0.0
-
@SInCE 5.3.0 Now returns an integer if
$typeis 'U'. Previously a string was returned. -
@param string $type Type of time to retrieve. Accepts 'mysql', 'timestamp', 'U',
-
or PHP date format string (e.g. 'Y-m-d'). -
@param bool $gmt Optional. Whether to use GMT timezone. Default false.
-
@return int|string Integer if
$typeis 'timestamp' or 'U', string otherwise.
*/
function current_time( $type, $gmt = false ) {
// Don't use non-GMT timestamp, unless you know the difference and really need to.
if ( 'timestamp' === $type || 'U' === $type ) {
return $gmt ? time() : time() + (int) ( (float) get_option( 'gmt_offset' ) * HOUR_IN_SECONDS );
}if ( 'mysql' === $type ) {
$type = 'Y-m-d H:i:s';
}$timezone = $gmt ? new DateTimeZone( 'UTC' ) : wp_timezone();
$datetime = new DateTime( 'now', $timezone );return $datetime->format( $type );
}
There was a problem hiding this comment.
@iftakharul-islam Yes, this will cause a problem. Let me explain why with the function body you've provided:
The Problem
When you call current_time('mysql') without the second parameter (or with $gmt = false):
$timezone = $gmt ? new DateTimeZone('UTC') : wp_timezone(); // Uses site's local timezone
$datetime = new DateTime('now', $timezone);
return $datetime->format('Y-m-d H:i:s'); // Returns LOCAL time like "2025-12-04 15:30:00"This returns a datetime string in your site's local timezone (e.g., EST, PST, etc.).
Then when you do:
$today = gmdate('Y-m-d', strtotime(current_time('mysql')));Here's what happens:
current_time('mysql')returns"2025-12-04 15:30:00"in your site's timezone (e.g., EST)strtotime()parses this string, but it interprets it in PHP's default timezone context (which may differ from WordPress timezone)gmdate()then formats the result as UTC
This mixing of timezone contexts causes incorrect date calculations, especially:
- Near midnight boundaries (you might get yesterday's or tomorrow's date)
- For sites in timezones far from UTC
- When comparing dates stored in UTC in your database
The Solution
Use one of these approaches to get a proper UTC date:
Option 1 (Simplest):
$today = gmdate('Y-m-d'); // Uses current Unix timestamp, formats in UTCOption 2 (Using current_time correctly):
$today = current_time('Y-m-d', true); // Second param true = get GMT/UTC timeOption 3 (Most explicit):
$today = current_datetime()->setTimezone(new DateTimeZone('UTC'))->format('Y-m-d');All these ensure you're working in a consistent UTC context throughout. I recommend Option 1 (gmdate('Y-m-d')) as it's the simplest and most direct.
Rubaiyat-E-Mohammad
added
QA in Progress
QA Approved
Corrected the conditional logic in Offers::dismiss_offer to use OR instead of AND, ensuring proper validation of POST parameters. Updated Task_Controller to consistently use gmdate for date comparisons, improving date handling and consistency across different due date filters.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Task/Controllers/Task_Controller.php (2)
767-780: Inconsistent date/time handling mixes UTC and local time.Lines 769, 772, and 775 now use
gmdate()for$today, but line 777 still usesdate()for$last. This creates a date range where one boundary is in UTC and the other is in local time, causing incorrect filtering when the server timezone differs from UTC.Additionally, the pattern
gmdate('Y-m-d', strtotime(current_time('mysql')))is problematic becausecurrent_time('mysql')without the second parameter returns local time, which is then parsed and reformatted as GMT—this can introduce subtle timezone conversion issues.Apply this diff to use UTC consistently:
if ( ! empty( $due_date ) ) { if( $due_date == 'overdue' ) { - $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); + $today = gmdate('Y-m-d', current_time('timestamp', true)); $q->where( 'due_date', '<', $today ); } else if ( $due_date == 'today' ) { - $today = gmdate('Y-m-d', strtotime( current_time('mysql') ) ); + $today = gmdate('Y-m-d', current_time('timestamp', true)); $q->where( 'due_date', $today ); } else if ( $due_date == 'week' ) { - $today = gmdate('Y-m-d', strtotime( current_time('mysql') ) ); - $last = date('Y-m-d', strtotime( current_time('mysql') . '-1 week' ) ); + $today = gmdate('Y-m-d', current_time('timestamp', true)); + $last = gmdate('Y-m-d', strtotime('-1 week', current_time('timestamp', true))); $q->where( 'due_date', '>=', $last ); $q->where( 'due_date', '<=', $today ); } }
807-821: Inconsistent date/time handling mixes UTC and local time (duplicate code).This code block has the same UTC/local time inconsistency as lines 767-780:
$todayusesgmdate()but$laston line 817 usesdate(), creating incorrect date ranges for week filtering.Additionally, this entire filtering logic (lines 797-836) duplicates the logic in lines 755-794. Consider extracting this into a private method to reduce duplication and ensure consistency.
Apply this diff to fix the date handling:
if ( ! empty( $due_date ) ) { if( $due_date == 'overdue' ) { - $today = gmdate( 'Y-m-d', strtotime( current_time('mysql') ) ); + $today = gmdate('Y-m-d', current_time('timestamp', true)); $q->where( 'due_date', '<', $today ); } else if ( $due_date == 'today' ) { - $today = gmdate('Y-m-d', strtotime( current_time('mysql') ) ); + $today = gmdate('Y-m-d', current_time('timestamp', true)); $q->where( 'due_date', $today ); } else if ( $due_date == 'week' ) { - $today = gmdate('Y-m-d', strtotime( current_time('mysql') ) ); - $last = date('Y-m-d', strtotime( current_time('mysql') . '-1 week' ) ); + $today = gmdate('Y-m-d', current_time('timestamp', true)); + $last = gmdate('Y-m-d', strtotime('-1 week', current_time('timestamp', true))); $q->where( 'due_date', '>=', $last ); $q->where( 'due_date', '<=', $today ); } }To eliminate duplication, extract the due date filtering logic into a reusable method that both closures can call.
core/Promotions/Offers.php (1)
43-44: Timezone handling inconsistent with PR objectives.The PR objectives emphasize UTC consistency across date/time handling, but this code uses
America/New_Yorktimezone for promotional offer scheduling. This creates a timezone dependency that may cause confusion for international deployments.Consider migrating to UTC-based comparisons for consistency with the broader codebase changes.
♻️ Duplicate comments (1)
core/Promotions/Offers.php (1)
136-138: Security issue remains unaddressed: missingrel="noopener noreferrer".The anchor tag still opens in a new tab with
target="_blank"but lacks therelattribute, leaving the site vulnerable to reverse-tabnabbing. This was previously flagged in an earlier review.Apply the suggested fix from the previous review:
- <a class="link" target="_blank" href="<?php echo esc_url( $offer->link ); ?>"> + <a class="link" target="_blank" rel="noopener noreferrer" href="<?php echo esc_url( $offer->link ); ?>"> <?php echo esc_html( $offer->btn_txt ); ?> </a>
🧹 Nitpick comments (3)
core/Promotions/Offers.php (3)
50-50: Localize the fallback button text.The fallback
'Get Now'should be wrapped in a translation function for consistency with WordPress i18n practices.- $offer->btn_txt = ! empty( $promo_notice['action_title'] ) ? $promo_notice['action_title'] : 'Get Now'; + $offer->btn_txt = ! empty( $promo_notice['action_title'] ) ? $promo_notice['action_title'] : __( 'Get Now', 'wedevs-project-manager' );
52-58: Remove unnecessary translation wrappers.The
sprintf( __( '%s', ... ), $variable )pattern provides no i18n value since the content comes from external JSON and contains no translatable text. This adds unnecessary overhead.- $offer->message[] = sprintf( __( '<strong>%s</strong>', 'wedevs-project-manager' ), $promo_notice['title'] ); + $offer->message[] = '<strong>' . $promo_notice['title'] . '</strong>'; if ( ! empty( $promo_notice['description'] ) ) { - $offer->message[] = sprintf( __( '%s', 'wedevs-project-manager' ), $promo_notice['description'] ); + $offer->message[] = $promo_notice['description']; } - $offer->message[] = sprintf( __( '%s', 'wedevs-project-manager' ), $promo_notice['content'] ); + $offer->message[] = $promo_notice['content'];
82-82: Consider sanitizing direct superglobal access.While the value is only used for comparison, sanitizing input aligns with the PR's focus on security improvements and WordPress coding standards.
- if ( ! isset( $_GET['page'] ) || 'pm_projects' !== $_GET['page'] ) { + if ( ! isset( $_GET['page'] ) || 'pm_projects' !== sanitize_text_field( wp_unslash( $_GET['page'] ) ) ) { return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/Promotions/Offers.php(2 hunks)src/Task/Controllers/Task_Controller.php(2 hunks)
🔇 Additional comments (2)
core/Promotions/Offers.php (2)
137-137: LGTM: Proper escaping for button text.The
esc_html()function correctly escapes the button text for safe HTML output, protecting against XSS if the external JSON source is compromised.
174-174: LGTM: Improved guard condition logic.Changing from
&&to||strengthens the validation by ensuring both required parameters are present before proceeding. This correctly rejects requests missing either the nonce or the offer key, aligning with secure input validation patterns.
Wrapped the default button text 'Get Now' in a translation function for internationalization support. Added rel="noopener noreferrer" to offer links for improved security when opening in a new tab.
|
@copilot Please the coderabbit suggestions here and give a overall implementations suggestion according to our projects requirements |
|
@iftakharul-islam I've opened a new pull request, #569, to work on those changes. Once the pull request is ready, I'll request review from you. |
This pull request introduces several improvements focused on WordPress file handling, date/time consistency, and code standards compliance. The most significant changes are the migration to the WordPress Filesystem API for file operations, consistent use of
gmdatefor UTC-based date/time values, and enhanced code standards documentation and compliance. These updates improve security, reliability, and maintainability across the codebase.File Handling and Security
file_put_contents,file_get_contents,filesize, etc.) to use the WordPress Filesystem API (WP_Filesystem) for better compatibility and security incore/File_System/File_System.phpand SVG sanitization logic inbootstrap/loaders.php. [1] [2]Date and Time Consistency
date()withgmdate()for storing and displaying dates/times in UTC across upgrade scripts and utility functions (core/Upgrades/Upgrade_2_0.php,core/Upgrades/Upgrade_2_2_1.php,libs/functions.php,core/WP/Menu.php). [1] [2] [3] [4] [5] [6] [7] [8]Code Standards and Documentation
phpcs-xml/QUICK-REFERENCE.mdand included thephpcs-xmldirectory in.distignoreto support code standards enforcement and developer onboarding. [1] [2]Admin Menu and Global Variable Handling
phpcs:ignorecomments when overriding global variables for WordPress admin menus to clarify intentional design decisions and maintain standards compliance incore/WP/Menu.phpandcore/Pro/Menu.php. Improved role fetching logic to usewp_roles()when available. [1] [2] [3] [4]Minor Security and Escaping Improvements
Summary by CodeRabbit
New Features
Documentation
Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.