[debug] [logging] Automatically turn off `fs_debug_mode`after 24 hours #691

DanieleAlessandra · 2024-03-11T17:14:53Z

When the user activates debugging a single event cron starts:
wp_schedule_single_event(time() + 24 * HOUR_IN_SECONDS, 'fs_debug_turn_off_logging_hook');

When the user deactivates debuggong this cron is unscheduled:
wp_unschedule_event($timestamp, 'fs_debug_turn_off_logging_hook');

If the event happens, the option is set to off:
add_action('fs_debug_turn_off_logging_hook', array( self::class, '_turn_off_debug_mode' ) );

Also, in the debug page I added a countdown timer, so the user is informed about the fact that debugging will be disabled automatically.

swashata

@DanieleAlessandra greately done 👏 ... I think your logic is sound. Please see my suggestion for a little refactoring to make the main class a little bit slim and also code convention fixes.

includes/class-freemius.php

templates/debug.php

swashata

Hey @DanieleAlessandra nicely done 👏 ... Please see my few comments. Also kindly do a self review and fix the indentation glitches. We are getting close to merging this.

includes/class-freemius.php

includes/class-fs-storage.php

includes/managers/class-fs-debug-manager.php

templates/debug.php

swashata

Greatly done @DanieleAlessandra 👍 Please see my two comments and we can merge this.

Also don't forget to rebase on develop and bump pre-release version of start.php

swashata · 2024-04-08T14:51:26Z

includes/class-fs-storage.php

+        /**
+         * @var bool|int
+         */
+        public $install_timestamp;


@DanieleAlessandra would it also work if you just add a @property bool|int $install_timestamp in the PHP doc block, instead of this change?

The reason I am asking is, let's not make changes to the existing system, unless needed.

Maybe it will work, I try...

swashata · 2024-04-08T14:51:40Z

includes/managers/class-fs-debug-manager.php

swashata · 2024-04-08T14:52:30Z

require.php

Revert indentation changes please.

swashata · 2024-04-09T06:54:37Z

includes/class-freemius.php

