Fix: Add strict check to media sync hooks #34
Conversation
ff2c9f4 to
ab3cc54
Compare
ab3cc54 to
73a1b22
Compare
|
Good call on that, @GaryJones. These are now separated, with #35 being the test suite. |
| } | ||
|
|
||
| /** | ||
| * @testdox Allow media syncs when required settings set |
There was a problem hiding this comment.
I think if you call phpunit with --testdox, then this line which duplicates the method name isn't needed. The --testdox flag will take the method name, strip a test_ prefix, and change underscores to spaces anyway.
There was a problem hiding this comment.
Admittedly, it is redundant in these specific test cases (@testdox comments are the same as the final output based on the method name). I do like that @testdox in docblocks could be used as an alternative test description. If the @testdox would be redundant, is it best to omit the comment entirely?
There was a problem hiding this comment.
If a test class method can't be made into a human-readable sentence, such that the @testdox would be needed, then sure, use it, but I'd question why the test class method couldn't be rewritten to make it a human-readable sentence in the first place.
I like running PHPUnit with --testdox and never felt the need to use @testdox, so if it would be redundant, then scrap it.
| parent::set_private_property( | ||
| $this->s3_media_sync::class, | ||
| $this->s3_media_sync, | ||
| 'settings', | ||
| $this->complete_settings | ||
| ); |
There was a problem hiding this comment.
See #35 (comment) - in that, having to force change the value of a private class property with Reflection seems like a huge code smell (I know you're working with what you've got for this particular hotfix). Having a Settings object, injected into the main class constructor, means that the class under test could just be instantiated here with a data provider of arrays / array key values.
There was a problem hiding this comment.
Agreed. I'm going to attempt to flush this out in #35.
73a1b22 to
2c756dc
Compare
|
Removed The tests are preserved here: |
Description
Added strict check before adding media sync hooks.
Previously, if s3 bucket settings were saved and then later removed, the
! empty( $this->settings )condition would still be truthy because the settings option is not empty. This would cause issues, as media sync hooks would still be added and cause errors during routine actions (e.g., deleting media in WP Admin and unintentionally runningdelete_attachment_from_s3).