Changing plugins from annotations to attributes in contrib leads to error if plugin extends from a missing dependency

I think this makes sense and I can't see a simpler or better way to do it.

However let's open a followup to discuss that return value of parseClass() because I agree the structure with NULLs is strange, perhaps it should throw an exception in the failure case instead? The reason we do it is:

              ['id' => $id, 'content' => $content] = $this->parseClass($class, $fileinfo);

so the return value is always expected in that structure. I think this is all internal enough that we can change it without BC implications, but the followup can help us decide that.

Ugh, yeah, there is caching bug there, but not sure how to work around it.

Also, how about logging something if we encounter an exception, instead of swallowing it - it might help developers if they have typo'd something or similar that prevents discovery from working?

File cache was probably much more important when we were parsing for annotations? But maybe we can move it to that layer and drop it for attributes, though it would be good to do some benchmarking if possible.

I don't think we should get rid of the file cache here without performance numbers - but even worse we don't know what attribute discovery even looks like on a large site yet because not all of core and contrib is converted yet, so any numbers we did get would be artificially better than they might otherwise be.

However, I think I found a way to fix the error, avoid writing to cache just in this case, and leave the file cache for everything that currently would be successfully found and cached. Because this is an alternate approach, have made the change in a separate MR. I restored the test that was removed here, and made the minimal changes to the new test so that it still parses - I think it might be enough but maybe we need to explictly test the missing attribute class with file caching too. I initially did the fix wrong and the test found the error though at least.

Plugin discovery caches should be cleared on module install/uninstall, so that part ought to be OK I think?

So on module (un)install all the plugin files will be scanned in the directories, but only those missing from the file cache effectively also parsed? Sounds good - the file cache will have more but will not be retrieved.

Yes - it'll find all the files, but only actually parse the ones that aren't in the cache (which my MR should stop them going in), or whose mtime has changed.

I think I realised more what you meant in #21 though, scenario would be something like this:

1. Module A has a plugin that relies on an an attribute defined by an uninstalled module B -> doesn't enter into FileCache or discovery.
2. Module B is installed - now the plugin is in both FileCache and discovery.
3. Module B is uninstalled again - the discovery cache will be cleared, but the FileCache won't.

So now we have a file in FileCache that uses an attribute defined by an uninstalled module.

I guess that leaves a couple of questions:

1. Should we be clearing the file cache for attributes on module uninstall (just uninstall, since the MR should cover install).

2. Or is it OK, given that this is how annotations used to work anyway.

Made one comment on the MR: there's been discussion on #3421014: Convert MigrateSource plugin discovery to attributes not to just eat all exceptions, so I put in a suggestion to target the specific error based on pending work over there.

There's also discussion on how to deal with missing traits in that issue as well, but the solution is more complicated.

Looks like no.
https://stackoverflow.com/a/11609263
https://github.com/php/php-src/blob/ab449a7e466570f5700ff91548d2154e9c44...

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.

Rebased.

lgtm.

Posted some comments on the MR. There are some docs minutiae that seem out of scope, or if they're in scope, I don't understand why and so clarification of the docs changes would therefore be needed. :)

I think I've addressed everything except the two comment changes that may or may not be out of scope, and the $file_path_2 naming which I don't have a strong opinion on.

OK I think that's enough to move this back to RTBC.

Committed to 11.x and then backported to 11.0.x/10.4.x/10.3.x after confirming with catch

Thanks folks 🚀

Looks like this is breaking pipelines on 10.3.x somehow (seen in performance test pipeline, not sure about anywhere else) https://git.drupalcode.org/project/drupal/-/jobs/2310094

OK this is because phpunit test discovery is different in 10.x, and the test class is being picked up as if it's a test. run-tests.sh doesn't use the suites, so it's mainly a problem for local phpunit test runs + performance tests.

Two options I think:

1. Just remove the test coverage in 10.x, rely on it being in 11.x

2. Move the intentionally broken class somewhere else - a test module, fixture directory etc., where it won't be mistaken for a test class.

I was just going to comment that the only way to fix this is to move the classes under core/tests/fixtures ... 1 small nit on the MR. +1 for the fix.

Tests are green, #54 was addressed, so lgtm.

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

Amending attribution.