🇮🇹Italy @mondrake

So we just go ahead renaming yield() to commitOrRelease() without implementing commit() and releaseSavepoint()?

I’d be +1 on that as I really value not having to care about what method to use, and the manager caring about that.

mondrake → created an issue.

Removed in Sophron 3.

I think #7 stands for test failure confirmation? Test-only does not fail because afaics the yaml service file change does not get reverted.

Done #152. Added pre/post screenshots drush and GUI.

Testing manually, I can see the new driver showing up at installation time. The CR says differently, so it needs update. Besides, there is no indication that the driver is experimental or hidden - those are 'module' notions, but at the stage of database selection in the installer modules are not yet a thing. So if you select the mysqli driver for installation, the 'experimentality' of the module is a consequence as the driver is included in the mysqli module that is enabled as part of the install process.

I will add explicitly '(experimental)' in the driver string for now. In a follow up, we could add an additional Connection method indicating and reporting the db client in use (it being PDO, mysqli, oci8, sqlite3, etc).

Found that mysqli::exec() does not exist while testing manually the GUI installation and database is not yet present.

mondrake → created an issue.

Rebased.

New stats:

----------------------------------------
PHPStan baseline statistics
----------------------------------------
 16312 * Total baselined errors
----------------------------------------
Breakdown by error identifier:
  8885 missingType.return
  3411 method.notFound
  2163 property.notFound
   315 method.nonObject
   248 property.nonObject
   241 return.missing
   168 variable.undefined
   152 class.notFound
   108 arguments.count
    89 missingType.generics
    56 isset.variable
    55 parameter.phpDocType
    47 varTag.differentVariable
    45 varTag.nativeType
    40 empty.variable
    39 return.phpDocType
    39 staticClassAccess.privateMethod
    31 constructor.unusedParameter
    27 varTag.variableNotFound
    18 property.phpDocType
    15 pluginManagerSetsCacheBackend.missingCacheBackend
    11 binaryOp.invalid
     9 class.extendsDeprecatedClass
     8 class.nameCase
     8 assignOp.invalid
     8 interface.nameCase
     8 method.void
     6 parameter.defaultValue
     6 phpDoc.parseError
     5 staticMethod.nonObject
     5 varTag.misplaced
     4 parameter.notFound
     4 cast.string
     4 encapsedStringPart.nonString
     3 varTag.noVariable
     3 staticClassAccess.privateProperty
     3 method.deprecated
     2 throws.notThrowable
     2 impureMethod.pure
     2 property.protected
     2 parameter.deprecatedClass
     2 staticClassAccess.privateConstant
     2 includeOnce.fileNotFound
     1 smallerOrEqual.invalid
     1 generics.existingClass
     1 staticServiceDeprecatedService.deprecated
     1 greater.invalid
     1 nullCoalesce.variable
     1 varTag.trait
     1 method.protected
     1 function.deprecated
     1 equal.invalid
     1 unset.offset
     1 callable.void
     1 staticMethod.void
     1 phpunit.coversMethod

Thanks for review

The test-only job does not seem to work - maybe because the change is only of the service yml file?

mondrake → changed the visibility of the branch 3519804-dummymimetypemaploadedsubscriber-does-not to active.

mondrake → changed the visibility of the branch 3519804-dummymimetypemaploadedsubscriber-does-not to hidden.

Small issue found in 🐛 DummyMimeTypeMapLoadedSubscriber does not get registered Active .

mondrake → created an issue.

#7 sure - that will never execute because $this->fileSystem and $this->map are always set (see __construct). PHPStan rules the world :)

Updated IS. Do not think we are losing anything:

