Implement the File Sanitization in a Separate Service to Make it More Reusable

Created on 18 August 2024, 4 months ago

Problem/Motivation

The current file sanitization approach is implemented directly in the event subscriber, which makes it difficult to reuse.

Proposed resolution

I suggest moving the file name sanitization to a separate service, which will make it more usable for other modules.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

โœจ Feature request
Status

Active

Version

10.3 โœจ

Component
File systemย  โ†’

Last updated about 20 hours ago

Created by

๐Ÿ‡ฏ๐Ÿ‡ดJordan Qusai Taha Amman

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

Merge Requests

Comments & Activities

  • Issue created by @Qusai Taha
  • Issue was unassigned.
  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Qusai Taha Amman
  • Pipeline finished with Failed
    4 months ago
    Total: 214s
    #257037
  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    MR should be against 11.x

    Most likely will have some backwards compatibility impact so will have to deal with that.

  • First commit to issue fork.
  • Pipeline finished with Failed
    4 months ago
    Total: 246s
    #258352
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia arunkumark Coimbatore

    arunkumark โ†’ changed the visibility of the branch 3468722-implement-the-file-name-sanitization-in-separate-service- to hidden.

  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia arunkumark Coimbatore

    As per feedback #4 the branch for 10.x is hidden. Created new PR for 11.x version.

  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    NW for linting fails.

  • Pipeline finished with Failed
    4 months ago
    Total: 38062s
    #258355
  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia arunkumark Coimbatore

    Tested the patch in the local PHPUnit it working fine. Attached screenshot for reference,

    The failure might be of ๐Ÿ“Œ Bump phpstan/phpstan to latest to make daily "updated deps" QA run pass again Fixed . It was tracked on ๐Ÿ› Updated deps job fails Needs review . Hope there is no action required on the MR.

    @kim.pepper please change the status if you have a different opinion.

  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    Don't think those two issues are related. Error is:

     ------ -------------------------------------------------------------- 
      Line   core/modules/file/src/FileSanitizeName.php                    
     ------ -------------------------------------------------------------- 
      79     Call to preg_quote() is missing delimiter / to be effective.  
      79     Call to preg_quote() is missing delimiter / to be effective.  
     ------ -------------------------------------------------------------- 
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    You need to add a service alias for 'Drupal\file\FileSanitizeName' => 'file.sanitize_name'

  • Pipeline finished with Failed
    4 months ago
    Total: 2175s
    #258810
  • Pipeline finished with Failed
    4 months ago
    Total: 744s
    #258886
  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan Qusai Taha Amman
  • Pipeline finished with Success
    4 months ago
    Total: 472s
    #259145
  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    I think the name FilenameSanitizer makes more sense.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia kim.pepper ๐Ÿ„โ€โ™‚๏ธ๐Ÿ‡ฆ๐Ÿ‡บSydney, Australia

    Can you also describe the use case where this would be used outside of the event handler?

  • Pipeline finished with Failed
    4 months ago
    Total: 250s
    #259940
  • Status changed to Needs review 4 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia arunkumark Coimbatore

    As per teh suggestion #14 the Filename updated.

    use case where this would be used outside of the event handler

    If any user wants to change the Filename, there is no need to override the Event subscriber. Instead, they can extend the service class and make their changes.

  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    Pipeline appears to be having failures.

  • Pipeline finished with Failed
    4 months ago
    Total: 489s
    #271451
  • ๐Ÿ‡ฏ๐Ÿ‡ดJordan oways23

    Re-roll patch 2

Production build 0.71.5 2024