Skip to content
Snippets Groups Projects

Closes #3458177

Closed mondrake requested to merge issue/drupal-3458177:3458177-changing-plugins-from into 11.x
1 unresolved thread

Closes #3458177

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
122 122 protected function parseClass(string $class, \SplFileInfo $fileinfo): array {
123 123 // @todo Consider performance improvements over using reflection.
124 124 // @see https://www.drupal.org/project/drupal/issues/3395260.
125 $reflection_class = new \ReflectionClass($class);
125 try {
126 $reflection_class = new \ReflectionClass($class);
127 }
128 catch (\Throwable $t) {
129 // If reflection failed on the class, it could be due to the class
130 // implementing or extending from interfaces/classes that are not
131 // available to the classloader. This can happen in contrib when a class
132 // in a module extends from a class in another module that is not
133 // installed. In that case, just skip the plugin.
134 return ['id' => NULL, 'content' => NULL];
  • Returning a structure array when we actually want to say 'We've found nothing here' doesn't seem right to me.

  • Author Contributor

    Agree, in principle. However, that's exactly what Drupal\Core\Plugin\Discovery\AttributeDiscoveryWithAnnotations::parseClass() is doing in HEAD right now. Changing that is another issue IMHO.

  • Returning this array doesn't necessary work, because this result gets saved in the APCU file cache by the calling getDefinitions() method.

    So assuming a module is not installed, and discovery for this plugin leads to an empty result, when the module is installed, discovery won't pick up the plugin because no changes have been made to the file. This situation isn't detectable by anything other than browser tests, because in other tests, all module classes are discoverable.

    We've been working on some similar solutions in https://www.drupal.org/project/drupal/issues/3421014 because migrate source plugins specifically need this, but it's kinda stalled.

  • Author Contributor

    Aren’t discovery caches cleared upon a module’s installation (in normal, no migration, circumstances)?

  • The discovery cache yes. But when discovery looks for plugins again, the file cache that holds the plugin data is not cleared unless the modified time of the class file has changed.

    In the migrate source conversion, there were browser tests that were failing for this very reason.

    Edited by godotislate
  • Author Contributor

    Removed usage of the file cache for attribute-based discovery.

  • Please register or sign in to reply
  • mondrake added 44 commits

    added 44 commits

    Compare with previous version

  • mondrake added 1 commit
  • mondrake added 1 commit

    added 1 commit

    Compare with previous version

  • mondrake added 4 commits

    added 4 commits

    Compare with previous version

  • mondrake added 28 commits

    added 28 commits

    Compare with previous version

  • mondrake added 1 commit

    added 1 commit

    • 17946da2 - fix SlevomatCodingStandard.Exceptions.RequireNonCapturingCatch.NonCapturingCatchRequired

    Compare with previous version

  • mondrake added 44 commits

    added 44 commits

    Compare with previous version

  • closed

  • Please register or sign in to reply
    Loading