Skip to content

Fix for is_plugin_active() with slug-hash folder name #151

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.

Sign up for GitHub

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

Closed
wants to merge 21 commits into from
+103 −2
Closed

Fix for is_plugin_active() with slug-hash folder name #151

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d3b51e5
Add methods for slugs with DID
afragen Jul 4, 2025
fa47c07
Wants to be escaped for CI
afragen Jul 4, 2025
a794e80
just for CI testing
afragen Jul 4, 2025
a393954
update for renamed function
afragen Jul 4, 2025
d2fce51
Merge branch 'main' into did-plugins
afragen Jul 5, 2025
147f576
Update inc/plugins/namespace.php
afragen Jul 6, 2025
5795d00
refactor a bit
afragen Jul 6, 2025
829ae96
assume slug-hash and drop hash to return slug
afragen Jul 8, 2025
ed04e7d
rename as likely to be DID hash
afragen Jul 8, 2025
2f0f59d
no longer needed
afragen Jul 8, 2025
108c5a6
refactor and simplify
afragen Jul 8, 2025
f66e4f7
Merge branch 'main' into did-plugins
afragen Jul 8, 2025
c5d57a5
keep prior function name
afragen Jul 8, 2025
8286b67
rename function as more file than slug
afragen Jul 8, 2025
90c79e2
pass DID as key
afragen Jul 9, 2025
ba17062
check that did-hash is actually what is being removed
afragen Jul 9, 2025
267505a
Colin's string functions instead of my array functions
afragen Jul 9, 2025
153fdf6
load during load-plugins.php
afragen Jul 9, 2025
dcc9df0
D'oh
afragen Jul 9, 2025
01aa00b
Need moor cooofffeee
afragen Jul 9, 2025
05bbaf8
Just for Colin
afragen Jul 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions inc/namespace.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ function bootstrap() {
Icons\bootstrap();
Importers\bootstrap();
Pings\bootstrap();
Plugins\bootstrap();
Salts\bootstrap();
Settings\bootstrap();
Updater\bootstrap();
Expand Down
108 changes: 108 additions & 0 deletions inc/plugins/namespace.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
<?php
/**
* Sets DID-less plugin slug as active.
*
* @package FAIR
*/

namespace FAIR\Plugins;

use function FAIR\Updater\get_packages;

/**
* Bootstrap
*
* @return void
*/
function bootstrap() {
add_filter( 'option_active_plugins', __NAMESPACE__ . '\\set_as_active' );
add_filter( 'wp_admin_notice_markup', __NAMESPACE__ . '\\hide_notice', 10, 3 );

// just for testing.
if ( ! function_exists( 'is_plugin_active' ) ) {
require_once ABSPATH . 'wp-admin/includes/plugin.php';
}
if ( is_plugin_active( 'git-updater/git-updater.php' ) ) {
wp_admin_notice( 'Git Updater is active' );
}
Comment on lines +21 to +27
Copy link
Contributor Author

@afragen afragen Jul 6, 2025

Choose a reason for hiding this comment

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

We will need to remove this when ready to merge.

}

/**
* Set FAIR plugins as active using DID-less slug.
*
* @param array $active_plugins Array of active plugins.
*
* @return array
*/
function set_as_active( $active_plugins ) {
remove_filter( 'option_active_plugins', __NAMESPACE__ . '\\set_as_active' );
$packages = get_packages();
$plugins = $packages['plugins'] ?? [];
foreach ( $plugins as $plugin ) {
if ( is_plugin_active( plugin_basename( $plugin ) ) ) {
$plugins[] = get_didless_slug( $plugin );
Copy link
Member

@costdev costdev Jul 6, 2025

Choose a reason for hiding this comment

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

Careful - $plugin isn't a slug in this situation, it's trailingslashit( WP_PLUGIN_DIR ) . $file - Ref.

Copy link
Member

@costdev costdev Jul 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I meant the call to get_didless_slug( $plugin ), which expects the argument to be a slug, not a path. Maybe it's get_didless_slug()'s internally referring to $slug that's causing confusion for me.

Would it be clearer to rename get_didless_slug() to remove_did_id() and call the parameter $plugin? That way, $plugin could be a slug, an absolute path, or slug/file.php.

Copy link
Contributor Author

@afragen afragen Jul 6, 2025

Choose a reason for hiding this comment

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

Refactored in 5795d00

}
}
$active_plugins = array_map( 'plugin_basename', array_merge( $active_plugins, $plugins ) );

return array_unique( $active_plugins );
}

/**
* Return shortened DID withou `did:plc|web'.
*
* @param string $did Full DID.
*
* @return string|void
*/
function get_short_did( $did ) {
Copy link
Member

@costdev costdev Jul 6, 2025

Choose a reason for hiding this comment

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

The "short DID" as referred to here is known as the "method-specific-id" in the spec. I think we should try to stick to the terminology where possible. Besides "short" is vague 😉. What about get_did_id()?

Copy link
Member

@costdev costdev Jul 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Aside from that, I think there's a great opportunity here for a more general function, parse_did() that returns an array keyed on 'method' and 'id', or false for an improper DID.

So instead of having get_did_id(), you'd instead have:

function parse_did( $did ) {
  $parts = explode( ':', $did, 3 );

  if (
    count( $parts ) !== 3 ||
    $parts[0] !== 'did' ||
    ( $parts[1] !== 'web' && $parts[1] !== 'plc' )
  ) {
    return false;
  }

  return [
    'method' => $parts[1],
    'id' => $parts[2],
  ];
}

Usage:

$did = parse_did( $did );

if ( ! $did ) {
  // Error or something else.
}

$id = $did['id'];

Actually, there's already a parse_did() being introduced in #71 - here's its implementation.

Seems like we'll end up using that function when #71 gets merged.

Copy link
Contributor Author

@afragen afragen Jul 6, 2025

Choose a reason for hiding this comment

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

Refactored in 5795d00

if ( ! empty( $did ) ) {
if ( str_contains( $did, ':' ) ) {
Copy link
Member

@costdev costdev Jul 6, 2025

Choose a reason for hiding this comment

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

I'll be looking at what our minimum WP version should be bumped to, and opening a PR as soon as I can, so this doesn't need to be changed to strpos().

Still, with a current minimum PHP of 7.4 and WP of 5.4, bad Andy for using this function. hits with ruler

Copy link
Contributor Author

@afragen afragen Jul 6, 2025

Choose a reason for hiding this comment

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

str_contains() is in a shim in WP5.9

Copy link
Member

@costdev costdev Jul 6, 2025

Choose a reason for hiding this comment

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

Yep, and our current minimum is 5.4. We also use wp_admin_notice(), which is WP 6.4, so I know we'll need to at least bump our minimum to 6.4. I'll work out if there are any other uses that need us to bump above 6.4.

Copy link
Contributor Author

@afragen afragen Jul 6, 2025

Choose a reason for hiding this comment

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

Besides Ryan uses str_starts_with() in parse_did() 😆

Copy link
Contributor

@Ipstenu Ipstenu Jul 6, 2025

Choose a reason for hiding this comment

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

Thank you @costdev !

$did = (string) explode( ':', $did )[2];
}
return $did;
}
}

/**
* Return slug without DID.
*
* @param string $slug Current slug.
* @param string $did Full DID.
*
* @return string|void
*/
function get_didless_slug( $slug, $did = '' ) {
Copy link
Contributor

@Ipstenu Ipstenu Jul 6, 2025

Choose a reason for hiding this comment

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

Hm. If we do get_did_id() then this might be better as get_slug_without_did_id()?

Copy link
Contributor Author

@afragen afragen Jul 6, 2025

Choose a reason for hiding this comment

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

Good idea, I sort of did that in 5795d00

if ( empty( $did ) ) {
$did = get_file_data( $slug, [ 'PluginID' => 'Plugin ID' ] )['PluginID'];
$did = get_short_did( $did );
}
if ( ! empty( $did ) ) {
$slug = str_replace( '-' . get_short_did( $did ), '', $slug );
}

return $slug;
}

/**
* Hide notice reporting DID-less plugin is inactive because it doesn't exist.
*
* @param string $markup Markup of notice.
* @param string $message Message of notice.
* @param array $args Args of notice.
*
* @return string
*/
function hide_notice( $markup, $message, $args ) {
$active = get_option( 'active_plugins' );
if ( $args['id'] === 'message' ) {
foreach ( $active as $plugin ) {
if ( str_contains( $message, $plugin ) && str_contains( $markup, 'error' ) ) {
Copy link
Member

@costdev costdev Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Check for the 'error' before the loop, in the initial if - can alternatively use in_array( 'error', $args['additional_classes'], true ) instead of checking for 'error' somewhere in the markup.

Copy link
Contributor Author

@afragen afragen Jul 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But then I need to also check that the additional_classes is set correctly.

Copy link
Member

@costdev costdev Jul 9, 2025

Choose a reason for hiding this comment

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

Can just use:

in_array( 'error', $args['additional_classes'] ?? [], true )

Copy link
Member

@costdev costdev Jul 9, 2025

Choose a reason for hiding this comment

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

I'm noticing that there's another error that has an ID of message and has the error class.

I think, unfortunately, we'll have to check for __( 'Plugin file does not exist.' ) and add a phpcs:ignore comment for the lack of textdomain since this is a Core string.

remove_filter( 'wp_admin_notice_markup', __NAMESPACE__ . '\\hide_notice' );
return '';
}
}
}

return $markup;
}
1 change: 1 addition & 0 deletions plugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
require_once __DIR__ . '/inc/icons/namespace.php';
require_once __DIR__ . '/inc/importers/namespace.php';
require_once __DIR__ . '/inc/pings/namespace.php';
require_once __DIR__ . '/inc/plugins/namespace.php';
require_once __DIR__ . '/inc/salts/namespace.php';
require_once __DIR__ . '/inc/settings/namespace.php';
require_once __DIR__ . '/inc/updater/namespace.php';
Expand Down