Deprecate path.temporary in system.file configuration schema

Created on 9 November 2023, 7 months ago
Updated 15 November 2023, 7 months ago

Problem/Motivation

To land šŸ“Œ [PP-1] Configuration schema & required keys Needs review , we need to ensure that all properties in schema's are correct, Since šŸ“Œ Introduce a new #config_target Form API property to make it super simple to use validation constraints on simple config forms, and adopt it in several core config forms Fixed landed, we added configuration schema validation to system.file.
This property needs to be deprecated.

Steps to reproduce

N/A

Proposed resolution

Deprecate path.temporary in system.file

Remaining tasks

Review.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

šŸ“Œ Task
Status

Fixed

Version

10.2 āœØ

Component
File moduleĀ  ā†’

Last updated 4 days ago

Created by

šŸ‡§šŸ‡ŖBelgium borisson_ Mechelen, šŸ‡§šŸ‡Ŗ

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @borisson_
  • First commit to issue fork.
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    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.

  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    This blocks šŸ“Œ [PP-1] Configuration schema & required keys Needs review .

  • Status changed to Needs review 7 months ago
  • @wim-leers opened merge request.
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

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

  • Status changed to Needs work 7 months ago
  • šŸ‡ŗšŸ‡øUnited States smustgrave

    Seemed to cause a number of functional tests to fail.

  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    I was confused by the change record ā†’ . Not path, but path.temporary was deprecated. system_update_8801() provided the necessary update path. šŸ‘

  • Status changed to Needs review 7 months ago
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    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 
    
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    Down to a single failure!

    Turns out there's a new drupal-9.4.0.phpass.standard.php.gz fixture that was introduced in šŸ“Œ Replace custom password hashing library with PHP password_hash() Fixed 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.'
    
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    Green šŸ‘

  • Status changed to RTBC 7 months ago
  • šŸ‡§šŸ‡ŖBelgium borisson_ Mechelen, šŸ‡§šŸ‡Ŗ

    The change itself is really small, and the other changes to the fixtures are logical imho.

  • šŸ‡ŗšŸ‡øUnited States xjm

    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 future upgrade path tests for D10 upgrade hooks not work properly. Was the export created with a 9.4 codebase?

  • Status changed to Needs review 7 months ago
  • šŸ‡ŗšŸ‡øUnited States xjm

    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.

  • Assigned to Wim Leers
  • Status changed to Needs work 7 months ago
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ
  • Issue was unassigned.
  • Status changed to RTBC 7 months ago
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    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.

  • šŸ‡¬šŸ‡§United Kingdom catch

    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 (vaguely mentioned in #3305003: Create standard steps for creating database dumps ā†’ ).

  • šŸ‡ŗšŸ‡øUnited States xjm

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

    Saving credits.

  • Status changed to Needs work 7 months ago
  • šŸ‡ŗšŸ‡øUnited States xjm

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

  • Status changed to RTBC 7 months ago
  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

    Done! šŸ˜Š

    • xjm ā†’ committed e8f08891 on 11.x
      Issue #3400368 by Wim Leers, xjm, borisson_, catch: Deprecate path....
    • xjm ā†’ committed 57eac562 on 10.2.x
      Issue #3400368 by Wim Leers, xjm, borisson_, catch: Deprecate path....
  • Status changed to Fixed 7 months ago
  • šŸ‡ŗšŸ‡øUnited States xjm

    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!

  • šŸ‡§šŸ‡ŖBelgium Wim Leers Ghent šŸ‡§šŸ‡ŖšŸ‡ŖšŸ‡ŗ

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

    Good call! šŸ‘

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.69.0 2024