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 #10565

Merged

Conversation

emteknetnz
Copy link
Member

@emteknetnz emteknetnz commented Nov 2, 2022

Issue #10542

@@ -42,7 +45,9 @@ protected function setup(): void

protected function tearDown(): void
{
Deprecation::disable();
if (!$this->noticesWereEnabled) {
Copy link
Member Author

Choose a reason for hiding this comment

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

When doing multi-pr tests using silverstripe/installer, I'm calling Deprecation::enable() in _config.php. I've added funtionality to DeprecationTest to ensure we don't disable Deprecation notices after running DeprecatonTest

@@ -179,7 +179,7 @@ public static function get_enabled()
// noop
}

private static function get_is_enabled(): bool
public static function get_is_enabled(): bool
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 needs to be public now as we use this for skipping unit test of deprecated code, i.e.

if (Deprecation::get_is_enabled()) {
    $this->markTestSkipped('Test calls deprecated code');
}

@@ -15,10 +15,6 @@ class SessionAuthenticationHandlerTest extends SapphireTest

protected $usesDatabase = true;

/**
* @runInSeparateProcess
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.

These annotations were removed because they will cause phpunit to fail on any deprecations warnings e.g. swiftmailer is deprecated - running phpunit in isolation seems to upgrade deprecation notices to warnings


/**
* Migrates all 3.x file dataobjects to use the new DBFile field.
*
* @deprected 4.12.0 Will be removed without equivalent functionality to replace it
Copy link
Member Author

Choose a reason for hiding this comment

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

Deprecating this class because it uses a load of other deprecated classes from silverstripe/assets. It was used to migrate CMS 3 files.

@@ -200,12 +200,9 @@ private function cookieIsSecure(string $sameSite, bool $secure): bool

/**
* Get the correct samesite value - Session cookies use a different configuration variable.
*
* @deprecated 4.12.0 The relevant methods will include a `$sameSite` parameter instead.
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.

Undeprecating because there is no upgrade path. Also this is a private method so no need to deprecate.

@@ -586,13 +586,11 @@ public function save(HTTPRequest $request)
* Recursively apply the changes represented in $data to $dest.
* Used to update $_SESSION
*
* @deprecated 4.1.0 Use recursivelyApplyChanges() instead
Copy link
Member Author

Choose a reason for hiding this comment

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

Undeprecating because recursivelyApplyChanges() has different API - have raised issue to investigate

@@ -259,12 +259,9 @@ protected function bootErrorHandling()
* Get the environment type
*
* @return string
*
* @deprecated 4.12.0 Use Director::get_environment_type() instead
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.

Undeprecating because Director::get_environment_type() will call this method i.e. this deprecation was nonsensical

@@ -288,12 +285,9 @@ public function getEnvironment()
* Check or update any temporary environment specified in the session.
*
* @return null|string
*
* @deprecated 4.12.0 Use Director::get_session_environment_type() instead
Copy link
Member Author

Choose a reason for hiding this comment

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

Undeprecating because this is used by Director::sessionEnvironment()

@@ -622,26 +621,22 @@ public function getAttributes()
/**
* HTML-encoded label for this node, including css classes and other markup.
*
* @deprecated 4.0.1 Use setTitleField() instead
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.

Undeprecating this because getLabelField() because setTitleField() uses $this->titleField instead of $this->labelField

@@ -60,12 +59,9 @@ abstract class DBSchemaManager
/**
* @param string $table
* @param string $class
*
* @deprecated 4.0.1 Will be removed without equivalent functionality
Copy link
Member Author

Choose a reason for hiding this comment

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

Undeprecating this because it is used by DataObjectSchema::buildTableName()

@@ -24,18 +23,6 @@ class SQLSelectTest extends SapphireTest

protected $oldDeprecation = null;

protected function setUp(): void
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed because they called functionality that was previously removed from Deprecation

@@ -67,9 +66,6 @@ public function testSetField()

public function testGetArray()
{
$originalDeprecation = Deprecation::dump_settings();
Copy link
Member Author

Choose a reason for hiding this comment

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

Removed because this functionality was previously removed from Deprecation

@emteknetnz emteknetnz force-pushed the pulls/4/stop-depr branch 8 times, most recently from 228d227 to cb14003 Compare November 8, 2022 23:17
if (Director::isManifestFlushed()) {
/** @var BaseKernel $kernel */
$kernel = Injector::inst()->get(Kernel::class);
if ((method_exists($kernel, 'isFlushed') && $kernel->isFlushed())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

isFlushed() is not on the Kernel interface

if (!$flush || Director::isManifestFlushed()) {
/** @var BaseKernel $kernel */
$kernel = Injector::inst()->get(Kernel::class);
if (!$flush || (method_exists($kernel, 'isFlushed') && $kernel->isFlushed())) {
Copy link
Member Author

Choose a reason for hiding this comment

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

isFlushed() is not on the Kernel interface

@@ -179,12 +203,12 @@ public static function get_enabled()
// noop
}

private static function get_is_enabled(): bool
public static function isEnabled(): bool
Copy link
Member Author

Choose a reason for hiding this comment

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

Needs to be public so that it can be called by unit tests that wish to skip because they are testing deprecated code i.e.

if (Deprecation::isEnabled()) {
    $this->markTestSkipped('Test calls deprecated code');
}

@emteknetnz emteknetnz marked this pull request as ready for review November 9, 2022 01:25
src/Control/Middleware/HTTPCacheControlMiddleware.php Outdated Show resolved Hide resolved
src/Dev/Tasks/MigrateFileTask.php Show resolved Hide resolved
tests/php/Control/HTTPTest.php Outdated Show resolved Hide resolved
tests/php/Dev/DeprecationTest.php Show resolved Hide resolved
tests/php/Forms/CurrencyFieldTest.php Show resolved Hide resolved
tests/php/View/ViewableDataTest.php Show resolved Hide resolved
tests/php/View/SSViewerTest.php Show resolved Hide resolved
tests/php/Security/RandomGeneratorTest.php Show resolved Hide resolved
@@ -9,6 +9,7 @@
use SilverStripe\Security\PasswordValidator;
use SilverStripe\Security\Security;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Security\MemberAuthenticator\MemberAuthenticator;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use alphabetical order?

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 new import is needed in testCleartextPasswordsAreHashedWithDefaultAlgo()

use SilverStripe\Security\Member;
use SilverStripe\Dev\SapphireTest;
use SilverStripe\Core\Config\Config;
use SilverStripe\Control\HTTPRequest;
use SilverStripe\Security\MemberAuthenticator\SessionAuthenticationHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use alphabetical order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

@emteknetnz emteknetnz force-pushed the pulls/4/stop-depr branch 2 times, most recently from b90ca77 to d0911f5 Compare November 15, 2022 04:49
@sabina-talipova sabina-talipova merged commit ad116c6 into silverstripe:4 Nov 16, 2022
@sabina-talipova sabina-talipova deleted the pulls/4/stop-depr branch November 16, 2022 01:39
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