Conversation
WalkthroughA file-based logging system is added: new singleton WPUF_File_Logger, helper functions, cron-driven cleanup scheduling, and logging calls added across PayPal gateway paths; installer/bootstrap updated to schedule and unregister the cleanup event. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 6
🧹 Nitpick comments (1)
wpuf-functions.php (1)
5309-5313: Reuse the existing logger instance for cleanup.Constructing a fresh
WPUF_File_Loggerinside the cron callback re-runs constructor side effects likeadd_action( 'init', ... )and bypasses the singleton introduced elsewhere in this PR. Calling the existing instance keeps cleanup aligned with the rest of the logger usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wpuf-functions.php` around lines 5309 - 5313, In wpuf_cleanup_logs replace constructing a new WPUF_File_Logger with the shared singleton instance so you don't re-run constructor side effects; call the class's singleton accessor (e.g., WPUF_File_Logger::instance() or the project's logger getter) and then invoke clear_expired_logs on that instance rather than new WPUF_File_Logger(), keeping the call to clear_expired_logs() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@includes/Log/class-wpuf-file-logger.php`:
- Around line 180-189: The log() method currently overwrites $this->file and
always calls open(), which assumes $this->file is a basename under
$this->log_directory; update log() (and the related logic used at lines
~281-295) to accept and handle string|resource and absolute/custom paths: if
$log_file is a resource, assign it to an internal handler property (e.g.,
$this->handle) and skip open(); if $log_file is a string, detect whether it is
an absolute path or contains directory separators (not just a basename) and,
when so, use it directly instead of rewriting into get_log_file_name()/
$this->log_directory; otherwise fall back to existing basename behavior and call
open() to create/append under $this->log_directory. Ensure open() and any
file_exists()/fopen() calls only run when $this->file is a resolved path string
and not when a resource is already provided.
- Around line 189-199: The bug is that log() calls format_message() and then
write() caches an already-formatted string (with default level 'info'), so
write_cached_logs() re-feeds formatted text into log() causing double-formatting
and lost severity; change the flow so write() stores raw log data (message text,
level, datetime) instead of a formatted string, or modify write() to accept both
raw and formatted inputs and only call format_message() at the final flush
point; update write_cached_logs() to call format_message() exactly once per
cached entry (using the original level) and ensure log(), write(),
write_cached_logs(), and format_message() consistently handle raw entries (apply
same change for the other similar occurrences noted around the other
write_cached_logs usages).
- Around line 126-127: The log directory is set to
wp_get_upload_dir()['basedir'] which leaves .log files web-accessible; update
the $this->log_directory assignment in class-wpuf-file-logger.php to place logs
outside the public uploads folder (for example use a directory one level above
the uploads/webroot or a non-public path, e.g. WP_CONTENT_DIR . '/../wpuf-logs'
or another non-web-accessible directory) and apply the same change where the
path is computed (also update occurrences around lines 285-295). Ensure the
logger creates the directory with proper permissions and writes a protective
access file (e.g. .htaccess with "Deny from all" or equivalent server-specific
deny) when it creates the folder so files cannot be served directly.
- Around line 15-23: The WPUF_File_Logger class is a global-root singleton and
must be converted to a namespaced, container-registered service: add the
WeDevs\Wpuf\Log namespace to class WPUF_File_Logger (or rename to File_Logger),
remove the global static singleton pattern, and expose the instance via the
plugin container (register it under a service key like file_logger during
bootstrap). Update any consumers to access it through wpuf()->file_logger (magic
getter) instead of directly instantiating or calling
WPUF_File_Logger::get_instance(), and ensure the service registration follows
the plugin's existing container registration pattern so it can be swapped/mocked
in tests.
- Around line 281-298: The permission hardening in open() uses $this->file but
the actual file lives at $this->log_directory . '/' . $this->file; update open()
to compute the real path (e.g., $filepath = $this->log_directory . '/' .
$this->file) and use that $filepath for the existence check, fopen when creating
the log, and the chmod call (keeping the $needs_chmod logic), ensuring chmod
targets the real log file created by fopen; reference the open() method,
$this->file, $this->log_directory, $needs_chmod, and the chmod call when making
the change.
In `@wpuf-functions.php`:
- Around line 5285-5295: In wpuf_log, the WP_DEBUG guard uses defined( WP_DEBUG
) which passes the constant value instead of its name; update the condition to
use defined('WP_DEBUG') and keep the remainder of the guard (the subsequent
WP_DEBUG truthy check) intact so the function correctly bypasses logging when
WP_DEBUG is enabled; locate the wpuf_log function and change the defined call
accordingly.
---
Nitpick comments:
In `@wpuf-functions.php`:
- Around line 5309-5313: In wpuf_cleanup_logs replace constructing a new
WPUF_File_Logger with the shared singleton instance so you don't re-run
constructor side effects; call the class's singleton accessor (e.g.,
WPUF_File_Logger::instance() or the project's logger getter) and then invoke
clear_expired_logs on that instance rather than new WPUF_File_Logger(), keeping
the call to clear_expired_logs() unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cbc2517c-9a0b-44cc-8ac3-a9ff8100085d
📒 Files selected for processing (4)
includes/Installer.phpincludes/Log/class-wpuf-file-logger.phpwpuf-functions.phpwpuf.php
| class WPUF_File_Logger { | ||
|
|
||
| /** | ||
| * The single instance of the class. | ||
| * | ||
| * @var WPUF_File_Logger | ||
| * @since WPUF | ||
| */ | ||
| protected static $_instance = null; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Please wire the logger through the existing service conventions.
This new includes/ service is a global singleton in the root namespace, so it bypasses both the required WeDevs\Wpuf\... namespace and the container access pattern. That makes it inconsistent with the rest of the plugin bootstrap and harder to swap or test later.
As per coding guidelines, {includes,Lib}/**/*.php: Use namespaces starting with WeDevs\Wpuf\ for files under includes/, and includes/**/*.php: Services must be registered in the container and accessed via wpuf()->service_name magic getter pattern.
Also applies to: 140-144
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[warning] 23-23:
Property name "$_instance" should not be prefixed with an underscore to indicate visibility
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/Log/class-wpuf-file-logger.php` around lines 15 - 23, The
WPUF_File_Logger class is a global-root singleton and must be converted to a
namespaced, container-registered service: add the WeDevs\Wpuf\Log namespace to
class WPUF_File_Logger (or rename to File_Logger), remove the global static
singleton pattern, and expose the instance via the plugin container (register it
under a service key like file_logger during bootstrap). Update any consumers to
access it through wpuf()->file_logger (magic getter) instead of directly
instantiating or calling WPUF_File_Logger::get_instance(), and ensure the
service registration follows the plugin's existing container registration
pattern so it can be swapped/mocked in tests.
| $upload_dir = wp_get_upload_dir(); | ||
| $this->log_directory = apply_filters( 'wpuf_log_directory', $upload_dir['basedir'] . '/wpuf-logs' ); |
There was a problem hiding this comment.
Log files are still web-readable from uploads/.
Writing debug data under wp-content/uploads/wpuf-logs/ means a direct request to a known .log filename can expose its contents on common hosts; the blank index.php only blocks directory listing. This is risky for the use cases called out in the PR, especially payment/debug data.
Also applies to: 285-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/Log/class-wpuf-file-logger.php` around lines 126 - 127, The log
directory is set to wp_get_upload_dir()['basedir'] which leaves .log files
web-accessible; update the $this->log_directory assignment in
class-wpuf-file-logger.php to place logs outside the public uploads folder (for
example use a directory one level above the uploads/webroot or a non-public
path, e.g. WP_CONTENT_DIR . '/../wpuf-logs' or another non-web-accessible
directory) and apply the same change where the path is computed (also update
occurrences around lines 285-295). Ensure the logger creates the directory with
proper permissions and writes a protective access file (e.g. .htaccess with
"Deny from all" or equivalent server-specific deny) when it creates the folder
so files cannot be served directly.
| public function log( $message, $level = self::INFO, $log_file = null ) { | ||
| $this->level = $level; | ||
|
|
||
| if ( null !== $log_file ) { | ||
| $this->file = $log_file; | ||
| } else { | ||
| $this->file = $this->get_log_file_name(); | ||
| } | ||
|
|
||
| $this->open(); |
There was a problem hiding this comment.
$log_file does not support the documented resource/custom-path cases.
Line 183 accepts string|resource, but log() always calls open(), and open() assumes $this->file is a basename under $this->log_directory. Passing a valid resource will break on file_exists()/fopen(), and passing an absolute/custom path gets rewritten into the uploads directory instead of the requested target.
Also applies to: 281-295
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/Log/class-wpuf-file-logger.php` around lines 180 - 189, The log()
method currently overwrites $this->file and always calls open(), which assumes
$this->file is a basename under $this->log_directory; update log() (and the
related logic used at lines ~281-295) to accept and handle string|resource and
absolute/custom paths: if $log_file is a resource, assign it to an internal
handler property (e.g., $this->handle) and skip open(); if $log_file is a
string, detect whether it is an absolute path or contains directory separators
(not just a basename) and, when so, use it directly instead of rewriting into
get_log_file_name()/ $this->log_directory; otherwise fall back to existing
basename behavior and call open() to create/append under $this->log_directory.
Ensure open() and any file_exists()/fopen() calls only run when $this->file is a
resolved path string and not when a resource is already provided.
| $this->open(); | ||
|
|
||
| $message = $this->format_message( | ||
| [ | ||
| 'message' => $message, | ||
| 'level' => strtoupper( $level ), | ||
| 'datetime' => new DateTime(), | ||
| ] | ||
| ); | ||
|
|
||
| return $this->write( $message ); |
There was a problem hiding this comment.
Early cached entries will be replayed with the wrong level and a second prefix.
log() formats first, write() caches that formatted string with its default $level = info, and write_cached_logs() feeds it back through log() again. If the file is not ready yet, the eventual line becomes double-formatted and non-info severities are lost.
Also applies to: 257-266, 343-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@includes/Log/class-wpuf-file-logger.php` around lines 189 - 199, The bug is
that log() calls format_message() and then write() caches an already-formatted
string (with default level 'info'), so write_cached_logs() re-feeds formatted
text into log() causing double-formatting and lost severity; change the flow so
write() stores raw log data (message text, level, datetime) instead of a
formatted string, or modify write() to accept both raw and formatted inputs and
only call format_message() at the final flush point; update write_cached_logs()
to call format_message() exactly once per cached entry (using the original
level) and ensure log(), write(), write_cached_logs(), and format_message()
consistently handle raw entries (apply same change for the other similar
occurrences noted around the other write_cached_logs usages).
Add WPUF_File_Logger class that writes structured logs to wp-content/uploads/wpuf-logs/ with PSR-3-style log levels, automatic log rotation (60 days), and cached writes for early-lifecycle calls. - Add wpuf_log() helper with wpuf_log_enabled filter - Add daily cron for log cleanup with runtime scheduling - Add .htaccess protection for the log directory - Add logging to PayPal webhook verification and subscription payment failure paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sapayth
force-pushed
the
feat/log-file-for-wpuf
branch
from
fda89db to
8820c5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
includes/Log/class-wpuf-file-logger.php (4)
15-23: 🛠️ Refactor suggestion | 🟠 MajorPlease register this as a namespaced container service.
The new logger is still a global singleton, so it bypasses the plugin’s
includes/service conventions and makes consumers depend on static state instead ofwpuf()->....As per coding guidelines,
{includes,Lib}/**/*.php: Use namespaces starting withWeDevs\Wpuf\for files underincludes/, andincludes/**/*.php: Services must be registered in the container and accessed viawpuf()->service_namemagic getter pattern.Also applies to: 145-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Log/class-wpuf-file-logger.php` around lines 15 - 23, The WPUF_File_Logger is implemented as a global singleton (protected static $_instance) and lacks the required WeDevs\Wpuf\ namespace and container registration; refactor the class (WPUF_File_Logger) into the includes namespace (WeDevs\Wpuf\...) and remove the static singleton pattern, then register an instance in the plugin service container (so it can be accessed via wpuf()->file_logger or similar) and update consumers to use wpuf()->file_logger instead of WPUF_File_Logger::get_instance(); ensure the service is added to your container bootstrap/registration code following the plugin’s service registration conventions.
131-132:⚠️ Potential issue | 🟠 Major
uploads/wpuf-logsis still a public location.Placing payment/debug logs under
wp-content/uploadsleaves them directly fetchable on many common deployments. The.htaccessandindex.phpfiles added later are not a reliable protection layer once uploads are served by nginx, object storage, or a CDN.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Log/class-wpuf-file-logger.php` around lines 131 - 132, Default log directory should not be inside the public uploads folder; change the default in class-wpuf-file-logger.php so $this->log_directory uses a non-public path (e.g. WP_CONTENT_DIR . '/wpuf-logs') instead of wp_get_upload_dir()['basedir'] . '/wpuf-logs', keep the apply_filters('wpuf_log_directory', ...) but document that the filter expects a path outside webroot, and ensure the code that creates the directory (mkdir/wp_mkdir_p) still runs and sets proper permissions; update any references to the log_directory property or constructor to use the new default.
204-212:⚠️ Potential issue | 🟠 MajorCached entries will be replayed with a second prefix.
log()formats first,write()caches that formatted string when the file handle is not ready, andwrite_cached_logs()sends it back throughlog()again. Early entries will be double-formatted on flush, so the final line no longer matches the original payload.Also applies to: 275-279, 359-364
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Log/class-wpuf-file-logger.php` around lines 204 - 212, log() currently formats messages via format_message() before calling write(), and write() caches those formatted strings when the file handle isn't ready; write_cached_logs() then feeds those cached (already-formatted) strings back into log(), causing double-formatting. Fix by caching the raw payload instead of the formatted string: change write() to accept and store the original payload array (or add a flag like $is_formatted) and make write_cached_logs() call write() with the raw payload (or with $is_formatted=true when replaying), and update write() to only call format_message() when the payload is not already formatted; adjust callers of log()/write_cached_logs() accordingly (referencing log(), write(), write_cached_logs(), and format_message()).
191-202:⚠️ Potential issue | 🔴 CriticalThe documented
resourceand custom-path targets still do not work.
$log_fileis assigned straight into$this->file_name, butopen()always concatenates it under$this->log_directory. Passing a resource will break the path handling, and passing an absolute or nested path will silently write somewhere else than the caller requested.Also applies to: 295-317
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@includes/Log/class-wpuf-file-logger.php` around lines 191 - 202, The code assigns $log_file directly to $this->file_name but open() always prepends $this->log_directory, which breaks resource handles and absolute/nested paths; update the File Logger (class-wpuf-file-logger.php) so setFileName logic and open() detect and respect resources and full paths: if $log_file is a resource (is_resource()) keep it and skip directory handling; if $log_file is a string and contains directory separators or is an absolute path (leading '/' or drive-letter pattern) treat it as a full path and do not prepend $this->log_directory; otherwise continue to build the path using get_log_file_name()/log_directory; apply the same fix to the other affected block (lines ~295-317) to ensure custom paths and resource targets are honored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Lib/Gateway/Paypal.php`:
- Around line 558-561: The current loop in process_webhook() that calls wpuf_log
when any required header is missing will log arbitrary probes; instead, gate
logging by first checking for plausible PayPal indicators (e.g., presence of
PayPal-specific headers like 'PAYPAL-TRANSMISSION-ID' or a PayPal user-agent) or
by attempting a lightweight signature/verification check before emitting an
error; update the required_headers loop in process_webhook() to skip logging for
requests that lack all PayPal-identifying headers (or use a
rate-limited/log-once mechanism) and only call wpuf_log when a PayPal identifier
is present and verification (e.g., verify_webhook_signature() or equivalent) has
failed, leaving anonymous template_redirect handling unchanged.
---
Duplicate comments:
In `@includes/Log/class-wpuf-file-logger.php`:
- Around line 15-23: The WPUF_File_Logger is implemented as a global singleton
(protected static $_instance) and lacks the required WeDevs\Wpuf\ namespace and
container registration; refactor the class (WPUF_File_Logger) into the includes
namespace (WeDevs\Wpuf\...) and remove the static singleton pattern, then
register an instance in the plugin service container (so it can be accessed via
wpuf()->file_logger or similar) and update consumers to use wpuf()->file_logger
instead of WPUF_File_Logger::get_instance(); ensure the service is added to your
container bootstrap/registration code following the plugin’s service
registration conventions.
- Around line 131-132: Default log directory should not be inside the public
uploads folder; change the default in class-wpuf-file-logger.php so
$this->log_directory uses a non-public path (e.g. WP_CONTENT_DIR . '/wpuf-logs')
instead of wp_get_upload_dir()['basedir'] . '/wpuf-logs', keep the
apply_filters('wpuf_log_directory', ...) but document that the filter expects a
path outside webroot, and ensure the code that creates the directory
(mkdir/wp_mkdir_p) still runs and sets proper permissions; update any references
to the log_directory property or constructor to use the new default.
- Around line 204-212: log() currently formats messages via format_message()
before calling write(), and write() caches those formatted strings when the file
handle isn't ready; write_cached_logs() then feeds those cached
(already-formatted) strings back into log(), causing double-formatting. Fix by
caching the raw payload instead of the formatted string: change write() to
accept and store the original payload array (or add a flag like $is_formatted)
and make write_cached_logs() call write() with the raw payload (or with
$is_formatted=true when replaying), and update write() to only call
format_message() when the payload is not already formatted; adjust callers of
log()/write_cached_logs() accordingly (referencing log(), write(),
write_cached_logs(), and format_message()).
- Around line 191-202: The code assigns $log_file directly to $this->file_name
but open() always prepends $this->log_directory, which breaks resource handles
and absolute/nested paths; update the File Logger (class-wpuf-file-logger.php)
so setFileName logic and open() detect and respect resources and full paths: if
$log_file is a resource (is_resource()) keep it and skip directory handling; if
$log_file is a string and contains directory separators or is an absolute path
(leading '/' or drive-letter pattern) treat it as a full path and do not prepend
$this->log_directory; otherwise continue to build the path using
get_log_file_name()/log_directory; apply the same fix to the other affected
block (lines ~295-317) to ensure custom paths and resource targets are honored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92030587-4f70-4b45-a820-bdc5979a7655
📒 Files selected for processing (5)
Lib/Gateway/Paypal.phpincludes/Installer.phpincludes/Log/class-wpuf-file-logger.phpwpuf-functions.phpwpuf.php
🚧 Files skipped from review as they are similar to previous changes (2)
- wpuf.php
- wpuf-functions.php
| foreach ( $required_headers as $header ) { | ||
| if ( empty( $headers[ $header ] ) ) { | ||
| wpuf_log( 'PayPal webhook verification failed: missing required header ' . $header, 'error' ); | ||
| return false; |
There was a problem hiding this comment.
Avoid logging every malformed probe to the public webhook endpoint.
This endpoint is publicly reachable, and the anonymous template_redirect handler at Lines 2531-2540 calls process_webhook() for any action=webhook_triggered request. With this new wpuf_log() call, routine scans and arbitrary requests missing PayPal headers will append an error line on every hit, which makes disk growth attacker-controlled. Gate these requests before logging, or only log once you know the request is plausibly from PayPal.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Lib/Gateway/Paypal.php` around lines 558 - 561, The current loop in
process_webhook() that calls wpuf_log when any required header is missing will
log arbitrary probes; instead, gate logging by first checking for plausible
PayPal indicators (e.g., presence of PayPal-specific headers like
'PAYPAL-TRANSMISSION-ID' or a PayPal user-agent) or by attempting a lightweight
signature/verification check before emitting an error; update the
required_headers loop in process_webhook() to skip logging for requests that
lack all PayPal-identifying headers (or use a rate-limited/log-once mechanism)
and only call wpuf_log when a PayPal identifier is present and verification
(e.g., verify_webhook_signature() or equivalent) has failed, leaving anonymous
template_redirect handling unchanged.
2 participants
A new function is introduced where we can log our messages in a specific file. It will help us debug cron jobs, form settings, payment info etc. Calling a simple function
wpuf_log( $message )will do the trick. It will store a log with the current date time in thewp-content/uploads/wpuf-logs/directory. A new log file will be created for each day and all the log files older than 60 days will be removed.How to use
Simply calling
wpuf_log( $message )will store the log message with datetime. This function also have 2 others optional parameter to pass. So the three parameters are:$message [string]
The message to log
$level [string]
The log level. Default
info$log_file [string|resource]
Optional parameter to pass the log file. The log will be written in this file instead of the default log file.
A basic use:
Will store a log message like:
Hooks Introduced
wpuf_log_directory[filter]: The default log directory iswp-content/uploads/wpuf-logs/. We can change it using this filter.wpuf_log_file_name[filter]: We can change the log file name entirely using this filter.wpuf_formatted_log_message[filter]: The default log format is[%datetime%] %level%: %message%\n. With this filter we will get the formatted log message.wpuf_logger_expiration_days[filter]: Log file will be deleted after 60 days. We can change it using this filter.Summary by CodeRabbit
New Features
Chores
Bug Fixes