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

It's not just about image related plugins, it's about ALL plugins.

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 it probably should throw an catchable 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.

Thanks @longwave, opened 📌 Consider using exceptions instead of null values to signal invalid plugins during discovery Active for follow up.

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?

Not much into the plugin system, so this is naive…

can we just drop caching to filecache?

any module install/uninstall can potentially add/remove plugin types and actual plugins, so full scan is needed anyway on discovery?

If caching to filecache is due to considerations on reflection performance, then I think that needs to be reconsidered at the light of improvements in PHP 8.

https://gist.github.com/mindplay-dk/3359812 has benchmarks - and concludes

there is no performance-gain from caching reflection-objects

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.

Added deprecations and a draft CR.

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

AFAICS, Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery is already using file caching.

it would be good to do some benchmarking if possible

This is above me ATM

Can benchmarking per #14 be done in a follow up? It would be a bummer to release D11 without this fix.

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.

@catch thought about this earlier, what was preventing me to go in this direction is the fact that you might end up with info cached for files that no longer exist (i.e. if you uninstall a module + composer remove it). I am not fully in details of plugin discovery, but if that's not a problem, I think it's a great compromise.

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

mondrake → closed merge request !8606

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.

Closed the initial MR, cannot really RTBC @catch's one, needs someone else's review.

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.

Well my gut feeling is that’s ok for now. When annotations will be eventually gone, then it will make sense the benchmark to see if file caching performs better than running reflection every time. We may have surprises…

Made one comment on the MR: there's been discussion on 📌 Convert MigrateSource plugin discovery to attributes Needs work 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.

preg_match('/(Class|Interface) .* not found$/', $e->getMessage()) isn't the exception message (potentially) localised in PHP?

Just asking.

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

Applied @godotislate suggestion, and rebased.

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.

Green now.

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.

Addressed remaining three points.

Retracted the draft CR as no longer relevant.

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

larowlan → closed merge request !8922

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.

Let's try option 2. Should not be too difficult.

Let's see MR!9039.

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!

catch → closed merge request !9039

Automatically closed - issue fixed for 2 weeks with no activity.