Pipelines are green, yay.
The future update tests, relevant for mysqli, will be those introduced to update from 11.2 to above, right? Because those introduced before 11.2 are not relevant since the module is not released yet.
Then the task will be to add a new database dump for 11.2.0, at that moment including the install of myqli.
But that's a separate issue from here if we agree to proceed as per #140. A new dump will get its own 11.2.0 version name and this check
// Determine the version of the database dump if specified.
$matches = [];
$dumpVersion = preg_match('/drupal-(\d+.\d+.\d+)/', $file, $matches) === 1 ? $matches[1] : NULL;
// If the db driver is mysqli, we do not need to run the update tests for
// db dumps prior to 11.2 when the module was introduced.
if (Database::getConnection()->getProvider === 'mysqli' && $dumpVersion && version_compare($dumpVersion, '11.2.0', '<')) {
$this->markTestSkipped("The mysqli driver was introduced in Drupal 11.2, skip update tests from database at version {$dumpVersion}");
}
will no longer skip the test, if the test requires that dump. But it will keep skipping tests using the 10.3.0 dump.
In other words: if we do this, we do not need to change the fixtures now. We can wait for 11.2 to be released.
In https://git.drupalcode.org/project/drupal/-/merge_requests/11355/diffs?c..., we are skipping the update tests if the driver is mysqli and the database dump is from drupal-10.3.0.
I think this is more realistic and avoids tampering with database dump fixtures.
#138 well we are missing a c/p of mysql.install in this module so that the requirements can be rendered... that file was added to core 2 years ago in #2733675: Warning when mysql is not set to READ-COMMITTED → but did not get its way into the mysqli module. So the test failure is legitimate I think and we should fix it.
Thanks @daffie
Sanity check on #136, before I get hands dirty with db fixtures: apart from Drupal\Tests\mysql\Functional\RequirementsTest, all the other failing tests are update tests that rely on the 10.3 db dump fixtures.
Are we really sure we want to rewrite history and pretend a mysqli module was available in 10.3? In real life no one will ever update from a 10.3 install that already has mysqli installed. So is it right to rewrite history to make the tests happy, or should we just skip somehow the tests?
The situation in #71 was different - there we had the 'modulification' of db drivers that were already present beforehand, here we are introducing a totally new module.
I'll ping @catch for an opinion here too
#134 I looked at tests in that namespace, but still I miss what such a test should be written for, here.
I know we have made this module "hidden", but should we have installertests?
I am not familiar with such tests; can anybody point me to an example to work from, in case?
Added a mock CR... please help filling it in :)
@daffie
A number of tests only are run for certain database drivers. When such a test is run for the "mysql" database driver, the "mysqli" database driver needs to be added.
Are you talking about the kernel driver-specific tests? If so, they are run for mysqli based on inheritance from the mysql ones, we need not do anything. If something else, can you elaborate?
Filed https://github.com/drush-ops/drush/issues/6266 in drush issue queue; drush will need some adjustment to make it work with this module when it will be committed to core.
Some comments.
- General topic: adding new PHPStan rules to Drupal core. As already found in 📌 Introduce a @return-type-will-be-added annotation to indicate return type addition in next major; enforce with a PHPStan rule Active , new Drupal-defined rules added to Drupal phpstan.neon will not be visible to custom/contrib unless they also add the rule to their projects' neon files. mglaman/phpstan-drupal in that sense is different because in that case all the rules are added as part of the extension installation via composer. Unless we do something, there's high risk that Drupal core own rules will not be checked. The existing rule is different because it only checks Drupal component code that is not present in modules/themes/etc.
- As discussed in
✨
Add a trait for autowiring properties in tests
Needs work
, it seems to me that only preventing usage of
$this->container->get()is not enough. I understand the problem is that any service should always be accessed from the singleton. Therefore we should also discourage contructs that store a service in a test property or a local variable like
$cache = \Drupal::service('cache'); ... $cache->get(...);no? That would be tricky.
- The new PHPStan rule needs tests. Actually see how test coverage instruments in PHPStan rules testing are adding the red line in the MR where the rule is introduced.
mondrake → created an issue.
Charset toggling was removed in 📌 [policy] Decide what to do with MYSQLND_MINIMUM_VERSION and LIBMYSQLCLIENT_MINIMUM_VERSION Active , adjusted Connection::open() accordingly.
Pre-8.0 MySql ANSI modes were removed in 📌 Update INSTALL.txt and friends with Drupal 11 platform requirements Postponed , adjusted Connection::__construct() accordingly.
Introduced a SchemaPrimaryKeyMustBeDroppedException to abstract the error and drop the Schema class.
There's a smarter way than adding a Schema class override. Working on it.
Filed 🐛 @phpstan-ignore-next-line annotation collides with PHPCS Active in the Coder issue queue.
mondrake → created an issue.
For review. Fixing fixtures for update tests is a real PITA, so please let's focus on other items for now and let's address them when we are all comfortable with all the rest. Thanks.
IMHO this can go to sleep - GitlabCI is now used for core, run-tests.sh has changed in that part, and contrib is recommended to use PHPUnit CLI to execute tests
From #3515706-5: Switch the default test environment to PHP 8.4 → :
I wonder would it make sense to switch to PHPUnit 11 for 8.4 too? And let 8.3 and below onto PHPUnit 10? https://stitcher.io/blog/php-version-stats-january-2025
I wonder would it make sense to switch to PHPUnit 11 for 8.4 too? And let 8.3 and below onto PHPUnit 10? https://stitcher.io/blog/php-version-stats-january-2025
Somehow though, this will reopen the discussion from #3484966-6: Allow executing unit tests on multiple PHP images without having to duplicate unit tests across all environment combinations → because then the tests results will be split in two places - the 'main' pipeline for unit tests and the children pipelines for kernel/functional/etc.
https://gitlab.com/gitlab-org/gitlab/-/issues/363019 is the upstream issue to keep watching - still no timeline there.
Related, 📌 [PP-1][CI] Spin off Drupal Components tests in a job of their own Postponed .
We are talking about a job stage, not a child pipeline here, right?
mondrake → created an issue.
Well that issue, albeit closed, gives a lot of detail about exactly why the change is needed, IMHO.
#17 that’s just the summary. All the details are visible in the raw logs https://git.drupalcode.org/issue/drupal-3485069/-/jobs/4674359 and as artifacts.
Obviously a code coverage analisys service (e.g. Codecov or the likes of it) would provide full interactive reporting, trends etc but I suppose it’s well beyond the scope of this issue.
#15: see https://docs.gitlab.com/ci/testing/metrics_reports/
Apparently, not.
Made some progress. This is now postponed on 📌 [PP-1] Introduce a StatementBase abstract class Postponed .
mondrake → changed the visibility of the branch 3259709-create-the-database to hidden.
#11 is actually fixed upstream already in 7.9.0, that is only compatible with PHPUnit 12 - this will be the bigger problem to use paratest in Drupal: it only supports the latest PHPUnit version, that Drupal is consistently late in adopting at the moment.
The missing testdox output is caused by a change in PHPUnit 11.5.12 that is not matched by paratest 7.8.3
With some hacking I managed to fix it here, but will require being fixed upstream.
Adding a test failure and a deprecation, observations:
--testdoxcommand line toggle does not seem to work- deprecation reporting seem to be working fine (re comment #5 above)
MR!11573 combines 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed with 📌 [PP-1][CI] Spin off Drupal Components tests in a job of their own Postponed and runs Drupal\Component tests using paratest without any change.
PHPUnit CLI: PHP 8.3 43sec; PHP 8.4 43sec; PHP 8.5 44sec.
paratest (using 72 processes): PHP 8.3 18sec; PHP 8.4 19sec; PHP 8.5 23sec.
#57 @andypost that's because in PHPUnit 10 the arguments coming from the DataProvider are passed to the test method as a list sequence (regardless of the array keys), whereas in PHPUnit 11 the array keys are used to pass each parameter to the corresponding named argument of the test method. So effectively PHPUnit 11 swaps arguments vis a vis PHPUnit 10 for the same test unless in PHPUnit 10 the sequence of passed-in arguments is aligned to the sequence of the arguments in the test method's signature.
See
Looks like PHP is going to deprecate semicolons soon: https://wiki.php.net/rfc/deprecations_php_8_5#deprecate_semicolon_after_...
So a Drupal standard may become rather irrelevant.
Reopening, this was deferred in 🌱 [meta] Support PHPUnit 11 in Drupal 10 Postponed
Updated IS, added a release note snippet, tests are green. CR still missing but let's agree this is good to go before writing one?
Done #52. Now PHPUnit 11 executes for Unit tests under PHP 8.5. It's a bit too conservative IMHO, but a first step.
Filed 📌 [consistent test failure] ImageDimensionsTest::testImageDimensions() Active as a replacement of this one.
mondrake → created an issue.
I am working on this.
So let's close this for now, also based on feedback in https://drupal.slack.com/archives/C079NQPQUEN/p1742553484074889
NR for #81
#52 blast from the past :)
Maybe for now it would be better to do the other way round, i.e. keep PHPUnit 10 in the lock file, with relaxed constraints in composer.json, and allow opt-in to PHPUnit 11? So core can bump to PHPUnit 11 in its test pipelines, while we do not force everyone to downgrade from 11 to 10.
Yeah this is messy now, probably better close this and start with a new issue aiming at fixing the test failure (if we want to, this is probably a rather edge case).
The original intent here was to refresh the Rectangle class with an algo that is more aligned with PHP 8. The calculations done currently are mirroring PHP 5 - this means that TransformDimensions consistently produce results that differ from the actual results of processing images via GD.
The main problem is that it’s difficult to target a ‘standard’ algo because PHP relies on the GD library to perform the actual processing, different versions return different results, and you cannot predict which version of GD is used by the PHP instance used for testing.
mondrake → changed the visibility of the branch 2921123-adjust-rectangle-class to hidden.
mondrake → changed the visibility of the branch 2921123-adjust-rectangle-class to active.
Better do this after 📌 [PP-1] Allow using Table identifier value objects in database Query classes Postponed .
This issue is, if I understand correctly, about the fact that the term 'field' should have never reached the database API, where it's all rows and columns.
I think this is still a valid point, even if it's more a bit of an academic problem at the moment then anything else: everything works just fine as-is today - most probably people just got used to this.
Since when this issue was born, we have now a proper deprecation process in place. If someone wants to tackle this, replacing Schema::addField, ::changeField, etc. with Schema::addColumn, ::changeColumn is feasible in an orderly manner. Probably a bit more problematic replacing SelectInterface::fields() and friends if nothing else because of the sheer amount of change it will imply.
Asked a question, inline.
File follow up 📌 [PP-1] Allow using Table identifier value objects in database Query classes Postponed .
mondrake → created an issue.
Updated fork.
#24 thanks Daffie for your review!
1) for sure we can undeprecate those, or postpone removal to a later major (D13+). But why would that be desirable? Besides that those 'wrappers' would be redundant, what would someone call those for, given that the value objects encapsulate the result of those wrappers? Besides, once we move to value objects then we could pass them as arguments for methods instead of strings and the reason for such wrappers would simply not hold anymore - see for example [ #3514117] and how sqlite's Schema class would benefit.
Supposing a prefix 'foo' is required, given that the Table identifier implements \Stringable and it returns the machine name identifier:
$table = $this->connection->identifier->table('try_me');
echo "SELECT * FROM $table";
would return, without any further processing:
for SQLite:
SELECT * FROM "foo"."try_me"
for MySQL
SELECT * FROM "footry_me"
for PostgreSql (supposing a 'bar' schema is in use)
SELECT * FROM "bar"."footry_me"
2) that's exactly what the MR is doing no? Identifier/Database, Identifier/Schema, Identifier/Table are value objects, defined final - they only carry the values. The values themselves are determined by each driver's IdentifierHandler class, that parses the 'raw value' string identifier and instantiates the value objects with the needed values. Each driver's IdentifierHandler extends from the abstract IdentifierHandlerBase which has the 'regular' implementation - and all drivers need diversions one way or another, say for the different max lenght, or for how tables are prefixed, or for adding schemas etc.
Filed follow-up 📌 [PP-1] Use Table identifier value objects instead of table name strings in database Schema classes Postponed
mondrake → created an issue.
📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed now has a MR in NR that addresses this (for the time being for table identifiers) on the Database API level.
📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed now has a MR in NR that addresses this (for the time being for table identifiers) on the Database API level.
📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed now has a MR in NR that would address this (for the time being for table identifiers) on the Database API level:
Done #22, back to NR.
The machineName in the value objects should be stored unquoted. On that.
Ready for review now.
Added pgsql conversion. Tests are now green on all 3 core drivers. The concept is in and implemented; now to cleaning, BC and documenting.
Added a small script that processes PHPUnit's coverage text report into an OpenMetrics file with the relevant metrics included.
See file attached with a screenshot of the resulting report in the MR.
Added a small script that processes PHPUnit's coverage text report into an OpenMetrics file with the relevant metrics included.
See file attached with a screenshot of the resulting report in the MR.
Adding related issue
With regards to #8, we already have had coverage visualization configured in the MR.
This is producing inline display of the covered line in the merge request diffs. It's not visible in this MR as it does include changes to components code, but an example is here:
📌
Introduce a @return-type-will-be-added annotation to indicate return type addition in next major; enforce with a PHPStan rule
Active
: in the MR visualization of changes https://git.drupalcode.org/project/drupal/-/merge_requests/10111/diffs#7... you can see that the covered code has a green sideline close to the line numbers.
When it comes to summary metrics, IMHO the standard GitLab solution with adding a coverage key to the job yaml is insufficient, for the following reasons:
- trivial but annoying: it is not compatible with color output by PHPUnit's coverage text reporting. If you set
--colors=alwaysto PHPUnit CLI command, the coverage text report includes ANSI color codes that the regex in coverage cannot cope with. - more importantly: the standard approach is limited to reporting line coverage only and averages results from different jobs - this would lead to meaningless figures if we were later to, say, have code coverage for core too as it will make simple average without any weighting etc.
So I will try to take cue from the work in 📌 [CI] Report PHPStan baseline statistics in job Active , and see if with some postprocessing of the coverage text report, we can get a metrics report that can be more tuned to Drupal.
#11 well that's a new image thanks to @andypost, following the scheme of the Docker images in use by Drupal - so theoretically it should be available to contrib too. But not my piece of cake, maybe some fine guys from the infra team can comment.
So, here is the idea.
Developers should not be worried by exceeding identifiers length or using wrong identifiers; the database API should be self-sufficient in receiving input and cleansing/transforming it in machine usable information; failing as a last resort.
Therefore:
- Table names, database name, column names, index names in a db are 'identifiers'.
- 'Identifiers' are subject to db-specific limitations (length, chars allowed, etc)
- In Drupal, 'Identifiers' are value objects. Each identifier has:
- a type (e.g. table, column, alias, etc)
- a raw value (e.g. 'config', 'key_value', 'mydb."key_value"', 'with$$invalidchar', etc)
- a canonical value, i.e. the raw value cleansed of unallowed characters (e.g. from the above 'config', 'key_value', 'mydb.key_value', 'withinvalidchar', etc)
- a machine value, i.e. the canonical value transformed to be used on the db-specific SQL query - including prefixing and/or shortening (e.g. from the above, assuming a prefix 'abba': '"abbaconfig"', '"abbakey_value"', '"mydb"."key_value"', '"abbawithinvalidchar"', etc)
This works now for MySql and SQLite; the MySql implementation shortens table names that are longer than 64 characters using hashing techniques.
PgSql will follow with its own 63 chars max limitation.
In follow ups, this approach could be extended from tables to columns, aliases, indexes etc.
#8 I tried to configure coverage reporting, but it does not seem to produce any effect?
#5 yes, but not introduced here. See 📌 [CI] Introduce a separate stage for unit tests Active .
ftr, 📌 Deprecate TestDiscovery test file scanning, use PHPUnit API instead Active would add printout by run-tests.sh of the PHP version in use, which now is missing. ATM we would only be able to see it if a test fails and the full PHPUnit test class report is output.
I suggest to address GD failures of the GDToolkit elsewhere.
IMHO we should not change the PHP default in this issue. Only use PHP 8.5 to run unit tests (no kernel, no build, no functional)
📌 [CI] Run Unit tests on PHP 8.5 nightly Active has a MR that would run the unit tests on PHP 8.5, but (obviously) the image is not yet available in production. I honestly do not know what could I try?
Filed 📌 [CI] Run Unit tests on PHP 8.5 nightly Active .
Is the new image already available to MRs?
mondrake → created an issue.
mondrake → created an issue.
Hi! Please note that here we are deprecating usage of a method parameter, not of a whole method.
The instructions to be followed for this case are here https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... →
This is a dupe now.
Added CR https://www.drupal.org/node/3512006 →
Thanks @daffie!
Hi, thanks - we would need automated tests here so it's clear the expected behavior (I don't understand is it an image printed on STDOUT, or a text resulting from a function calculation, or what?) and we prevent possible regressions in the future. Suppose this is a side-usage of the toolkit, right? Nothing to do with GDToolkit behavior (canonical 'expected' behavior of a Drupal image toolkit)?
Thanks for review @daffie!
In general, I just c/p DriverSpecificTransactionTestBase into the new TransactionYieldTest, to try and keep the two test sets (one testing the commit-on-destruct behavior, one testing the commit-on-yield one) in sync, doing as few changes as possible. Anyway, I will look into your input and adjust as much as possible.