Add the ability to deprecate a plugin namespace

The extra namespaces aren't useful here, we'd need to add support for multiple namespace suffices/subdirs which if the only use is deprecation, we could maybe get from deprecatedNamespaces directly if we add that.

> Because plugin discovery is recursive

Is this a useful feature? Even if it is, we could optionally disable it for certain plugin managers - there are unlikely to be many image toolkit plugins.

I'm not sure I've ever seen it actually in use. Let's see what happens if we just drop it entirely.

All functional js tests and a lot of functional tests pass so we are not relying on this much, but we're relying on it a bit - e.g. core/modules/system/src/Plugin/ImageToolkit/Operation/gd/ puts all all the toolkit operations in a 'gd' subdirectory, for no reason as far as I can tell, it was probably namespaced in case we added another toolkit to core or something.

However if we allow individual plugin managers to switch off recursion, how would they notify modules that they've done this?

Opened 📌 Deprecate recursive plugin discovery Active .

I've figured this out -- see https://git.drupalcode.org/project/entity_share/-/merge_requests/136/diffs

Basically, to deprecate a plugin subdir/namespace, the plugin manager needs to do:

  use LegacySubdirPluginManagerTrait;

// in __construct():

    $this->legacySubdir = 'Plugin/ClientAuthorization';
    $this->deprecationMessage = "The plugin '%s' is in a deprecated subdirectory {$this->legacySubdir}. It should be moved to {$this->subdir}.";

The rest of the work is done in LegacySubdirPluginManagerTrait and a discovery decorator.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Both of those will need adding to the PHPStan ignore thingy, as neither of them can be fixed -- AFAICT __call() implementations don't have a return type, and the deprecation message needs to be a variable.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

__call return should be mixed:

https://www.php.net/manual/en/language.oop5.overloading.php#object.call

As long as you match the spec it should be fine.

As for the phpcs error I haven't looked closely, but as long as you pass in parts you should be able to get the right format.

> As for the phpcs error I haven't looked closely, but as long as you pass in parts you should be able to get the right format.

I considered passing in parameters for $project_name, $removed_version, and $cr_url, but it seemed a lot more fiddly for both the code and the caller! Do you think it would be better to do that, just to satisfy PHPCS?

Yes, it's a pattern some other deprecations take I think like the moved classes issue.

It looks like it's still going to need some phpcs ignores for the specifics, even if it's happy with the format -- result on my local:

 29 | ERROR   | [ ] Doc comment for parameter $deprecationMessage does not match actual variable name $changeRecordUrl
    |         |     (Drupal.Commenting.FunctionComment.ParamNameNoMatch)
 52 | WARNING | [ ] The deprecation-version '{$this->deprecationVersion}' does not match the lower-case machine-name standard:
    |         |     drupal:n.n.n or project:n.x-n.n or project:n.x-n.n-label[n] or project:n.n.n or project:n.n.n-label[n]
    |         |     (Drupal.Semantics.FunctionTriggerError.TriggerErrorVersion)
 52 | WARNING | [ ] The removal-version '{$this->removedVersion}' does not match the lower-case machine-name standard: drupal:n.n.n or
    |         |     project:n.x-n.n or project:n.x-n.n-label[n] or project:n.n.n or project:n.n.n-label[n]
    |         |     (Drupal.Semantics.FunctionTriggerError.TriggerErrorVersion)
 52 | WARNING | [ ] The url '{$this->changeRecordUrl}.' does not match the standard: http(s)://www.drupal.org/node/n or
    |         |     http(s)://www.drupal.org/project/aaa/issues/n (Drupal.Semantics.FunctionTriggerError.TriggerErrorSeeUrlFormat)<
/code>

for this code:

<code>
      @trigger_error("Plugin discovery of plugin {$plugin_id} {$this->deprecatedDiscoveryDetails} is deprecated in {$this->deprecationVersion} and is removed in {$this->removedVersion}. {$this->updateDetails} See {$this->changeRecordUrl}.", E_USER_DEPRECATED);

I think it's expecting deprecationVersion to be deprecation_version. I'm not sure about that.

Yeah, some of those are because my code is WIP.... but things like this

> 52 | WARNING | [ ] The url '{$this->changeRecordUrl}.' does not match the standard: http(s)://www.drupal.org/node/n or

are because it's using variables to build the message.

I thought I'd got around this in the moved classes classloader, but it looks like the way I did that was a phpcs:ignore too: https://git.drupalcode.org/project/drupal/-/commit/9244acff98497d4b7961b...

Wait, then why am I not hitting that issue on where I'm trying to convert hook deprecations to an attribute: https://git.drupalcode.org/project/drupal/-/merge_requests/13125/diffs#5...

That issue has other problems with the timing, but no cs errors about the formatting.

📌 Deprecate ModuleHandlerInterface::*Deprecated() in favor of hook metadata Needs work

I think that could be because you're building the message in a variable, then putting that entire variable in the trigger_error(), and maybe the phpcs rule just doesn't pick that up at all? If you change the message does it notice?

Sorry for the side track, but I tested that here: https://git.drupalcode.org/project/drupal/-/merge_requests/13160/pipelines we can check there!

> I think that could be because you're building the message in a variable, then putting that entire variable in the trigger_error(), and maybe the phpcs rule just doesn't pick that up at all?

No, because when I had the whole thing as a variable in an earlier version of my MR -- trigger_error($message) -- it was complaining with the Drupal.Semantics.FunctionTriggerError.TriggerErrorTextLayoutRelaxed sniff for the whole message.

I'll add ignores for the specifics -- this way we still get phpcs coverage for the overall format of the message.