• isset.initializedProperty errors relate to code paths that can never be executed.
• assign.readOnlyProperty error relates to a (rather opinionated) PHPStan rule that expects readonly properties are always only assigned in the constructor (see https://github.com/phpstan/phpstan/issues/6562). In this case with a private method being in the path, I think there's no harm in adjusting the code to ensure that.

Some code fixes driven by PHPStan findings.

I think we should add a test to check if DummyMimeTypeMapLoadedSubscriber has an effect on the mapping. The legacy hook was already not processed by Sophron, but now that we have methods we should be able to process the event. This event will trigger 'core' changes, while the Sophron's initializeMap will be specific to Sophron.

In essence, the sequence should be:
a) the Sophron map is initialized in the Drupal\sophron\MimeMapManagerInterface service, initializeMap event is triggered and will process map changes according to fileeye/mimemap primitives;
b) core triggers the MimeTypeMapLoadedEvent event in MimeTypeMapFactory and will process map changes according to MimeTypeMap primitives (that are proxies to the fileeye/mimemap ones via Sophron's MimeMapManager.

A bit complicated but this would allow keeping Sophron as a separate MIME type mapping service not used for guessing purposes.

Good point, thanks @kim.pepper

mondrake → made their first commit to this issue’s fork.

mondrake → created an issue.

Sophron 3.0.0-alpha1 → now no longer overrides Drupal's ExtensionMimeTypeGuesser, just the newly introduced MimeTypeMap.

Any review/input there appreciated.

mondrake → created an issue.

mondrake → changed the visibility of the branch 3519413-fix to hidden.

The core commit leads to this failure on the module

Exception: Deprecated function: Creation of dynamic property Drupal\sophron\CoreExtensionMimeTypeGuesserExtended::$moduleHandler is deprecated

Let's quick fix and release this so not to have issues when D11.2 is released.

Then we probably need a new major 3.0.x to deal with the other deprecations.

📌 [PP-1][CI] Spin off Drupal Components tests in a job of their own Postponed adds some more.

mondrake → created an issue.

This should be marked 'fixed' now, right?

Done.

Apologies if this sounds a bit scholarly... not my intention.

One thing I'm thinking about is rollback... Atm we call rollback on a transaction and that will go back to your previous savepoint or the end the root transaction without a commit - right? So given rollback already does 2 different things...

Right. But IMHO we should not assimilate the combination commit/release savepoint to the rollback. Commit permanently saves data to the DBMS, release savepoint doesn't. Rollback, doesn't matter whether to a savepoint or to the begin of the transactions, just returns to a temporary state within the transaction, it does not persist anything. Commit is very a specific, unambiguous action.

why don't we rename yield to commit() and document that it will commit the root transaction or release the savepoint depending on transaction depth as we already have this behaviour with rollback...

In an earlier version of the MR, it was like that. Then, I pondered the above and thought that a 'commit' method that does not really persist anything may be seen as a WTF - anyone not aware of all the details/not reading the docs would call 'commit' on a savepoint and then complain data are not saved.

For me if we add a Transaction::commit() method we should not be allowing ambiguity: it must persist the data as everyone would expect.

So, how about this next proposal:

• once instantiating a new Transaction object, store the transaction type in it
• add a Transaction::commit() and a Transaction::releaseSavepoint() methods, to be used respectively for... what they respectively mean
• in case we need to let the manager decide whether to commit or release a savepoint, we could still have a method like ::yield() with an appropriate rename, or we could have a simple match construct in calling code like
  

  match ($transaction->type) {
      StackItemType::Root => $transaction->commit(),
      StackItemType::Savepoint => $transaction->releaseSavepoint(),
  }

NR for the proposal in #38.

Thanks for review @alexpott.

Transaction does not have an interface for now.

We could add an additional Connection::startSavepoint() method, and Transaction::commit() + Transaction::releaseSavepoint() methods. That will somehow force how transactions are managed in code, though, in the sense that devs will have to be aware of whether they are working within the root transaction or within a savepoint. If some code starts a root transaction, you would not be able to encapsulate that code in a higher method if that will try to start a root transaction as well.

The beauty of the MR here is that you do not really care - you always call Connection::startTransaction and then it's the manager deciding what to do depending on the stack depth.

I am not going to make changes until we have agreed on a plan forward.

Indeed, it does!

Rebased, let's see if ✨ [CI] Generate full baseline upon PHPStan failure Active works

✨ [CI] Generate full baseline upon PHPStan failure Active got committed, this is actionable now.

Thanks for review @mstrelan, added some of your suggestions and replied inline to some other ones; NW to address the remaining comments.

I had to add the assert since PHPStan was complaining more or less for the same reason of your comment - PHPStan cannot determine what's included in the include (pun intended), so we need to inform it that a variable with that name is existing at that point.

Once the parent is committed I will rebase and add a comment.

From https://www.php.net/manual/en/function.include.php

When a file is included, the code it contains inherits the variable scope of the line on which the include occurs. Any variables available at that line in the calling file will be available within the called file, from that point forward. However, all functions and classes defined in the included file have the global scope.

So in this case, $databases is a local variable of InstallerNonDefaultDatabaseDriverTest::getInstalledDatabaseSettings() coming from the evaluation of the code in the included settings.php file.

I think this is outdated now - the method was alteady cleaned up and latest approach IIUC is to default anyway the module name to be equal to the druver name if not specified.

This was fixed in the referred issue.

IMO the test should have not been using the test container, that remains linked to the db driver passed in to the test. Since the essence of the test is to diverge from it in the SUT, let’s check the SUT’s GUI for the modules statuses.

Well, not if we had 📌 Introduce a @return-type-will-be-added annotation to indicate return type addition in next major; enforce with a PHPStan rule Active … we would be able to typehint the classes extending Connection.

Why would those method return null? I see it’s technically possible, but wouldn’t return type hinting or annotations do a better job in narrowing the type?

mondrake → changed the visibility of the branch 3406985-pp-1-convert-all to active.

I feel so

Sorry I was probably underestimating this when tagging for Novice. Removed tag. Working on this.

I think this is now superceded by 📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed .

composer update mglaman/phpstan-drupal phpstan/extension-installer phpstan/phpstan  phpstan/phpstan-deprecation-rules phpstan/phpstan-phpunit -w

mondrake → created an issue.

I may have missed completely how this work, but I am not sure we are really doing what we should. See question inline.

done, tks

To meaningfully manual test this, apply the MR patch locally, break some tests, and run tests via run-tests.sh, and llok at results.

Drush problem reported in #129 📌 Create the database driver for MySQLi for async queries Active is actually due to a bug in Connection::convertDbUrlToConnectionInfo() that is checking existence of a class before the dependent classes can be autoloaded. Fixed here, but this needs to go back to NR for the fix now, sorry.

That was it for an experiment.

Maybe there is a small misconception in the deprecation ignore file? See comment inline

While it's relatively straightforward to do this for plain INSERTs, this get complicated for all other operations where conditions get into the equation. The problem is that named placeholders are allowed in the expressions' string: while we could easily parse the strings to replace them with positional ones, that would defy the purpose of this issue that is to natively build SQL with positional placeholders, without need of reprocessing.

I start thinking we need something along the lines of 📌 [WIP] Decouple identifier management from database Connection, introduce an IdentifierHandler class Postponed , but for placeholders. So to have placeholders as value objects and not strings and therefore being able to build SQL taking that into account.

I closed 258.

Maybe first step would be to file an issue upstream @ https://github.com/sebastianbergmann/phpunit/issues just to see if there's any interest to have such an attribute directly in PHPUnit?

Merged MR with 📌 Create the database driver for MySQLi for async queries Active to allow progressing.

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.

1. 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.
2. 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.

3. 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.