Skip to content

Revert added exception in the sandbox#3893

Merged
LeoColomb merged 2 commits intomasterfrom
fix/plugin-deact
Apr 6, 2025
Merged

Revert added exception in the sandbox#3893
LeoColomb merged 2 commits intomasterfrom
fix/plugin-deact

Conversation

@LeoColomb
Copy link
Copy Markdown
Member

@LeoColomb LeoColomb commented Apr 6, 2025

Fixes #3892
Ref #3871

@LeoColomb LeoColomb changed the title Revert added expection in the sandbox Revert added exception in the sandbox Apr 6, 2025
@LeoColomb LeoColomb enabled auto-merge (squash) April 6, 2025 20:00
@LeoColomb LeoColomb requested a review from a team April 6, 2025 20:02
Copy link
Copy Markdown
Member

@dgw dgw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fine on its face; #3871 changed yourls_include_file_sandbox()'s behavior when it probably didn't need to.

Deeper musings unrelated to making a hotfix

Hmm, from this comment in the function doc:

 * This function does not check first if the file exists : depending on use case, you may check first.

I would think that upstream code is expected to test for the file's existence before trying to include_file_sandbox it, here in between setting $uninst_file and $attempt:

// Check if we have an uninstall file - load if so
$uninst_file = YOURLS_PLUGINDIR . '/' . dirname($plugin) . '/uninstall.php';
$attempt = yourls_include_file_sandbox( $uninst_file );
// Check if we have an error to display
if ( is_string( $attempt ) ) {
$message = yourls_s( 'Loading %s generated unexpected output. Error was: <br/><pre>%s</pre>', $uninst_file, $attempt );
return( $message );
}
if ( $attempt === true ) {
define('YOURLS_UNINSTALL_PLUGIN', true);
}

Something like this, maybe (GH doesn't support creating suggestions outside the patched area of a PR, sadly)?

    // Check if we have an uninstall file - load if so 
    $uninst_file = YOURLS_PLUGINDIR . '/' . dirname($plugin) . '/uninstall.php';
    $attempt = yourls_include_file_sandbox( $uninst_file );

    // Try to set plugin uninstall mode if valid
    if ( file_exists( $uninst_file ) {
        if ( $attempt === true ) {
            define('YOURLS_UNINSTALL_PLUGIN', true);
        }
    }

    // Check if we have an error to display
    elseif ( is_string( $attempt ) ) {
        $message = yourls_s( 'Loading %s generated unexpected output. Error was: <br/><pre>%s</pre>', $uninst_file, $attempt );
        return( $message );
    }

@LeoColomb LeoColomb merged commit ef18d1d into master Apr 6, 2025
9 checks passed
@LeoColomb LeoColomb deleted the fix/plugin-deact branch April 6, 2025 20:46
@dgw dgw added this to the 1.10.1 milestone Apr 6, 2025
tomtenuta pushed a commit to tomtenuta/YOURLS that referenced this pull request Nov 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unable to deactivate [default] plugins

2 participants