@@ -20815,7 +20448,7 @@ private function _fetch_newer_version( $plugin_id = false, $flush = true, $expir
         *
         * @return bool|FS_Plugin_Tag
         */
-        function get_update( $plugin_id = false, $flush = true, $expiration = FS_Plugin_Updater::UPDATES_CHECK_CACHE_EXPIRATION, $newer_than = false ) {
+        function get_update( $plugin_id = false, $flush = true, $expiration = WP_FS__TIME_24_HOURS_IN_SEC, $newer_than = false ) {


@DanieleAlessandra rebase glitch.. Please use $expiration = FS_Plugin_Updater::UPDATES_CHECK_CACHE_EXPIRATION

swashata · 2024-04-09T06:55:00Z

includes/class-freemius.php

                 */
-                $was_license_expired_before_sync = $this->_license->is_expired();


@DanieleAlessandra rebase glitch, please revert

swashata · 2024-04-09T06:55:11Z

includes/class-freemius.php

@@ -22484,7 +22115,7 @@ private function check_updates(
            $background = false,
            $plugin_id = false,
            $flush = true,
-            $expiration = FS_Plugin_Updater::UPDATES_CHECK_CACHE_EXPIRATION,
+            $expiration = WP_FS__TIME_24_HOURS_IN_SEC,


@DanieleAlessandra rebase glitch

swashata

Hey @DanieleAlessandra, nicely done. But it looks like I (accidentally) stumbled upon a race-condition or bad-state like issue in the class-fs-logger that can bite us.

Please see this

To reproduce, I have

Changed wp_schedule_single_event( time() + 24 * HOUR_IN_SECONDS, 'fs_debug_turn_off_logging_hook' ); to wp_schedule_single_event( time() + 5, 'fs_debug_turn_off_logging_hook' );
Turned on WP_DEBUG
Enabled the Freemius debugging.
Waited 5 sec and refreshed
The error pops up sometimes and sometimes it doesn't.

I think the issue is in class-fs-logger and the static variable $_isStorageLoggingOn which is not cleared/updated after calling _set_storage_logging. Kindly take a look. Thanks.

…, moved debug logic from Freemius to new FS_DebugManager class, fix a bug about logging attempt to DB when table does not exist.

swashata · 2024-04-21T10:42:59Z

includes/class-fs-logger.php

+        /**
+         * @var bool
+         */
+        public static  $ON          = false;


@DanieleAlessandra I think we are mixing two things here :)

We have the function _set_storage_logging which only deals with DB related storage. So making it off shouldn't really turn off regular logging.

For calling $wpdb->insert when we have just turned off the logging

We need to update the already existing $_isStorageLoggingOn static variable inside the _set_storage_logging. Something like

public static function _set_storage_logging( $is_on = true ) { // .. existing code self::$_isStorageLoggingOn = $is_on; }

and that's it. I think this will work just fine.

Out of sync DB creation issue

I see you've made adjustment in the creation statement with CREATE TABLE IF NOT EXISTS. I propose we do it like this:

Drop table if exists.

Then create the table.

I think this way when the logging is turned on we are 100% sure to get the table schema we are after. Since we are dropping table anyway when turning off, I think this approach is fine.

So kindly see if the above suggestion would work.

swashata

Please see the indentation glitch and one conceptual thing @DanieleAlessandra

swashata · 2024-05-07T13:17:44Z

includes/class-fs-logger.php

@@ -125,6 +128,14 @@ function on() {
 			self::hook_footer();
 		}

+        /**


Indentation glitch

swashata · 2024-05-07T13:17:54Z

includes/class-fs-logger.php

@@ -240,6 +251,9 @@ function api_error( $api_result, $wrapper = false ) {
 		}

 		function entrance( $message = '', $wrapper = false ) {
+            if ( ! $this->is_on() ) {


Indentation glitch

swashata · 2024-05-07T13:24:39Z

includes/class-fs-logger.php

@@ -240,6 +251,9 @@ function api_error( $api_result, $wrapper = false ) {
 		}

 		function entrance( $message = '', $wrapper = false ) {
+            if ( ! $this->is_on() ) {


This might not be needed since the _log is already bailing. If you want to be more memory/compute optimistic, then we need to have this check for every public log related function, but that isn't needed IMO.

swashata · 2024-05-07T13:24:58Z

includes/class-fs-logger.php

@@ -320,6 +334,11 @@ public static function _set_storage_logging( $is_on = true ) {

 			$table = "{$wpdb->prefix}fs_logger";

+            /**


Indentation glitch, also please fix for the rest of the file.

swashata · 2024-05-07T13:26:53Z

includes/class-fs-logger.php

@@ -329,7 +348,7 @@ public static function _set_storage_logging( $is_on = true ) {
 				 *
 				 * @link https://core.trac.wordpress.org/ticket/2695
 				 */
-				$result = $wpdb->query( "CREATE TABLE {$table} (
+				$result = $wpdb->query( "CREATE TABLE IF NOT EXISTS {$table} (


Is this IF NOT EXISTS needed now that we are dropping the table anyway?

Table could exist if it was created with a previous version and never deleted

swashata · 2024-05-07T13:39:31Z

includes/class-fs-logger.php

+                /**
+                 * Since logging table does not exist anymore, we need to turn off all instances
+                 */
+                foreach ( self::$LOGGERS as $logger ) {


I also think this isn't needed. Because we have just turned off "DB" storage of the log. The actual log happens with the constant WP_FS__DEBUG_SDK. So if DB storage is turned off, let there still be memory logging within the _log and it will show up in the JavaScript console. The only fix needed is turning off the $_isStorageLoggingOn in line 385 like you did :)

swashata · 2024-05-07T13:40:26Z

includes/class-fs-logger.php

+         *
+         * @return void
+         */
+        public function off() {


This method won't be needed anymore (please see my suggestion below). Also just a code convention thingy, we add a blank line after a function/method declaration.

swashata · 2024-05-07T14:00:37Z

includes/managers/class-fs-debug-manager.php

+                }
+
+                // Turn on/off storage logging.
+                FS_Logger::_set_storage_logging( ( 1 == $is_on ) );


@DanieleAlessandra please see that you are already setting storage login from the _turn_off_debug_mode or the _turn_on_debug_mode above (line 284). So I feel this call is redundant.

* develop: [opt-in] Fixed an issue with the opt-in moderation and API connectivity test logic. [compile] Version 2.7.3 [cleanup] [version] Delete old commented out code and prepare v2.7.3. [deactivation] [ui] [fix] Fix UI glitch in the deactivation feedback form. [version] Version 2.7.2 Hotfix [version] [compile] [i18n] Version 2.7.1 [version] [pre-release] 2.7.0.2 [subscription-cancellation] [button-text] [l10n] Use proper localization function to populate the "Cancel" button of the subscription cancellation modal. [i18n] Update the strings for case sensitivity. Remove lines spacings that can be removed while translating.

swashata requested changes Mar 12, 2024

View reviewed changes

majick777 mentioned this pull request Mar 15, 2024

[Freemius Debug] Bloated wp7f_fs_logger.ibd database identified netmix/radio-station#480

Open

DanieleAlessandra requested a review from swashata March 21, 2024 10:04

swashata requested changes Mar 27, 2024

View reviewed changes

DanieleAlessandra force-pushed the feature/auto-off-debugging branch 8 times, most recently from 2977f06 to f34b38f Compare March 28, 2024 08:53

swashata requested changes Apr 8, 2024

View reviewed changes

DanieleAlessandra force-pushed the feature/auto-off-debugging branch from f34b38f to 2f3ddee Compare April 8, 2024 16:14

DanieleAlessandra requested a review from swashata April 8, 2024 16:19

swashata requested changes Apr 9, 2024

View reviewed changes

[debug] [logging] Automatically turn off fs_debug_mode after 24 hours…

aad603c

…, moved debug logic from Freemius to new FS_DebugManager class, fix a bug about logging attempt to DB when table does not exist.

DanieleAlessandra force-pushed the feature/auto-off-debugging branch from 6b0a528 to aad603c Compare April 9, 2024 15:05

swashata requested changes Apr 21, 2024

View reviewed changes

DanieleAlessandra added 2 commits April 25, 2024 12:49

Removed useless $ON static variable, using $_isStorageLoggingOn

2a8dc07

Removed useless $ON static variable, using $_isStorageLoggingOn

b13db2d

swashata requested changes May 7, 2024

View reviewed changes

swashata reviewed May 7, 2024

View reviewed changes

DanieleAlessandra added 2 commits July 2, 2024 13:50

Removed useless off method and redundant checks

787bd33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[debug] [logging] Automatically turn off `fs_debug_mode`after 24 hours #691

[debug] [logging] Automatically turn off `fs_debug_mode`after 24 hours #691

DanieleAlessandra commented Mar 11, 2024

swashata left a comment

swashata left a comment

swashata left a comment

swashata Apr 8, 2024

DanieleAlessandra Apr 8, 2024

swashata Apr 8, 2024

swashata Apr 8, 2024

swashata Apr 9, 2024

swashata Apr 9, 2024

swashata Apr 9, 2024

swashata left a comment

swashata Apr 21, 2024

swashata left a comment

swashata May 7, 2024

swashata May 7, 2024

swashata May 7, 2024

swashata May 7, 2024

swashata May 7, 2024

DanieleAlessandra Jul 2, 2024

swashata May 7, 2024

swashata May 7, 2024

swashata May 7, 2024

		*/
		$was_license_expired_before_sync = $this->_license->is_expired();

		@@ -320,6 +334,11 @@ public static function _set_storage_logging( $is_on = true ) {

		$table = "{$wpdb->prefix}fs_logger";

		/**

[debug] [logging] Automatically turn off fs_debug_modeafter 24 hours #691

Are you sure you want to change the base?

[debug] [logging] Automatically turn off fs_debug_modeafter 24 hours #691

Conversation

DanieleAlessandra commented Mar 11, 2024

swashata left a comment

Choose a reason for hiding this comment

swashata left a comment

Choose a reason for hiding this comment

swashata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swashata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

For calling $wpdb->insert when we have just turned off the logging

Out of sync DB creation issue

swashata left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[debug] [logging] Automatically turn off `fs_debug_mode`after 24 hours #691

[debug] [logging] Automatically turn off `fs_debug_mode`after 24 hours #691

For calling `$wpdb->insert` when we have just turned off the logging