Applied @godotislate suggestion, and rebased.
preg_match('/(Class|Interface) .* not found$/', $e->getMessage()) isn't the exception message (potentially) localised in PHP?
Just asking.
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…
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.
@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.
Can benchmarking per #14 be done in a follow up? It would be a bummer to release D11 without this fix.
Looking at Issue Priority docs, https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →
Critical bugs include those that:
[...]
Cause tests to fail in HEAD on the automated testing platform for any supported environment (including random failures), since this blocks all other work.
[...]
I think this is Major if not Critical, since this issue has been systematically failing all SQLite test runs for quite some time.
Back to NR.
I'd like to add more specific tests for the case of attempted rollback after a DDL statement is executed. On that.
merged with head
The SQLite test failure is unrelated; should be fixed by 🐛 GenerateThemeTest::testContribStarterkitDevSnapshotWithGitNotInstalled fails on sqlite Needs review .
@quietone your change of the title text does no longer read. Unsure what you meant to change, so leaving as is.
In the alternate MR, made the interface empty. So we can typehint with the interface where we can accept both Connection and NonTransactionalConnection, or the more specific class where needed.
Also, tried a different approach to mark a db driver allowing or not concurrent non-transactional connections, which does not involve services.
Finally - diverging MySql from SQLite here is starting to scare me a bit, we would end up everywhere in very different db state results depending on whether the db supports or not the concurrent non-transactional connection...
Looking a bit at SQLite docs, https://www.sqlite.org/wal.html and https://www.sqlite.org/lockingv3.html, it looks to me that SQLite cannot perform a DML statement in the non-transactional connection while an open transaction in the normal connection is holding the flushing of a DML statement. For example: INSERT in nonTransactionConnection while a transaction is open in Connection, and an INSERT is in the transaction --> Database Locked exception.
Could not find a way to circumvent this, so the problem here is bigger I am afraid - and do not think depends on the MR.
On the interface - I added the open() method, but in fact it could even be empty and just be there to indicate the commonality between Connection and NonTransactionalConnection; also, we do not necessarily need to change everywhere, only where we could have one of the two alternative classes.
MySQL passes tests, but SQLite and PostgreSql fail quite spectacularly
Opened a new branch and MR (MR!8762) to try and introduce a minimal interface, common to both Connection and NonTransactionalConnection. The idea is to use this interface where we can inject both.
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
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
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?
mondrake → created an issue.
FTR, Sophron module was adjusted to mirror this.
My 0.02$: throwing/catching something like an InvalidPluginClassException would be best - clean, unambiguous, testable
Thanks @longwave, opened 📌 Consider using exceptions instead of null values to signal invalid plugins during discovery Active for follow up.
mondrake → created an issue.
It's not just about image related plugins, it's about ALL plugins.
rebased
mondrake → made their first commit to this issue’s fork.
mondrake → created an issue.
Done #6.
mondrake → created an issue.
mondrake → created an issue.
mondrake → created an issue.
mondrake → created an issue.
4.5 is planned to be D11 compatible
Version 4 is planned to be D10 compatible
The issue that started this was fixed AFAICS.
mondrake → created an issue.
Rebased MR!8054
mondrake → created an issue.
Thought about this, and IMHO dumping to a file for it to be tracked in real time via tail -f would better be done with an alternative extension, rather than trying to fit both behaviors in one.
mondrake → created an issue.
+1 on the issue, but -1 on the proposal: it states the obvious.
I'd rather
omit the first line comment.
as the alternative suggestion says.
Other projects just add extra annotations (including phpstan or psalm ones) for the parameters where it's relevant to add them.
Interesting. If I understand your idea, that would mean running two separate terminals, one running PHPUnit CLI, and the other the tail command? That would pose the problem of knowing in advance the filepath of the temp file across both terminals, and of course of avoiding the hashing of the file content that the MR is currently doing.
Well that would mean writing to STDOUT, so back to square one. This issue is, fundamentally, avoiding to use standard streams - just capture the output of dump in a temp file, and post-test-mortem, print it via the standard streams of the CLI testrunner that are no longer being checked by PHPUnit for unexpected output. No live output, in this case, sorry - it's the design of the MR itself that prevents it.
Note that if you call dump() from a component test that extends directly from PHPUnit's TestCase, or from a Drupal unit test case not run isolation, without the extension being active, then yes, you got exactly that - live dumps, and for free since there is not even the need to do anything.
This entire issue is about tests run in isolation. It's process isolation (and all kernel+functional tests, plus a few Drupal unit tests, are executied in isolation) that causes the dump() failures.
At least, that's my understanding - but I'd love to be proven wrong :)
No, we changed from STDOUT to STDERR already, and the newly reported failures in kernel tests are related to this.
Updated the main CR https://www.drupal.org/node/3365413 → to reflect this.
Added
<parameter name="printCaller" value="true"/>
as a config parameter for the extension.
I guess this is required because of changes in PHPUnit 10?
Well yes and no. There were already changes in PHPUnit 9 that forced to change the current solution to print to STDERR instead of STDOUT. The real problem is that, to an extent, test code and SUT code share the same standard streams. Quite rightly IMHO, especially if tests need to run in isolation, PHPUnit is 'sealing' more and more the SUT to prevent confusion between SUT and test output.
No VFS, sorry. In tests run in process isolation, the VFS instance will only be visibile to the child process, and then destructed before returning to the main process. Or viceversa, if visible to the main process, it won't be visible to the children.
I want 'somethingbad' to stick out in the last, and the list being simple is important to being able to do this. Filenames and line numbers will hamper this.
We could add a parameter in the extension configuration in the xml file to opt-out from printing the whereabouts of the dump() call.
See 📌 Remove Jquery Colorpicker support Fixed .
I think it’s more accurate to typehint \Generator where yield is used.
iterable is an alias for array|\Traversable which means it’s the most generic typehint possible. See https://www.php.net/manual/en/language.types.iterable.php
mondrake → created an issue.
mondrake → created an issue.
mondrake → created an issue.
mondrake → created an issue. See original summary → .
mondrake → created an issue.
mondrake → created an issue.
mondrake → changed the visibility of the branch 3454781-release-8.x-3.7 to hidden.
mondrake → created an issue.