[Gravity] Add parameter types for FilesystemAccessObject classes

Review Request #869 — Created Sept. 26, 2022 and submitted

pprkut
Lunr
c094c5c...
lunr

Add parameter types for FilesystemAccessObject classes

Unit tests

  • 0
  • 0
  • 0
  • 1
  • 1
Description From Last Updated
smillernl
  1. 
      
  2. missing a type for the mode

    1. Nope, that's intentional. Can't put an int typehint there, because that would cast a string type "0777" to 777 which is wrong and we can no longer detect that.

    2. shouldn't the type be string then?

    3. No, for the same reasons. It would be cast to int and there's no way to reliably do that. We need it to be an integer, and we check that it's in a supported range.

      But 777 as int translates to a valid octal mode that is not 777.

    4. I'm very confused how it can't be captured in any type yet somehow still worked fine without one. https://github.com/symfony/filesystem/blob/6.1/Filesystem.php#L82-L87 seems to think that int is fine.

    5. If you're shifting responsibility for bad input to the user, int is fine. That's what PHP's internal function is doint too, and symphony's code is just a dumb wrapper.
      But we wanted to actually verify that the mode is correct because we ran into issues with it one too many times.

      The main problem is that we can't prevent strings from being cast to int in the typehint, and without that we don't know whether 777 was meant as the integer 777 or the octal 0777. By allowing string as input, we can rule that out, and be sure that whatever int comes in was meant to be like that and there's no casting in between that changes the value.

  3. 
      
pprkut
Review request changed

Status: Closed (submitted)

Change Summary:

Merged into master

Loading...