Fix for is_plugin_active() with slug-hash folder name

[afragen]

This PR will/should make any installed FAIR plugin, that was installed with folder at slug-did, as an active plugin when using is_plugin_active( slug ).

Yes, I'm making a preference for avoiding name collisions by installing as slug-did.

Requires #150

[costdev]

Some changes, some thoughts, and a whack with a ruler. This review has everything!

[costdev]

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()?

[costdev]

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.

[afragen]

Refactored in 5795d00

[costdev]

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

[costdev]

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.

[afragen]

Refactored in 5795d00

[costdev]

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

[afragen]

str_contains() is in a shim in WP5.9

[costdev]

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.

[afragen]

Besides Ryan uses str_starts_with() in parse_did() 😆

[Ipstenu]

Thank you @costdev !

[Ipstenu]

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

[afragen]

Good idea, I sort of did that in 5795d00

[Ipstenu]

Thank you @costdev !

[afragen]

We will need to remove this when ready to merge.

[afragen]

I have a better way of getting the method specific ID from the DID. I plan on extending Ryan's parse_did(). This will change a number of things in this PR, hopefully simplifying it. It also means the PR to extend parse_did() will need to be evaluated first. See #152

[afragen]

Needs #153 to pass tests.

[costdev]

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.

[afragen]

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

[costdev]

Can just use:

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

[costdev]

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.

[afragen]

This will error until #71 is merged. It requires get_did_hash().

[rmccue]

I will say that I don't love the pattern of hook-and-self-unhook here, and it also doesn't seem like it works consistently - what happens when you call is_plugin_active() twice? Seems like the code here breaks with that. Additionally, filtering option_active_plugins on every call seems like it'd have the potential to accidentally get saved into the DB, which is dangerous.

I'd rather we instead a) install consistently into {slug}-{hash}/ meaning that code can rely on that pretty consistently, and b) encourage the use of is_plugin_active( 'did:...' ) instead of the filenames anyway.

[afragen]

@rmccue consider this more of a POC. It does seem that your hijack and backtrace method might be cleaner.