Adds new file attachment deletion logic in the PhabricatorFile object from the file side and destruction engine extension from the object side and SQL patch for existing leftover attachments from deleted files.
Closes T15110
Details
Details
- Reviewers
avivey - Group Reviewers
O1: Blessed Committers - Maniphest Tasks
- T15110: File reference not removed when file is deleted
Upload file, delete file. No Unknown Object in Referenced Files curtain.
Diff Detail
Diff Detail
- Repository
- rP Phorge
- Branch
- master
- Lint
Lint Passed - Unit
Test Failures - Build Status
Buildable 82 Build 82: arc lint + arc unit
| Time | Test | |
|---|---|---|
| 0 ms | PhabricatorCelerityTestCase::testCelerityMaps EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage'
#0 [internal function]: PhutilErrorHandler::handleError(2, 'Code coverage n...', '/opt/arcanist/s...', 703, Array)
#1 /opt/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
| |
| 0 ms | PhabricatorConduitTestCase::testConduitMethods EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage'
#0 [internal function]: PhutilErrorHandler::handleError(2, 'Code coverage n...', '/opt/arcanist/s...', 703, Array)
#1 /opt/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
| |
| 0 ms | PhabricatorInfrastructureTestCase::testApplicationsInstalled EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage'
#0 [internal function]: PhutilErrorHandler::handleError(2, 'Code coverage n...', '/opt/arcanist/s...', 703, Array)
#1 /opt/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
| |
| 0 ms | PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage'
#0 [internal function]: PhutilErrorHandler::handleError(2, 'Code coverage n...', '/opt/arcanist/s...', 703, Array)
#1 /opt/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
| |
| 1 ms | PhabricatorLibraryTestCase::testEverythingImplemented EXCEPTION (RuntimeException): Code coverage needs to be enabled in php.ini by setting 'xdebug.mode' to 'coverage'
#0 [internal function]: PhutilErrorHandler::handleError(2, 'Code coverage n...', '/opt/arcanist/s...', 703, Array)
#1 /opt/arcanist/src/unit/engine/phutil/PhutilTestCase.php(703): xdebug_start_code_coverage(3)
| |
| View Full Test Results (7 Failed) |
Event Timeline
Comment Actions
I'm very curious about this improvement.
Feel free to share a comment about your current concerns on this change. I ask this since it's indicated as "planned changes".
Comment Actions
Organised attachment deletion from the file side into the File object and deletion from the object side into the destruction engine extension. Also updated the SQL patch date in the file name.
| resources/sql/autopatches/20230917.fileattachment.01.delete.sql | ||
|---|---|---|
| 1 ↗ | (On Diff #1390) | I'm a little worried about this migration; Do we have garbage collector for this? it would be safer to let it run. |
| src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php | ||
| 3 ↗ | (On Diff #1390) | I might not have enough coffee yet, but what and when is triggering this extension? |
| src/applications/files/storage/PhabricatorFile.php | ||
| 1713 ↗ | (On Diff #1390) | I don't like using mpull for the side-effect of the method invoked. |
| src/applications/files/storage/PhabricatorFile.php | ||
|---|---|---|
| 1713 ↗ | (On Diff #1390) | I agree, also because it returns an array of [null, null, null, null] which is not useful to read. A simple foreach() may be better |
| resources/sql/autopatches/20230917.fileattachment.01.delete.sql | ||
|---|---|---|
| 1 ↗ | (On Diff #1390) | Agreed! |
| resources/sql/autopatches/20230917.fileattachment.01.delete.sql | ||
|---|---|---|
| 1 ↗ | (On Diff #1390) | No there isn't any garbage collector for this specific issue (it's a bug originally from upstream so it's not intended for lingering file attachments to exist in the first place). I agree it's a destructive migration, although I don't see any other way to get rid of the orphaned records (which will clean up the UI spam in the Referenced Files curtain). I've ran the query a couple times locally, and as far as I can tell, it deletes the intended records which reference a file PHID which does not exist. Alternatively we could just change the UI logic to not show attachments that reference non-existent PHIDs, and leave the records left in the table, although I'd prefer not leaving a bunch of broken records using space/reducing efficiency. |
| src/applications/files/engineextension/PhabricatorFileAttachmentDestructionEngineExtension.php | ||
| 3 ↗ | (On Diff #1390) | |
Comment Actions
PhabricatorFileAttachmentQuery seems to eagerly load the object relation, however since the DestructionEngine extensions run after the object is already deleted, this fails and nothing is returned in the query. Changed to LiskDAO loadAllWhere, which also seems to use less queries in general (due to not eagerly loading), so changed the query in PhabricatorFile as well. Changed mpull to foreach per suggestion.
| resources/sql/autopatches/20230917.fileattachment.01.delete.sql | ||
|---|---|---|
| 1 ↗ | (On Diff #1390) | Probably we can take some inspiration from this file: |
| resources/sql/autopatches/20230917.fileattachment.01.delete.sql | ||
|---|---|---|
| 1 ↗ | (On Diff #1390) | I'm not sure a garbage collector would be any different to a migration, you'd still be running the same query but in just in different ways. |