Deprecate path.temporary in system.file configuration schema

This was deprecated in #3039026: Deprecate file_directory_temp() and move to FileSystem service.

So why is it still in the config schema? Because it was necessary for tests. Why was it not deprecated? Because #2997100: Introduce a way to deprecate config schemas landed a year later.

This blocks #3364108: Configuration schema & required keys.

Existing change record updated: https://www.drupal.org/node/3039255.

Seemed to cause a number of functional tests to fail.

I was confused by the change record. Not path, but path.temporary was deprecated. system_update_8801() provided the necessary update path. 👍

This is tricky 😬🙃

Even though system_update_8801() should have run, it's been possible to regress in any given site because there has been no validation (which is true for all config except CKEditor 5's) and it was not yet deprecated yet (which is what this issue is doing). Setting a deprecated key-value pair would've triggered a test failure if a site had regressed.

… and that's literally what we're seeing here for some update path tests 😮

Investigation

The last update to the update path test fixtures happened in #3306545: Replace ckeditor with ckeditor5 in the 9.4.x database dumps in Drupal 10.0.x & 10.1.x.

1. So let's import both fixtures in the local MySQL instance:
   

   $ gunzip core/modules/system/tests/fixtures/update/drupal-9.4.0.bare.standard.php.gz 
   $ php -d memory_limit=-1 core/scripts/db-tools.php import core/modules/system/tests/fixtures/update/drupal-9.4.0.bare.standard.php --database-url mysql://root:root@localhost:3306/bare
   Import completed successfully.
   $ gunzip core/modules/system/tests/fixtures/update/drupal-9.4.0.filled.standard.php.gz 
   $ php -d memory_limit=-1 core/scripts/db-tools.php import core/modules/system/tests/fixtures/update/drupal-9.4.0.filled.standard.php --database-url mysql://root:root@localhost:3306/filled
   Import completed successfully.
   

2. Next, let's inspectsystem.file in the config table in both fixtures.

   ✅ The one in the "filled" fixture looks fine:

   a:4:{s:22:"allow_insecure_uploads";b:0;s:14:"default_scheme";s:6:"public";s:4:"path";a:0:{}s:21:"temporary_maximum_age";i:21600;}
   

   ❌ But the one in the "bare" fixture does NOT:

   a:4:{s:22:"allow_insecure_uploads";b:0;s:14:"default_scheme";s:6:"public";s:4:"path";a:1:{s:9:"temporary";s:26:"/Applications/MAMP/tmp/php";}s:21:"temporary_maximum_age";i:21600;}
   

Conclusion

Let's change the "bare" fixture to match the "filled" one to the same value in the DB, and then update the fixture:

$ php -d memory_limit=-1 core/scripts/db-tools.php dump-database-d8-mysql --database-url mysql://root:root@localhost:3306/bare > core/modules/system/tests/fixtures/update/drupal-9.4.0.bare.standard.php
$ gzip core/modules/system/tests/fixtures/update/drupal-9.4.0.bare.standard.php 

Did that in https://git.drupalcode.org/project/drupal/-/merge_requests/5387/diffs?co....

Down to a single failure!

Turns out there's a new drupal-9.4.0.phpass.standard.php.gz fixture that was introduced in #1845004: Replace custom password hashing library with PHP password_hash() and contains the same problem.

Same fix:

$ gunzip core/modules/system/tests/fixtures/update/drupal-9.4.0.phpass.standard.php.gz 
$ php -d memory_limit=-1 core/scripts/db-tools.php import core/modules/system/tests/fixtures/update/drupal-9.4.0.phpass.standard.php --database-url mysql://root:root@localhost:3306/phpass
Import completed successfully.

# Manually fix.

$ php -d memory_limit=-1 core/scripts/db-tools.php dump-database-d8-mysql --database-url mysql://root:root@localhost:3306/phpass > core/modules/system/tests/fixtures/update/drupal-9.4.0.phpass.standard.php
$ gzip core/modules/system/tests/fixtures/update/drupal-9.4.0.phpass.standard.php 
$ git commit -nam 'Fix the "phpass" fixture.'

Green 👍

This is an interesting issue. I had to read all the comments to understand what's going on (excellent explanations as always, thanks @Wim Leers).

That said, I'm not sure if re-exporting the 9.4 upgrade fixture is the correct choice. If this was done with a D10 codebase, it could pollute the fixture with D10 data and make upgrade path tests for other update hooks not work properly. Was the export created with a 9.4 codebase?

Setting NR to answer the above question (also pinged @Wim Leers in Slack). Meanwhile, asking the other committers just to make sure they have no concerns with this approach, since it's an odd sort of issue with a previously-impossible deprecation spanning four major releases.

No, I didn't do this from the 9.4.0 codebase.

But that is irrelevant AFAICT. All I did was db-tools.php import , modify one row manually in the database (i.e. without using Drupal), db-tools.php dump-database-d8-mysql, done.

Neither core/lib/Drupal/Core/Command/DbImportCommand.php nor \Drupal\Core\Command\DbDumpCommand have changed in any way since 9.4.0 (released June 15, 2022) that would make either the imported or dumped result different.

You would have a really good point if I had recreated the bare and phpass fixtures from scratch. But I literally modified one row in the DB directly, and did so in a way where I copied the literal string value from the filled fixture.

Yes it's OK to use the dump scripts from later releases, we actually had to do that with the 9.3.0 dumps at the time due to a bug in the 9.3.0 dump script that was only fixed in 9.4.something (vaguely mentioned in #3305003: Create standard steps for creating database dumps).

Thanks @Wim Leers and @catch; that info is helpful.

Saving credits.

I think we should update https://www.drupal.org/node/3039255 to mention this missing deprecation?

Done! 😊

Thanks @Wim Leers!

Committed to 11.x and 10.2.x as a newly surfaced deprecation. I also toggled the publish status of the CR to move it up the list for the additional deprecation.

Thanks everyone!

I also toggled the publish status of the CR to move it up the list for the additional deprecation.

Good call! 👏