Easy-digital-downloads: Add a debugging class

[cklosowski]

The EDD error handling needs to be improved and following the footsteps of AffiliateWP we can introduce a logging class (https://github.com/AffiliateWP/AffiliateWP/blob/master/includes/class-logging.php) that will allow us to easily log errors and exceptions. This will be great for core and extensions as it gives us a place to store those exceptions and off cases so we can better offer support later.

Off the top I think it should have a things (up for discussion):

• Allow a 'context' reference (Core, Software Licensing, Mailchimp), which would ake it easier to see where the log originated from
• Auto timestamp the errors coming in
• Add a method for popping the last log off the top
• Allow some sort and count parameters when getting logs

[michaelbeil]

Do you think we should add a debug assistant tool a la AffiliateWP as well?

[cklosowski]

The goal is to do all of that yes.

Chris Klosowski
[email protected]
@cklosowski

On Thu, Aug 11, 2016 at 10:17 AM, Michael Beil [email protected]
wrote:

Do you think we should add a debug assistant tool a la AffiliateWP as
well?
[image: screen shot 2016-08-11 at 12 15 54 pm]
https://cloud.githubusercontent.com/assets/4752949/17597861/61cb0d34-5fbd-11e6-85b8-2d0ba4927df6.png

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
https://github.com/easydigitaldownloads/easy-digital-downloads/issues/4864#issuecomment-239228133,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABU2LPcqs54bTXdsCvxft210SI49LHKSks5qe1kbgaJpZM4JiVs8
.

[michaelbeil]

Right on.

[cklosowski]

Some more thoughts on this that we had in a discussion on Slack with @chriscct7

• Still use the wp_log type, which allows easy use of the wp list tables and searching and bulk actions.
• {expand/delete buttons} |Importance Level | Name of Issuer | Action when issuing | Title (character limited to post_title in the unminimized view) | Date/Time of Event
• Possibly we could do like i’m doing with PPP as well, using the post_content as json https://thomasgriffin.io/wordpress-performance-outside-box-handling-retrieving-data/
• Use the post_author to report the person logged in (or customer) triggering the action
• Also instead of a importance level column, we'd do 1 for importance and 1 for type so you can distinguish an important notice from an urgent error
• we should also let the issuer provide an expiry, default false, so by default a logged event would be kept forever, but extensions could make notices that are cleared from the post table. That way if someone wanted they could build a stream like system for logging edd stuff. After expiry, a daily cron would remove the expired log items, so that we could keep some logs forever, but let informational ones expire that way extensions like FES could log issues with users trying to bypass vendor permission checks, and let those be removed after a while, so that they don't clutter up the logs

[pippinsplugins]

I'd like to see all the debug data stored in a text file. This isn't intended for something that needs historical records kept so it's okay that it's not displayed in a data table format. Text files can be easily searched for patterns so the lack of DB search isn't an issue.

It's also intended that debug mode only be enabled while actually debugging something, so I do not feel it is important that log entries include an expiration date of any kind.

[pippinsplugins]

@cklosowski this is ready for a first look.

I've mimicked the debug tool in AffiliateWP.

Once we're happy with this, I'd like for us to go through and add debug log entries throughout core.

[cklosowski]

Sounds good @pippinsplugins I'll take a look today.

[chriscct7]

IMO at minimum there should be a button to delete the debug log in the tools area if the file exists, and ideally we should find a way to delete this file automatically when it's not being used. The difference between db and file outside of search is also the fact that the log file is publicly exposed.

[danieliser]

@chriscct7 Can't it just be stored in the already secured uploads/edd folder?

[pippinsplugins]

@chriscct7 there is a button to clear the contents of the file.

[chriscct7]

@danieliser We shouldn't assume the edd folder is secure in this case, because by default on Nginx it is not. While that's not an issue for downloadable files from a security perspective, the debug plugin could foreseeable be used by developers to put important non-public information as they var_dump them from API services so things like API keys, server config information, etc. We should take reasonable precautions because of this.

[chriscct7]

@pippinsplugins yep I missed the tab before. I would suggest adding a is_readable check when you're trying to read the file. It's conceivable that the file could be readable but not writeable.

The other thing is the delete I feel should be a bit more comprehensive for more safeguarding against wierd server configs, something like

    /**
     * Removes all contents in the log file
     *
     * @since 2.8.7
     * @return void
     */
    public function clear_log_file() {
        @unlink( $this->file);
        if ( file_exists( $this->file ) ){
             // it's still there, so maybe server doesn't have delete rights
            chmod( $this->file, 0664 ); // Try to give the server delete rights
            @unlink( $this->file );

            // See if it's still there
            if ( @file_exists( $this->file ) ) {
                    // if it's still there, then the server has a wierd config
                    // We will not be able to delete the file, but we can at least do 2 more things
                    if ( is_writeable( $this->file ) {
                         file_put_contents( $this->file, '' ); // if it's writeable, blank it out
                    }

                    // Also we should alert the user
                    return false; // since unlink returns true on success and false on failure, we should detect a false return in the edd settings panel and show a 1 time notice.
             } else {
                         // file removed. Set $this->file to null and return success.
                         $this->file = '';
                         return true;
            }
        } else {
                     // file removed. Set $this->file to null and return success.
                     $this->file = '';
                     return true;
        }
    }

[chriscct7]

Basically if it can delete the file, it will do so and if it can't we will try to give it the permissions to delete the file and try again.

If it still cannot delete the file, we will try to erase the file, and then return false, which we can then detect and show an EDD notice on the settings panel.

This offers the maximum protection with no extra cost to users where the delete works the first try (except for a quick file exists check)

[chriscct7]

Also $this->file should be set to null after unlinking. Will update code above.

[chriscct7]

Updated

[chriscct7]

This change by the way also makes it easy to unit test the delete action. While a unit test on this will always pass for core, we should include it, that way the people that run the unit tests for EDD when deploying EDD to their site (to check for site server config issues) can easily identify if there will be a problem

[chriscct7]

PR for the delete function to pippin's PR opened here: https://github.com/easydigitaldownloads/easy-digital-downloads/pull/6028

[cklosowski]

We could make this a little more "secure" by adding a hash to the file name, and have it be predictable by the code, but not a person.

Right now the file is named edd-debug.log, but if we named it something like:
wp_hash( home_url( '/' ) ) . '-edd-debug.log it would use the salts defined in the wp-config.php file while being predicable in the clear and delete methods.

This way we can put it in a place that _may_ be publicly visible, but less predictable.

[pippinsplugins]

@chriscct7 PR looks good. Merged it in and now going to make a few minor changes to it.

[pippinsplugins]

Ok this is ready for another look: https://github.com/easydigitaldownloads/easy-digital-downloads/pull/6020

[chriscct7]

It'd be nice if the debug mode could be enabled via a constant, ala

$ret = edd_get_option( 'debug_mode', false );
$ret = $ret || ( is_defined( 'EDD_DEBUG_MODE' ) && EDD_DEBUG_MODE );

[chriscct7]

That way in WP-config someone can do a

if ( is_defined( 'WP_DEBUG' ) && WP_DEBUG ) {
   define( 'EDD_DEBUG_MODE', true );
}

Plus it'd make it easy to enable on unit tests if desired

[chriscct7]

Also made a suggestion re:sunny's comment on the PR about the global for logging

[sunnyratilal]

Agree with @chriscct7 on the EDD_DEBUG_MODE constant

[pippinsplugins]

Added.

[sunnyratilal]

@pippinsplugins This looks great, I've been testing it with MailChimp and it is working well.

Tested the EDD_DEBUG_MODE constant too and working great. 👍

[sunnyratilal]

Further testing complete - works like a charm.