Make WordPress Core

Opened 7 years ago

Last modified 5 years ago

#41978 reviewing defect (bug)

`_delete_all_data()` does not delete attachment files

Reported by: frank-klein's profile Frank Klein Owned by: johnbillion's profile johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version: 4.7
Component: Build/Test Tools Keywords: has-patch needs-testing
Focuses: Cc:


The _delete_all_data() function is called by WP_UnitTestCase::wpTearDownAfterClass(). The function deletes all the contents of the posts table in the tests database. However it does not delete any attachment files.

So when you create a shared attachment fixture in WP_UnitTestCase::wpSetUpBeforeClass(), _delete_all_data() will delete the attachment post in WP_UnitTestCase::wpTearDownAfterClass(), but leave the file.

There is no way to manually delete the attachment file in wpTearDownAfterClass(), because the attachment id no longer exists, and wp_delete_attachment() will therefore fail.

The solution: Instead of a direct SQL query, _delete_all_data() should use _delete_all_posts(), which accounts for attachments.

Attachments (1)

41978.patch (2.2 KB) - added by Frank Klein 7 years ago.

Download all attachments as: .zip

Change History (7)

@Frank Klein
7 years ago

#1 @Frank Klein
7 years ago

  • Keywords has-patch added

#2 @johnbillion
6 years ago

  • Keywords reporter-feedback needs-testing added
  • Milestone changed from Awaiting Review to 5.0
  • Owner set to johnbillion
  • Status changed from new to reviewing
  • Version changed from 4.9 to 4.7

I'm concerned about the performance impact of this on the tests. I'm going to run some numbers.

Is this strictly necessary if all tests that deal with attachments delete their own attachment files? Ref #38264

#3 @Frank Klein
6 years ago

I'm concerned about the performance impact of this on the tests.

This change does add an extra SQL query for every test class, which means more database load, and therefore worse performance.

However on every test method run, WP_UnitTestCase::scan_user_uploads() is called. So as the uploads directory fills with files, this would then be a slower operation. So this should improve performance through that, especially when running the entire test suite.

So not sure which one has more impact here.

Is this strictly necessary if all tests that deal with attachments delete their own attachment files?

No, it is not.

To me this issue boils down to how we want the test framework to behave. Right now, it is inconsistent in how it deals with fixtures: It deletes database fixtures, but not file fixtures.

This change would make the fixture deletion more consistent in that it deletes all fixtures.

However one might approach this differently, and say that tests need to delete their attachment fixtures themselves. One must be careful though, as #38264 shows, to delete the attachments before calling parent::tearDownAfterClass(). This is not obvious for developers that aren't very familiar with the behaviour of the testing framework.

Both approaches do not solve the consistency issue. So either we automatically delete all fixtures, or no fixtures.

Personally I'd prefer that tests do delete all of their fixtures manually. I acknowledge that this would be a lot of code churn, but it would make the test suite more transparent. Because the current behaviour is weird.

So we'd need to find a consensus, implement it, and document it, so that the behaviour is well understood by contributing developers, and developers using the testing framework for their own code.

#4 @desrosj
6 years ago

  • Keywords reporter-feedback removed

#5 @jorbin
6 years ago

  • Milestone changed from 5.0 to 5.1

#6 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

This ticket needs further discussion and a decision.

Note: See TracTickets for help on using tickets.