Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Stop using deprecated API #526

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Nov 2, 2022

@@ -11,7 +11,6 @@
use SilverStripe\Core\Injector\Injector;
use SilverStripe\Dev\FunctionalTest;
use SilverStripe\Control\HTTPResponse;
use SilverStripe\Assets\Tests\Storage\AssetStoreTest\TestAssetStore;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deprecated class was unused

$store = reset($params);
if (!$store instanceof AssetContainer) {
/** @var AssetContainer $assetContainer */
$assetContainer = reset($params);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed this variable because it was confusing me

@@ -74,6 +75,9 @@ protected function tearDown(): void
*/
public function testSanityCheck()
{
if (Deprecation::get_is_enabled()) {
Copy link
Member Author

@emteknetnz emteknetnz Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I possibly could have called $this->markTestSkipped() in setUp(). However I still prefer to call $this->markTestSkipped() on test individually so that we're being consistent with most of the the other unit-tests

@emteknetnz emteknetnz force-pushed the pulls/1/stop-depr branch 2 times, most recently from 735bf3c to 68dcccb Compare November 7, 2022 05:01
{
parent::setUp();
if (Deprecation::get_is_enabled()) {
$this->markTestSkipped('Test calls deprecated code');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking test as skipped in setUp() rather than on individual test*() because this is a strange unit test where the bulk of the tests are in parent class FileIDHelperTester()

{
parent::setUp();
if (Deprecation::get_is_enabled()) {
$this->markTestSkipped('Test calls deprecated code');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking test as skipped in setUp() rather than on individual test*() because this is a strange unit test where the bulk of the tests are in parent class FileIDHelperTester()

@@ -899,6 +921,98 @@ public function listOfFilesToNormalise()
$public = AssetStore::VISIBILITY_PUBLIC;
$protected = AssetStore::VISIBILITY_PROTECTED;

/** @var FileIDHelper $hashHelper */
$hashHelper = new HashFileIDHelper();
Copy link
Member Author

@emteknetnz emteknetnz Nov 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a couple of the unit-test + provider combinations, I've duplicated the methods and retained the deprecated code in methods that have WithLegacy() suffixed to them.

@emteknetnz emteknetnz force-pushed the pulls/1/stop-depr branch 5 times, most recently from 4dd43cd to 895af35 Compare November 8, 2022 23:48
@emteknetnz emteknetnz marked this pull request as ready for review November 9, 2022 01:25
tests/php/Dev/Tasks/FileMigrationHelperTest.php Outdated Show resolved Hide resolved
tests/php/FileTest.php Outdated Show resolved Hide resolved
tests/php/Storage/AssetStoreTest.php Show resolved Hide resolved
tests/php/VersionedFilesMigratorTest.php Show resolved Hide resolved
@emteknetnz emteknetnz force-pushed the pulls/1/stop-depr branch 3 times, most recently from f59e1f0 to 2b75615 Compare November 15, 2022 04:39
@sabina-talipova sabina-talipova merged commit c98b8fb into silverstripe:1 Nov 16, 2022
@sabina-talipova sabina-talipova deleted the pulls/1/stop-depr branch November 16, 2022 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants