Hi @daffie, my opinion:
#21.1 - I think the opposite, i.e. that we should remove $dbSpecificType from Column.
$dbSpecificType kills abstraction, and is impractical as it forces core to add definition for db drivers that it does not own. And in fact it does not work: do you see specific types for Oracle or MSSql or anything else which is not a core-supported driver?
However, until 21.2 is done, I'd be fine to add what you would suggest as a stop-gap, with the understanding that it will only probably serve core db drivers anyway. Asking for framework manager review in that case.
#21.2 - that is EXACTLY the endgame that #3410480-4: [META] Use events in Database Schema operations → describes, please look at it and at the PoC MR in it. The idea is
(1) SchemaDefinition (abstract) --> (2) dispatchSchemaDefinitionEvent (abstract) --> (3) subscribeSchemaDefinitionEvent (DBMS-specific) --> Schema (DBMS-specific)
(1) SchemaDefinition is a DMBS-abstract, immutable value object
(2) when we need to create a table on my TimeWarpBase database, we dispatch an event with the SchemaDefinition object as input that requests subscribers to collect the db-specific specs in the Schema object
(3) my TimeWarpBase database driver implements the subscriber and reacts on (2), providing the TimeWarpBase-specific specs
(4) at the end of event processing, the Schema objects has the dbms-specific specs necessary for creating the table
#9 sounds like a good first step, let's do that and see how the numbers will change. Opened a separate issue 📌 Remove @legacy-covers in cases where the test method name starts with the covered method Active for that.
Let's discuss #7 first.
The options I see:
1) we just remove any @legacy-covers through a Rector rule. Easy, but we may lose meaningful information.
2) we convert @legacy-covers to a generic comment through a Rector rule. Example;
from
/**
* @legacy-covers ::buildProxyClassName
*/
public function testBuildProxyClassName(): void {
to
/**
* Tests the ::buildProxyClassName() method.
*/
public function testBuildProxyClassName(): void {
Easy, but could lead to silly docblocks like e.g.
from
/**
* Tests set form state.
*
* @legacy-covers ::setFormState
*/
public function testSetFormState(): void {
to
/**
* Tests set form state.
*
* Tests the :setFormState() method.
*/
public function testSetFormState(): void {
3) We bring all the @legacy-cover entries defined on the test methods to the test class as a #[CoversMethod()] attribute. Relatively easy again, but would end up in a messy combination of #[CoversClass] and #[CoversMethod] attributes on the class level, and coverage declaration will not be relevant any more on the pair-runtime method<->test-method.
4) We split all test classes to test only one method each. Crazy difficult.
We also need to decide what we do when we have multiple @legacy-covers in the same docBlock.
#4 a new one just went in: https://git.drupalcode.org/project/drupal/-/commit/8443c9a570abef7bac414...
Thanks! Please target 5.0.x. Yes, this would need a config toggle in the UI. Also would need a test and a fixture pdf test file.
Filed a first follow-up: 📌 Convert hook_schema() implementations to SchemaDefinition Postponed .
Needs a rebase. That will show failures related to usage of annotations instead of attributes in the test code, that will have to be fixed too.
We cannot do #33, because undefined --sqlite means using the currently installed database. So latest commits fail Kernel tests because they try inserting into the SUT db and not on the test runner db.
@daffie now this starts getting a bit old and I cannot recall the details of two years ago. However, please read #3410480-4: [META] Use events in Database Schema operations → : the part that this issue is covering relates to the 'immutable' schema, the driver/db specifics should be dealt with somewhere else.
So for me the question here should be: what is
📌
Adding GIN and GIST indexes to PostgreSQL databases
RTBC
trying to achieve 'in abstract' i.e. independently from the db? Creating an index over a JSON column? If yes, then maybe we need another JsonIndex 'immutable' object to describe the schema. Then, each db would implement it on its own logic, Postgres with GIN/GIST, others they would just fallback to the normal index.
This is in principle, I haven't looked at the details. But my purpose here is to separate the abstract schema definition from the db implementation.
That said, I have no issue to remove final from the classes. As long as this is not seen as a door to make the 'abstract' schema, db-dependent.
FTR, @coversClass has never been a PHPUnit supported annotation...
We shouldn't, but I saw this happening in a commit recently (sorry I lost track of which one). But yeah - it's harmless anyway and now numbers are just too big to baseline them at the moment.
I think we can close this now. Yes there are couple of follow-ups open but the test codebase is now fully converted.
Setting to RTBC for any comment.
Good catch.
Few comments:
1) this is now partially overlapping with
📌
Enforce removal of PHPUnit annotations from test classes' class metadata
Postponed
, in the sense that it analyses same scope with slightly different checks. May be better try to have one rule only?
2) the check on #[Group] attribute presence here is limited to Functional and Kernel tests AFAICS - should be on any test class actually
Approx 1k instances of $this->container in Functional/FunctionaJavascript tests... I think that probably needs a meta and childs.
+1, can we check what would be the impact of deprecating access to $container here before deciding if to do here or in follow up?
Also, I wouldn't have allowed access to dynamic properties at all; minor though as in any case both warnings and deprecations are set to cause tests to fail.
Not sure it's worth changing this isolated case here.
My suggestion would be to do, for core/tests:
- 📌 Add return types to core tests code via Rector Active adding some return types
- Round 2 (issue tbd) - complete return types and add argument types via Rector
- Round 3 (issue tbd) - add types to class properties via Rector
- Round 4 (issue tbd) - convert anything that is left out
Rebased and reverted changes to DocParserTest, back to RTBC.
Per 4.0.0 release notes, this is expected as now the module enforces strict typing.
Let's be bold here, #28 is almost two years old and no activity since.
Looks like the consolidated approach now is to use autowiring attributes in code to solve this issue - I suggest to close won't fix this
Out of curiosity, I checked out the rotated image derivatives from the CI runs, and they're actually quite different in PHP 8.4 and 8.5. Attaching here.
This is an area where local testing may yield different results, as PHP with the embedded GD library (that Drupal GitLabCI uses) has code quite different from PHP that links the library separately, as #2921123: Adjust Rectangle class to calculate rotated image dimensions according to libgd 2.2.2+ → shown extensively.
All the new Kernel tests (one per image type) are green on PHP 8.5 now.
#29 - thanks @andypost, AVIF now looks OK in latest tests.
#28 - In the end I think we should be testing conditionally on PHP version, without changing the default interpolation algo. Latest commit does that.
IMHO
imagesetinterpolation($toolkit->getImage(), IMG_NEAREST_NEIGHBOUR);
should be placed in the toolkit code, conditionally on the PHP version, not in the test. Otherwise we'd be testing something that does not match the normal runtime behavior.
Or, we need to have different reference values to check against (again, PHP version-dependent).
Setting to NR for comments.
AVIF images misdetected as HEIF after introducing HEIF support in getimagesize() was fixed upstream so in PHP 8.5RC4 we should not longer have that part to be solved.
What remains is the different default interpolation used in PHP 8.5 vs earlier versions.
This test is one of the longest running kernel tests, I think it would make sense to split it in an abstract base class and one concrete test class per image type (gif, png, webp, etc.).
It should not be that hard, will try to look into it.
#93 and #94 IMHO the effort to try supporting global constants in unloaded .module files is not worth doing at this point. So dropped it in the last commit.
Retitled and updated IS.
#8 sure it would make sense. Can PHPCS selectively detect that the test class inherits from one of KernelTestBase or BrowserTestBase? Only those should have the attribute, Unit and Build tests do not need it, at least by default.
📌 doctrine/annotations is abandoned Active seems the culprit, may need reverting
Rebased post commit of 📌 Remove support for PHPUnit 10 Active
I think the CR of 📌 Remove support for PHPUnit 10 Active is enough. Not sure about a release note specifically for this issue, but leaving the tag.
mondrake → changed the visibility of the branch 3554077-incorrect-failure-message to hidden.
I think @smustgrave this is the issue 🌱 [META] [policy] Remove PHPUnit assertion messages when possible, and standardize remaining messages Active
The MR is built on top of 📌 Make Drupal\Core\Database\Database type strict and PHPStan L10 compliant Active , but it is reviewable now.
Newbie here, excuse my ignorance
Could this help with 🐛 [CI] Performance pipeline execution drops warmed caches Postponed ?
@smustgrave yes, or postponed on the referenced. There are already a bunch of issues about run-tests.sh that are not moving, alas, let's not add more.
📌 Enforce removal of PHPUnit annotations from test classes' methods metadata Needs review will report #50 as a PHPStan error.
Apparently, PHPUnit 11 is not throwing deprecations when @testWith is used...
Any reason why Drupal\image\ImageEffectBase is not converted?
Committed to 5.0.x and cherry-picked to 4.0.x
Not cherry-picked to 4.1.x because there is not likely going to be a release 4.1.0 ever, for the same reason of 💬 Stable release imagemagick 4.1.0 Active .
Thanks!