-
Notifications
You must be signed in to change notification settings - Fork 27
[WIP] Add a logger #1124
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
base: development
Are you sure you want to change the base?
[WIP] Add a logger #1124
Conversation
cd0e345
to
9c475df
Compare
Plugin build for 224275f is ready 🛎️!
Note You can preview the changes in the Playground |
9c475df
to
574bade
Compare
71664d0
to
ae65e5c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive logging functionality to Feedzy RSS Feeds, introducing a structured logging system to help debug import issues and monitor system health.
- Implements a new JSON-based logger with configurable log levels (debug, info, warning, error, none)
- Adds a logs management interface in the settings with filtering, export, and email reporting capabilities
- Replaces legacy
themeisle_log_event
usage throughout the codebase with the new logging system
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
includes/admin/feedzy-rss-feeds-log.php | Core logging class implementing singleton pattern with file-based JSON logging |
includes/admin/feedzy-rss-feeds-task-manager.php | Manages scheduled tasks for log cleanup and email reporting |
includes/layouts/settings.php | Updated settings page to include logs configuration and viewer tab |
includes/layouts/feedzy-logs-viewer.php | New logs viewer interface with filtering and export functionality |
includes/layouts/feedzy-email-report.php | Email template for weekly error reports |
tests/test-log.php | Comprehensive unit tests for the logging functionality |
tests/e2e/specs/logger.spec.js | End-to-end tests for the logging interface |
includes/admin/feedzy-rss-feeds-import.php | Updated import process to use new logging system extensively |
css/settings.css | Styling for the new logs interface |
js/feedzy-setting.js | JavaScript for log management UI interactions |
|
||
if ( is_wp_error( $logs ) ) { | ||
printf( | ||
'<p>%$1s(%2$s)</p>', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a syntax error in the printf function call. The format string has mismatched placeholders - it should be '%1$s (%2$s)' but currently shows '%$1s(%2$s)' with incorrect placeholder syntax.
'<p>%$1s(%2$s)</p>', | |
'<p>%1$s (%2$s)</p>', |
Copilot uses AI. Check for mistakes.
header( 'Expires: 0' ); | ||
header( 'Connection: Keep-Alive' ); | ||
header( 'Cache-Control: must-revalidate' ); | ||
header( 'Pragma: public' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code uses filesize()
directly without checking if the file exists, but earlier checks use $this->log_file_exists()
and $this->is_log_readable()
. This could cause a PHP warning if the file doesn't exist at this point due to race conditions.
header( 'Pragma: public' ); | |
header( 'Pragma: public' ); | |
if ( ! $this->log_file_exists() || ! $this->is_log_readable() ) { | |
return new WP_Error( 'no_logs', '', array( 'status' => 404 ) ); | |
} |
Copilot uses AI. Check for mistakes.
header( 'Cache-Control: must-revalidate' ); | ||
header( 'Pragma: public' ); | ||
header( 'Content-Length: ' . filesize( $this->filepath ) ); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the filesize issue, readfile()
is called without checking if the file still exists. Consider adding a final file existence check before these file operations.
// Final check before reading the file. | |
if ( ! file_exists( $this->filepath ) || ! is_readable( $this->filepath ) ) { | |
return new WP_Error( 'no_logs', '', array( 'status' => 404 ) ); | |
} |
Copilot uses AI. Check for mistakes.
} elseif ( strpos( $feed_img_tag, '[#item_custom' ) !== false ) { | ||
$value = ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The variable $value
is declared but may not be used when the business/personal conditions are false. Consider moving this declaration inside the conditional block or providing a default value that's more meaningful.
$value = ''; | |
$value = null; |
Copilot uses AI. Check for mistakes.
tests/test-image-import.php
Outdated
@@ -70,7 +76,7 @@ public function test_import_image_special_characters() { | |||
$import_errors = array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The variable $import_errors
is declared but never used in this test method. Since the logging system has changed, this variable appears to be leftover from the old implementation and should be removed.
Copilot uses AI. Check for mistakes.
@Soare-Robert-Daniel, I noticed two things so far:
![]() |
This is intentional. The file is for us to analyze, its scope is to be given to support, then posted into the Github issues.
Yes |
@Soare-Robert-Daniel, I have used this feed URL https://www.nasa.gov/feeds/podcasts/explorers-apollo and the full content imported without any issue but the log shows an error Full content URL response received. Could you please let me know why it appears? ![]() ![]() |
This was marked wrong as an |
Important
In ALPHA testing. Not final.
Summary
global $themeisle_log_event;
to gather error messages per import uiWill affect visual aspect of the product
NO
Screenshots
Test instructions
Important
Test with and without PRO version: https://github.com/Codeinwp/feedzy-rss-feeds-pro/pull/927
Check before Pull Request is ready:
Closes https://github.com/Codeinwp/feedzy-rss-feeds-pro/issues/845