- Issue created by @mondrake
- Status changed to Needs review
2 months ago 1:09pm 1 July 2024 - 🇮🇹Italy mondrake 🇮🇹
It's not just about image related plugins, it's about ALL plugins.
- Status changed to RTBC
about 2 months ago 1:07pm 12 July 2024 - 🇬🇧United Kingdom longwave UK
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.
- 🇮🇹Italy mondrake 🇮🇹
Thanks @longwave, opened 📌 Consider using exceptions instead of null values to signal invalid plugins during discovery Active for follow up.
- Status changed to Needs work
about 2 months ago 9:03pm 12 July 2024 - 🇬🇧United Kingdom longwave UK
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?
- 🇮🇹Italy mondrake 🇮🇹
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?
- 🇮🇹Italy mondrake 🇮🇹
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
- Status changed to Needs review
about 2 months ago 11:05am 13 July 2024 - 🇬🇧United Kingdom longwave UK
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.
- 🇮🇹Italy mondrake 🇮🇹
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
- 🇮🇹Italy mondrake 🇮🇹
Can benchmarking per #14 be done in a follow up? It would be a bummer to release D11 without this fix.
- First commit to issue fork.
- 🇬🇧United Kingdom catch
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.
- 🇮🇹Italy mondrake 🇮🇹
@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.
- 🇬🇧United Kingdom catch
Plugin discovery caches should be cleared on module install/uninstall, so that part ought to be OK I think?
- 🇮🇹Italy mondrake 🇮🇹
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.
- 🇬🇧United Kingdom catch
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.
- 🇮🇹Italy mondrake 🇮🇹
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.
- 🇮🇹Italy mondrake 🇮🇹
preg_match('/(Class|Interface) .* not found$/', $e->getMessage())
isn't the exception message (potentially) localised in PHP?Just asking.
- Status changed to Needs work
about 1 month ago 2:01pm 26 July 2024 - Status changed to Needs review
about 1 month ago 7:46pm 26 July 2024 - Status changed to Needs work
about 1 month ago 10:07pm 26 July 2024 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.
- Status changed to Needs review
about 1 month ago 2:44am 27 July 2024 - First commit to issue fork.
- Status changed to RTBC
about 1 month ago 2:09pm 30 July 2024 - Status changed to Needs work
about 1 month ago 1:37am 31 July 2024 - 🇺🇸United States xjm
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. :)
- 🇬🇧United Kingdom catch
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.
- Status changed to Needs review
about 1 month ago 4:47am 31 July 2024 - Status changed to RTBC
about 1 month ago 12:20pm 1 August 2024 - 🇬🇧United Kingdom catch
OK I think that's enough to move this back to RTBC.
-
larowlan →
committed 7a331d50 on 11.x
Issue #3458177 by mondrake, catch, godotislate, longwave, xjm: Changing...
-
larowlan →
committed 7a331d50 on 11.x
-
larowlan →
committed 27a40bef on 10.3.x
Issue #3458177 by mondrake, catch, godotislate, longwave, xjm: Changing...
-
larowlan →
committed 27a40bef on 10.3.x
-
larowlan →
committed 7aacebf3 on 10.4.x
Issue #3458177 by mondrake, catch, godotislate, longwave, xjm: Changing...
-
larowlan →
committed 7aacebf3 on 10.4.x
-
larowlan →
committed e8aec68a on 11.0.x
Issue #3458177 by mondrake, catch, godotislate, longwave, xjm: Changing...
-
larowlan →
committed e8aec68a on 11.0.x
- Status changed to Fixed
about 1 month ago 11:17pm 1 August 2024 - 🇦🇺Australia larowlan 🇦🇺🏝.au GMT+10
Committed to 11.x and then backported to 11.0.x/10.4.x/10.3.x after confirming with catch
Thanks folks 🚀
- 🇬🇧United Kingdom catch
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
- Status changed to Needs work
about 1 month ago 6:46am 2 August 2024 - 🇬🇧United Kingdom catch
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.
- Status changed to Needs review
about 1 month ago 9:24am 2 August 2024 - 🇬🇧United Kingdom alexpott 🇪🇺🌍
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.
- Status changed to RTBC
about 1 month ago 4:24pm 2 August 2024 - 🇬🇧United Kingdom catch
Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!
- Status changed to Fixed
about 1 month ago 10:43pm 2 August 2024 Automatically closed - issue fixed for 2 weeks with no activity.